Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix installation of fish, enable autoconf configure argument #1016

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

norbusan
Copy link
Collaborator

No description provided.

@norbusan norbusan requested a review from abraunegg August 11, 2020 02:17
Copy link
Owner

@abraunegg abraunegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM though no way to test - I am assuming here that you have tested thus this is how this was found

@abraunegg abraunegg added this to the v2.4.5 milestone Aug 11, 2020
@norbusan
Copy link
Collaborator Author

norbusan commented Aug 11, 2020 via email

@abraunegg
Copy link
Owner

When building the Debian packages I realized that the fish completion was installed into $DESTDIR/@FISH_COMPLETION_DIR@ literally, so that must happen to each and everyone in fact ;-) -- --- PREINING Norbert                              https://www.preining.info Accelia Inc. + IFMGA ProGuide + TU Wien + JAIST + TeX Live + Debian Dev GPG: 0x860CDC13   fp: F7D8 A928 26E3 16A1 9FA0 ACF0 6CAC A448 860C DC13 Aug 11, 2020 19:00:03 abraunegg notifications@github.com:

@abraunegg approved this pull request. LGTM though no way to test - I am assuming here that you have tested thus this is how this was found — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[#1016 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AANHXJOUMBVPT3FGSRUOCOTSAEJCBANCNFSM4P2ROOJQ]. [https://github.com/notifications/beacon/AANHXJMVIL4AJDRY5HUAWN3SAEJCBA5CNFSM4P2ROOJ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODO3CJBI.gif]

Ahh ok .. not good ...

Bump / re-release 2.4.4 with this fix as 2.4.5 ?

@abraunegg abraunegg merged commit ff80e99 into master Aug 11, 2020
@SchoolGuy
Copy link
Contributor

I also just noticed this while upgrading the Devel package. Sadly when approving PR #991 I only tested the completion and not the build process. Sorry about that.

Regarding the release... I think it would be good to have that fix in because it is broken in 2.4.4 but I leave that to you @abraunegg

@norbusan norbusan deleted the fix-fish-installation branch August 12, 2020 17:25
@norbusan
Copy link
Collaborator Author

@SchoolGuy btw, what can be used as "default" installation directory? My patch uses /usr/loca/share/fish/completions but I'm not sure whether this actually would work.

@SchoolGuy
Copy link
Contributor

According to https://fishshell.com/docs/current/#writing-your-own-completions this belongs into /usr/share/fish/completions which is the directory my completions are living in on openSUSE.

@norbusan
Copy link
Collaborator Author

Mumumu, but I really don't like it when packages with --prefix=/usr/local install files outside this hierarchy. This is very inpolite in my opinion (hating Gnome/Gtk for that). Furthermore, it probably depends on with what kind of prefix fish was compiled. If it was compiled with --prefix=/opt/fish I guess it will not search for /usr/share/fish/completions, or?

@norbusan
Copy link
Collaborator Author

Ahh, doing echo $fish_completion_path in a fish shell shows:

echo $fish_complete_path 
/home/norbert/.config/fish/completions /etc/fish/completions /home/norbert/.local/share/flatpak/exports/share/fish/vendor_completions.d /var/lib/flatpak/exports/share/fish/vendor_completions.d /usr/local/share/fish/vendor_completions.d /usr/share/fish/vendor_completions.d /usr/share/fish/completions /home/norbert/.local/share/fish/generated_completions

at least on Debian. That means that /usr/local/share/fish/vendor_completions.d might be better to use. WDYT?

@abraunegg
Copy link
Owner

@norbusan , @SchoolGuy

Do we need to do another PR here or is this just a packaging issue?

Do we release 2.4.5 to ensure there is no issue?

@norbusan
Copy link
Collaborator Author

I think we can leave everything as is. fish is underdocumented and it is not clear whether there is a global directory that is always searched.

We could require that -with-fish-completion-dir is given a path instead of allowing auto and yes (as we do for zsh and bash, where the directories can be found automatically or are well known).

But I don't think this is really an issue ... those who want fish need to pass a proper path to the configure call.

@abraunegg
Copy link
Owner

@norbusan

OK .. so .. do we do a 2.4.5 release to make sure everything is well rounded - as - if fish completions are added now, it installs to a poor location (that this PR fixes) ?

@norbusan
Copy link
Collaborator Author

Fine with me

@SchoolGuy
Copy link
Contributor

@abraunegg Since we can override the directory via an option I am fine with 2.4.5

@SchoolGuy
Copy link
Contributor

@norbusan Well my packaging experience is limited so I don't have such strong opinions about packages installing stuff somewhere on the system. But yes I agree that docs can never be extensive enough.

@abraunegg
Copy link
Owner

@norbusan , @SchoolGuy

Will do a 2.4.5 release then later tonight

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants