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

Special Treatment of MANPATH broken #4158

Closed
schwahn opened this Issue Jun 22, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@schwahn

schwahn commented Jun 22, 2017

Hi,
I noticed that with Fish 2.6 my man pages are not working as expected with Ubuntu 16.04.2 (no idea for other distros).
For the MANPATH environment variable, colons at the beginning, the end and double colons have special meaning. However, since MANPATHis now converted to an array and empty array elements are no longer allowed, MANPATHs that rely on the special meaning of the colon are no longer possible without hacky workarounds.

Example:
In this example, I want to prepend my custom man page locations to the man search path, so MANPATH should end with :

Actual Behavior:

$ set -xg MANPATH "/home/me/man" "/home/me/man2" ""
$ echo $MANPATH
> /home/me/man /home/me/man2 .
$ manpath
> manpath: warning: $MANPATH set, ignoring /etc/manpath.config
> /home/me/man:/home/me/man2:.

Expected Behavior:

$ set -xg MANPATH "/home/me/man" "/home/me/man2" ""
$ echo $MANPATH
> /home/me/man /home/me/man2 
$ manpath
> manpath: warning: $MANPATH set, appending /etc/manpath.config
> /home/me/man:/home/me/man:/usr/local/man:/usr/local/share/man:/usr/share/man

The problem occurs as empty array elements are replaced with . for the current directory.
This behavior was already discussed in Issue 2106. Not sure if that makes sense for MANPATH, though.

Hacky Workaround:

$ set -xg MANPATH "/home/me/man" "/home/me/man2:"
$ echo $MANPATH
> /home/me/man /home/me/man2:
$ manpath
> manpath: warning: $MANPATH set, appending /etc/manpath.config
> /home/me/man:/home/me/man:/usr/local/man:/usr/local/share/man:/usr/share/man

