Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 578826 - env-update support for app-shells/fish
Summary: env-update support for app-shells/fish
Status: CONFIRMED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Enhancement/Feature Requests (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords: PATCH
Depends on:
Blocks:
 
Reported: 2016-04-02 11:55 UTC by Dennis Schridde
Modified: 2017-02-23 05:17 UTC (History)
2 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
portage-2.2.28-env-update-fish-support.patch (portage-2.2.28-env-update-fish-support.patch,1.41 KB, patch)
2016-04-02 11:55 UTC, Dennis Schridde
Details | Diff
portage-2.2.28-env-update-fish-support.patch (portage-2.2.28-env-update-fish-support.patch,1.41 KB, patch)
2016-04-02 12:03 UTC, Dennis Schridde
Details | Diff
portage-2.2.28-env-update-fish-support.patch (portage-2.2.28-env-update-fish-support.patch,1.42 KB, patch)
2016-04-02 12:07 UTC, Dennis Schridde
Details | Diff
portage-2.2.28-env-update-fish-support.patch (portage-2.2.28-env-update-fish-support.patch,1.21 KB, patch)
2016-06-01 13:26 UTC, Dennis Schridde
Details | Diff
portage-2.2.28-env-update-fish-support.patch (portage-2.2.28-env-update-fish-support.patch,1.21 KB, patch)
2016-06-18 08:16 UTC, Dennis Schridde
Details | Diff
portage-2.2.28-env-update-fish-support.patch (portage-2.2.28-env-update-fish-support.patch,1.36 KB, patch)
2016-06-26 08:25 UTC, Dennis Schridde
Details | Diff
00-profile.fish (00-profile.fish,351 bytes, text/plain)
2016-06-26 08:25 UTC, Dennis Schridde
Details
modified fish ebuild (fish-2.4.0-r1.ebuild,2.07 KB, text/plain)
2017-01-12 05:34 UTC, Georgy Yakovlev
Details
/etc/profile.env parser for fish (profile-env.fish,680 bytes, text/plain)
2017-01-12 05:35 UTC, Georgy Yakovlev
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dennis Schridde 2016-04-02 11:55:10 UTC
Created attachment 429476 [details, diff]
portage-2.2.28-env-update-fish-support.patch

Exporting variables works slightly different in app-shells/fish, especially for the PATH variable. Hence /etc/profile.env cannot be sourced by the fish shell.

Attached patch makes env-update generate an /etc/profile.fish file that adheres to fish shell's syntax.

This one possible solution to bug #545830.
Comment 1 Dennis Schridde 2016-04-02 12:03:45 UTC
Created attachment 429478 [details, diff]
portage-2.2.28-env-update-fish-support.patch
Comment 2 Dennis Schridde 2016-04-02 12:07:22 UTC
Created attachment 429480 [details, diff]
portage-2.2.28-env-update-fish-support.patch
Comment 3 Dennis Schridde 2016-05-29 16:52:14 UTC
Any chance of getting this merged before 2.3? The patch still applies cleanly to sys-apps/portage-2.3.0_rc1. The generated /etc/profile.fish file still works with app-shells/fish-2.3.0.
Comment 4 Zac Medico gentoo-dev 2016-05-31 02:45:47 UTC
(In reply to Dennis Schridde from comment #2)
> Created attachment 429480 [details, diff] [details, diff]
> portage-2.2.28-env-update-fish-support.patch

The patch looks pretty reasonable, except for a few things:

It should use " ".join instead of string.join (string.join does not exist anymore in python3), like this:

    env_path = " ".join("'%s'" % v for v in specials["PATH"]))

Note that my version also uses specials["PATH"], which is a list containing the elements of PATH.

Also, env_keys should not include LDPATH, since that's a special onE that none of the other environment files export. We can re-use the filtered and sorted env_keys variable defined earlier in the function, as follows:

    for k in (x for x in env_keys if x != "PATH"):
        outfile.write("set -gx %s '%s'\n" % (k, env[k]))
Comment 5 Dennis Schridde 2016-06-01 13:26:47 UTC
Created attachment 436018 [details, diff]
portage-2.2.28-env-update-fish-support.patch

(In reply to Zac Medico from comment #4)
> The patch looks pretty reasonable, except for a few things:

Thanks for reviewing this! I implemented your suggestions and also took the liberty of porting the string formatting to str.format, which is afaik the preferred way to do this in Python 3. The patched env-update works with both Python 2.7.11 and 3.4.3.
Comment 6 Zac Medico gentoo-dev 2016-06-06 16:28:36 UTC
@polynomial-c: As app-shells/fish maintainer, please review.

(In reply to Dennis Schridde from comment #5)
> Created attachment 436018 [details, diff] [details, diff]
> portage-2.2.28-env-update-fish-support.patch

The patch looks pretty reasonable now. However, I wonder about the PATH setting:

    outfile.write("set -gx PATH {!s} $PATH\n".format(env_path))

I guess the $PATH part preserves an earlier PATH setting? Is that really needed? Will that cause the PATH variable to keep growing endlessly if the file is sourced multiple times?
Comment 7 Dennis Schridde 2016-06-07 06:36:12 UTC
(In reply to Zac Medico from comment #6)
> @polynomial-c: As app-shells/fish maintainer, please review.
> 
> (In reply to Dennis Schridde from comment #5)
> > Created attachment 436018 [details, diff] [details, diff] [details, diff]
> > portage-2.2.28-env-update-fish-support.patch
> 
> The patch looks pretty reasonable now. However, I wonder about the PATH
> setting:
> 
>     outfile.write("set -gx PATH {!s} $PATH\n".format(env_path))
> 
> I guess the $PATH part preserves an earlier PATH setting?

Yes

> Will that cause the PATH variable to keep growing endlessly if the
> file is sourced multiple times?

Yes

I'll remove it.
Comment 8 Dennis Schridde 2016-06-18 08:16:24 UTC
Created attachment 437932 [details, diff]
portage-2.2.28-env-update-fish-support.patch

(In reply to Dennis Schridde from comment #7)
> (In reply to Zac Medico from comment #6)
> > Will that cause the PATH variable to keep growing endlessly if the
> > file is sourced multiple times?
> 
> I'll remove it.

done
Comment 9 Dennis Schridde 2016-06-26 08:00:49 UTC
(In reply to Dennis Schridde from comment #8)
> done

This version requires that following file be included in sys-apps/baselayout (which is where the bash equivalent, /etc/profile, comes from) or app-shells/fish (where the directory /etc/fish/conf.d comes from):
```name=/etc/fish/conf.d/00-profile.fish
#!/usr/bin/fish

if test -f /etc/profile.fish
    source /etc/profile.fish
end

if [ "$EUID" = "0" ] ; or [ "$USER" = "root" ]
    set -xg PATH /usr/local/sbin /usr/local/bin /usr/sbin /usr/bin /sbin /bin $ROOTPATH
else
    set -xg PATH /usr/local/bin /usr/bin /bin $PATH
end
set -eug ROOTPATH
```

Otherwise the user will end up in a shell without /usr/bin, /bin and similar in the path.
Comment 10 Dennis Schridde 2016-06-26 08:11:16 UTC
P.S. This still messes up $fish_user_paths: It is supposed to prepend elements to $PATH, but it appears that /etc/fish/conf.d/*.fish is being sourced after doing this. Thus the 00-profile.fish file, that I pasted earlier, will override any user paths. I "fixed" this by adding the following to 00-profile.fish:
```
 Re-prepend $fish_user_paths
__fish_reconstruct_path
```

But I assume that more variables might be affected and that the same problem will affect other variable injection mechanisms like pam_env. Thus someone with more experience in this area should have a look at it.
Comment 11 Dennis Schridde 2016-06-26 08:25:07 UTC
Created attachment 438830 [details, diff]
portage-2.2.28-env-update-fish-support.patch

00-profile.fish also needs a changed patch to env-update to include ROOTPATH in the logic.
Comment 12 Dennis Schridde 2016-06-26 08:25:36 UTC
Created attachment 438832 [details]
00-profile.fish
Comment 13 Georgy Yakovlev gentoo-dev 2017-01-12 05:33:38 UTC
I wanted to bump this bug. patch works for me, but I'm not sure it's correct way to solve the problem.
I currently maintain a couple of fish related projects, this includes packaging for MacOS and it's brew package manager, and familiar with fish.


here are my points

1) according to upstream fish, file 00-profile.fish should go to:

 /usr/share/fish/vendor_conf.d

It's a special directory for distro vendor provided configurations not meant to be edited by regular users or admins.
see:
https://github.com/fish-shell/fish-shell/issues/1956#issuecomment-140583144
http://fishshell.com/docs/current/index.html#initialization

2) env-update should not explicitly support fish. there is no reason to have this functionality in portage by default and no reason to add new code. no reason to have an additional file in baselayout if a user never installs fish.fish can take care of itself.

3) fish ebuild should install a file to /usr/share/fish/vendor_conf.d that will read existing /etc/profile.env and parse it.

here is an example
https://github.com/gyakovlev/portage-overlay/tree/master/app-shells/fish
Comment 14 Georgy Yakovlev gentoo-dev 2017-01-12 05:34:03 UTC
Created attachment 459674 [details]
modified fish ebuild
Comment 15 Georgy Yakovlev gentoo-dev 2017-01-12 05:35:01 UTC
Created attachment 459676 [details]
/etc/profile.env parser for fish
Comment 16 Zac Medico gentoo-dev 2017-01-12 16:58:14 UTC
(In reply to Georgy Yakovlev from comment #13)
> 2) env-update should not explicitly support fish. there is no reason to have
> this functionality in portage by default and no reason to add new code. no
> reason to have an additional file in baselayout if a user never installs
> fish.fish can take care of itself.

That seems like a very good solution.


(In reply to Georgy Yakovlev from comment #15)
> Created attachment 459676 [details]
> /etc/profile.env parser for fish

Here's a tigher grep expression to ensure that only the desired lines are filtered:

egrep -v "^(#|export (PATH|ROOTPATH)=)" /etc/profile.env
Comment 17 Dennis Schridde 2017-01-15 13:46:36 UTC
bug #545830 now has a solution (implemented in app-shells/fish-2.4.0-r1), which is almost correct and does not require a modification of sys-apps/portage (env-update). When/if bug #545830 comment #14 is addressed, this bug here does not contain any additional value over the solution proposed there.

*** This bug has been marked as a duplicate of bug 545830 ***
Comment 18 Georgy Yakovlev gentoo-dev 2017-02-23 05:17:45 UTC
solution does non provide handling /etc/profile.d/*.sh files.
there are about 21 ebuilds in portage which utilize /etc/profile.d/*.sh files to add customization and some other ebuilds that I cannot simply grep.

>app-shells/autojump/autojump-22.2.4-r4.ebuild:  sed -e "s: \(/etc/profile.d\): \"${EPREFIX}\1\":" \
>app-shells/autojump/autojump-22.2.4-r4.ebuild:  insinto /etc/profile.d
>dev-vcs/colorcvs/colorcvs-1.4-r1.ebuild:        insinto /etc/profile.d
>gnustep-base/gnustep-make/gnustep-make-2.6.2.ebuild:    exeinto /etc/profile.d
>gnustep-base/gnustep-make/gnustep-make-2.6.6.ebuild:    exeinto /etc/profile.d
>gnustep-base/gnustep-make/gnustep-make-2.6.7-r1.ebuild: exeinto /etc/profile.d
>gnustep-base/gnustep-make/gnustep-make-2.6.7.ebuild:    exeinto /etc/profile.d
>gnustep-base/gnustep-make/gnustep-make-2.6.8.ebuild:    exeinto /etc/profile.d
>sci-chemistry/cns/cns-1.2.1-r8.ebuild:  insinto /etc/profile.d
>sci-chemistry/coot/coot-0.8.2-r1.ebuild:        source "${EPREFIX}/etc/profile.d/40ccp4.setup.sh"
>sys-apps/nix/nix-1.11.6-r3.ebuild:              rm "${ED}"/etc/profile.d/nix.sh || die
>sys-apps/nix/nix-1.11.6-r3.ebuild:              ewarn "${EROOT}etc/profile.d/nix.sh was removed (due to USE=-etc_profile)."
>sys-fs/udisks/udisks-1.0.5-r1.ebuild:   rm -f "${ED}"/etc/profile.d/udisks-bash-completion.sh
>sys-process/prll/prll-0.6.2.ebuild:     insinto /etc/profile.d/
>x11-libs/vte/vte-0.44.2.ebuild: mv "${D}"/etc/profile.d/vte{,-${SLOT}}.sh || die
>x11-libs/vte/vte-0.44.3.ebuild: mv "${D}"/etc/profile.d/vte{,-${SLOT}}.sh || die
>x11-libs/vte/vte-0.46.1.ebuild: mv "${D}"/etc/profile.d/vte{,-${SLOT}}.sh || die
>x11-misc/cdm/cdm-0.5.3.ebuild:  insinto /etc/profile.d/
>x11-misc/cdm/cdm-0.6.1_pre20130419.ebuild:      insinto /etc/profile.d/
>x11-terms/gnome-terminal/gnome-terminal-3.20.3.ebuild:  . /etc/profile.d/vte.sh"
>x11-terms/gnome-terminal/gnome-terminal-3.22.1.ebuild:  . /etc/profile.d/vte.sh"



major examples are:

sys-apps/baselayout-java
provides some variables like MANPATH, JAVA_HOME and gentoo_system/user_vm handling for eselect.

x11-libs/vte
provides some terminal quirks

x11-terms/gnome-terminal
uses above vte as backend.


So it's not a good idea to ship a semi-broken solution, unless someone wishes to fix ALL ebuilds above and provide fish equivalent profile.d file where it makes sense.