If done carefully, the workaround works for my use cases but it is probably not a nice solution :-(

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jun 22, 2017

Member

I did not know MANPATH worked like that. It also appears POSIX (in their infinite wisdom) has not standardized MANPATH on purpose, so there's nothing to rely on here - maybe e.g. mandoc doesn't do this.

I would expect that, to get the default value appended, I'd have to actually append it - i.e. set MANPATH like set MANPATH /some/directory (manpath | string split ":"). This however requires that you do that the first time you set MANPATH, since manpath prints the current effective value. Maybe the "--global" option works, but I'm not sure about that since it produces something different than just the default (in my case it adds some directories that are either non-existent or empty, so it wouldn't change anything). Apparently my man also has a "~/.manpath" configuration file, though that ironically doesn't seem to be documented anywhere.

Note that adding a ":" element also works - set -xg MANPATH "/home/me/man" "/home/me/man2" ":".

The annoying thing here is that the solution for #2106 has some desirable properties - it makes code like for bin in $PATH/* just work.

The question is what we should do about this, and (as always) I have a few options in mind:

  • Nothing. Don't support this feature in this way, instead require a "workaround"/"different solution".

  • Revert #2106 completely. Since it's quite nice for $PATH I don't really want to do that.

  • Don't do the "." replacement for $MANPATH specifically. This adds one more special case, so I also don't quite like it.

Personally, I'm leaning towards the former. If you really want this (rather obscure) feature, set a : element. Does that sound okay to you?

Member

faho commented Jun 22, 2017

I did not know MANPATH worked like that. It also appears POSIX (in their infinite wisdom) has not standardized MANPATH on purpose, so there's nothing to rely on here - maybe e.g. mandoc doesn't do this.

I would expect that, to get the default value appended, I'd have to actually append it - i.e. set MANPATH like set MANPATH /some/directory (manpath | string split ":"). This however requires that you do that the first time you set MANPATH, since manpath prints the current effective value. Maybe the "--global" option works, but I'm not sure about that since it produces something different than just the default (in my case it adds some directories that are either non-existent or empty, so it wouldn't change anything). Apparently my man also has a "~/.manpath" configuration file, though that ironically doesn't seem to be documented anywhere.

Note that adding a ":" element also works - set -xg MANPATH "/home/me/man" "/home/me/man2" ":".

The annoying thing here is that the solution for #2106 has some desirable properties - it makes code like for bin in $PATH/* just work.

The question is what we should do about this, and (as always) I have a few options in mind:

  • Nothing. Don't support this feature in this way, instead require a "workaround"/"different solution".

  • Revert #2106 completely. Since it's quite nice for $PATH I don't really want to do that.

  • Don't do the "." replacement for $MANPATH specifically. This adds one more special case, so I also don't quite like it.

Personally, I'm leaning towards the former. If you really want this (rather obscure) feature, set a : element. Does that sound okay to you?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 23, 2017

Contributor

My apologies for the breakage. The BSD manpath doesn't have those semantics and I didn't think to check the GNU implementation. And as @faho noted I was striving for consistency in how we treated these three magic env vars. We definitely do not want to revert #2106 in its entirety. So the choices are to keep the current behavior or special-case MANPATH. I'm leaning towards the latter despite the inconsistency because empty elements in MANPATH don't have a consistent, universally implemented, meaning. And not transforming those empty elements to . (the cwd) doesn't materially affect fish. Which is unlike PATH and CDPATH where empty elements are by definition on all UNIX implementations I'm familiar with equivalent to ..

Contributor

krader1961 commented Jun 23, 2017

My apologies for the breakage. The BSD manpath doesn't have those semantics and I didn't think to check the GNU implementation. And as @faho noted I was striving for consistency in how we treated these three magic env vars. We definitely do not want to revert #2106 in its entirety. So the choices are to keep the current behavior or special-case MANPATH. I'm leaning towards the latter despite the inconsistency because empty elements in MANPATH don't have a consistent, universally implemented, meaning. And not transforming those empty elements to . (the cwd) doesn't materially affect fish. Which is unlike PATH and CDPATH where empty elements are by definition on all UNIX implementations I'm familiar with equivalent to ..

@krader1961 krader1961 added the bug label Jun 23, 2017

@krader1961 krader1961 added this to the fish 2.7.0 milestone Jun 23, 2017

@schwahn

This comment has been minimized.

Show comment
Hide comment
@schwahn

schwahn Jun 23, 2017

Thanks for the quick reply. I agree that the special colon semantics of MANPATH are weird and that adding a special case here is probably not worth it. I updated my environment to use the trick of adding a : element (I did not notice this option previously). For me that is fine, but maybe this could be worth mentioning somewhere in the docs? From a quick skim, I got the impression that the handling of empty array elements for PATH, CDPATH, and MANPATH is only mentioned in the changelog?

schwahn commented Jun 23, 2017

Thanks for the quick reply. I agree that the special colon semantics of MANPATH are weird and that adding a special case here is probably not worth it. I updated my environment to use the trick of adding a : element (I did not notice this option previously). For me that is fine, but maybe this could be worth mentioning somewhere in the docs? From a quick skim, I got the impression that the handling of empty array elements for PATH, CDPATH, and MANPATH is only mentioned in the changelog?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 24, 2017

Contributor

I'm going to remove MANPATH from the vars that are mangled by converting empty elements to .. In hindsight treating all three vars consistently was a mistake since MANPATH is a very different var (at least with respect to how fish uses it) when compared to CDPATH and PATH.

Contributor

krader1961 commented Jun 24, 2017

I'm going to remove MANPATH from the vars that are mangled by converting empty elements to .. In hindsight treating all three vars consistently was a mistake since MANPATH is a very different var (at least with respect to how fish uses it) when compared to CDPATH and PATH.

@bernhardreiter

This comment has been minimized.

Show comment
Hide comment
@bernhardreiter

bernhardreiter Aug 29, 2017

For fish 2.6.0 here is a workaround when trying to use a MANPATH variable that has already been set to something before fish was started, e.g. /home/intevation/shared/man:.
Add the code below to ~/.config/fish/config.fish or so.

# replaces dot elements in the $MANPATH array with colons
# because fish 2.6.0 will split MANPATH by colons into an array.
#
# This helps the command "manpath" on a GNU system to find the default manpages
#
# It is not perfect: there may be too many colons in the final manpath
# but this is considered to be a non-issue, as this is the place where the
# system config path elements should be inserted anyway and once the current
# directory is included it does not hurt to include it again.
#
# future fish shells (version 3?) may provide a better solution here
# see https://github.com/fish-shell/fish-shell/issues/2090
#     https://github.com/fish-shell/fish-shell/issues/4158   
for i in (seq 1 (count $MANPATH))
    if test $MANPATH[$i] = .
        set MANPATH[$i] :
    end
end

bernhardreiter commented Aug 29, 2017

For fish 2.6.0 here is a workaround when trying to use a MANPATH variable that has already been set to something before fish was started, e.g. /home/intevation/shared/man:.
Add the code below to ~/.config/fish/config.fish or so.

# replaces dot elements in the $MANPATH array with colons
# because fish 2.6.0 will split MANPATH by colons into an array.
#
# This helps the command "manpath" on a GNU system to find the default manpages
#
# It is not perfect: there may be too many colons in the final manpath
# but this is considered to be a non-issue, as this is the place where the
# system config path elements should be inserted anyway and once the current
# directory is included it does not hurt to include it again.
#
# future fish shells (version 3?) may provide a better solution here
# see https://github.com/fish-shell/fish-shell/issues/2090
#     https://github.com/fish-shell/fish-shell/issues/4158   
for i in (seq 1 (count $MANPATH))
    if test $MANPATH[$i] = .
        set MANPATH[$i] :
    end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment