[PATCH] search-paths: 'environment-variable-definition' output for fish

  • Open
  • quality assurance status badge
Details
3 participants
  • Dan Frumin
  • Ludovic Courtès
  • Meiyo Peng
Owner
unassigned
Submitted by
Dan Frumin
Severity
normal
D
D
Dan Frumin wrote on 31 May 2019 12:36
(address . guix-patches@gnu.org)(name . Dan Frumin)(address . dfrumin@cs.ru.nl)
20190531103630.6739-1-dfrumin@cs.ru.nl
---
guix/search-paths.scm | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)

Toggle diff (44 lines)
diff --git a/guix/search-paths.scm b/guix/search-paths.scm
index 002e6342bb..fe9253e88e 100644
--- a/guix/search-paths.scm
+++ b/guix/search-paths.scm
@@ -177,15 +177,28 @@ current value), or 'suffix (return the definition where VALUE is added as a
suffix to VARIABLE's current value.) In the case of 'prefix and 'suffix,
SEPARATOR is used as the separator between VARIABLE's current value and its
prefix/suffix."
- (match (if (not separator) 'exact kind)
- ('exact
- (format #f "export ~a=\"~a\"" variable value))
- ('prefix
- (format #f "export ~a=\"~a${~a:+~a}$~a\""
- variable value variable separator variable))
- ('suffix
- (format #f "export ~a=\"$~a${~a:+~a}~a\""
- variable variable variable separator value))))
+ (let* ([shell-env (getenv "SHELL")]
+ [is-fish? (and shell-env
+ (equal? (last (string-split shell-env #\/))
+ "fish"))])
+ (match (if (not separator) 'exact kind)
+ ('exact
+ (if is-fish?
+ ;; See <https://fishshell.com/docs/current/commands.html#set> for syntax
+ (format #f "set -x ~a \"~a\"" variable value)
+ (format #f "export ~a=\"~a\"" variable value)))
+ ('prefix
+ (if is-fish?
+ (format #f "set -x ~a \"~a\" $~a"
+ variable value variable)
+ (format #f "export ~a=\"~a${~a:+~a}$~a\""
+ variable value variable separator variable)))
+ ('suffix
+ (if is-fish?
+ (format #f "set -x ~a $~a \"~a\""
+ variable variable value)
+ (format #f "export ~a=\"$~a${~a:+~a}~a\""
+ variable variable variable separator value))))))
(define* (search-path-definition search-path value
#:key (kind 'exact))
--
2.17.1
D
D
Dan Frumin wrote on 31 May 2019 12:41
(address . 36021@debbugs.gnu.org)
4c3d1d91-18ae-954f-2f0f-a979a974fb2c@cs.ru.nl
Hi Guix!

Some background on this patch:
Right now whenever I do any Guix operation that requires me to modify environment variables (e.g. installing a Guile library requires me to update
$GUILE_LOAD_PATH afterwards), Guix helpful tells me what commands I have to run to update the variables.

However, those commands are currently in bash/POSIX(?) format `export VAR=VALUE`. I've modified the `environment-variable-definition` function to
support the syntax for Fish shell as well. I don't know if this method of looking at the $SHELL variable is sound, but it works on my machine.

Documentation for the `set' function in Fish: https://fishshell.com/docs/current/commands.html#set

PS: this is my first non-package patch for Guix so I apologize if there is something wrong with the patch

Best regards,
-Dan
L
L
Ludovic Courtès wrote on 1 Jun 2019 15:10
Re: [bug#36021] [PATCH] search-paths: 'environment-variable-definition' output for fish
(name . Dan Frumin)(address . dfrumin@cs.ru.nl)(address . 36021@debbugs.gnu.org)
87v9xpfo3d.fsf@gnu.org
Hi,

Dan Frumin <dfrumin@cs.ru.nl> skribis:

Toggle quote (6 lines)
> Some background on this patch:
> Right now whenever I do any Guix operation that requires me to modify
> environment variables (e.g. installing a Guile library requires me to
> update $GUILE_LOAD_PATH afterwards), Guix helpful tells me what
> commands I have to run to update the variables.


Toggle quote (6 lines)
> However, those commands are currently in bash/POSIX(?) format `export
> VAR=VALUE`. I've modified the `environment-variable-definition`
> function to support the syntax for Fish shell as well. I don't know if
> this method of looking at the $SHELL variable is sound, but it works
> on my machine.

“export VAR=VALUE” is actually Bash-specific. The POSIX way to do it
is:

VAR=VALUE; export VAR

Would that work with Fish?

If it does, we might just as well take that route as it will also cater
to other POSIX-compatible shells.

If not, your patch sounds like the right way.

Thanks,
Ludo’.
D
D
Dan Frumin wrote on 2 Jun 2019 11:29
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 36021@debbugs.gnu.org)
ff171fad-0650-0967-0bea-154216a7f866@cs.ru.nl
Hi Ludovic,

On 01-06-19 15:10, Ludovic Courtès wrote:
Toggle quote (13 lines)
> Hi,
>
> Dan Frumin <dfrumin@cs.ru.nl> skribis:
>
>> Some background on this patch:
>> Right now whenever I do any Guix operation that requires me to modify
>> environment variables (e.g. installing a Guile library requires me to
>> update $GUILE_LOAD_PATH afterwards), Guix helpful tells me what
>> commands I have to run to update the variables.
>
> But see <https://issues.guix.gnu.org/issue/35942>. :-)
>

I was actually oblivious to the fact that these environment variables can be set up for you automatically in a new shell -- I guess that's because
both ~/.guix-profile/etc/profile and `guix package --search-paths` output everything in Bash format, so I didn't use it with Fish.

Toggle quote (13 lines)
>> However, those commands are currently in bash/POSIX(?) format `export
>> VAR=VALUE`. I've modified the `environment-variable-definition`
>> function to support the syntax for Fish shell as well. I don't know if
>> this method of looking at the $SHELL variable is sound, but it works
>> on my machine.
>
> “export VAR=VALUE” is actually Bash-specific. The POSIX way to do it
> is:
>
> VAR=VALUE; export VAR
>
> Would that work with Fish?

Unfortunately not. I wish they'd support more standard features.

Best,
Dan

Toggle quote (9 lines)
>
> If it does, we might just as well take that route as it will also cater
> to other POSIX-compatible shells.
>
> If not, your patch sounds like the right way.
>
> Thanks,
> Ludo’.
>
M
M
Meiyo Peng wrote on 3 Jun 2019 07:51
(name . Dan Frumin)(address . dfrumin@cs.ru.nl)(address . 36021@debbugs.gnu.org)
87o93fkyhs.fsf@riseup.net
Hi Dan,

Dan Frumin writes:

Toggle quote (15 lines)
> Some background on this patch:
> Right now whenever I do any Guix operation that requires me to modify
> environment variables (e.g. installing a Guile library requires me to update
> $GUILE_LOAD_PATH afterwards), Guix helpful tells me what commands I have to run
> to update the variables.
>
> However, those commands are currently in bash/POSIX(?) format `export
> VAR=VALUE`. I've modified the `environment-variable-definition` function to
> support the syntax for Fish shell as well. I don't know if this method of
> looking at the $SHELL variable is sound, but it works on my machine.
>
> Documentation for the `set' function in Fish: https://fishshell.com/docs/current/commands.html#set
>
> PS: this is my first non-package patch for Guix so I apologize if there is something wrong with the patch

I think this patch will cause more trouble than good. Does this patch
replaces etc/profile with fish syntax script? etc/profile is supposed
to be in POSIX shell syntax. Fish shell has it's own configuration
files in /etc/fish/config.fish and /etc/fish/conf.d/*.fish. If you
really want to generate fish syntax scripts, please put them in
/etc/fish/config.fish or /etc/fish/conf.d/*.fish.

See:

Have you tried the fish-foreign-env package? You can source bash
scripts in fish via `fenv source ~/.guix-profile/etc/profile`. That's how
the fish shell in Guix set up environment variables upon login.

Perhaps we can handle this better. Do you have any suggestions? My
idea:

1. The fish-foreign-env package is very useful but it's not installed by
default in order to avoid polluting user's profiles according to the
Guix convention. In this case I think it's a useful utility rather than
pollution. Do you want it to be installed together with the fish
package by default?

2. etc/profile is supposed to be sourced by a login shell. And we
should avoid sourcing it from a non-login shell. That's why I made the
fish package in Guix only sources it upon login. That means starting a
new fish shell won't source it automatically. So please source it
manually if you need the new environment variables (fenv source
~/.guix-profile/etc/profile).

3. We can write better documentations explaining how to deal with fish
shell in Guix. There's an old discussion about having a Guix wiki or
a Guix book.


--
Meiyo Peng
D
D
Dan Frumin wrote on 3 Jun 2019 17:50
(name . Meiyo Peng)(address . meiyo@riseup.net)(address . 36021@debbugs.gnu.org)
cf1f869c-78c3-68c3-d2ec-4b8faa31cb70@cs.ru.nl
Hi Meiyo,

On 03-06-19 07:51, Meiyo Peng wrote:
Toggle quote (27 lines)
> Hi Dan,
>
> Dan Frumin writes:
>
>> Some background on this patch:
>> Right now whenever I do any Guix operation that requires me to modify
>> environment variables (e.g. installing a Guile library requires me to update
>> $GUILE_LOAD_PATH afterwards), Guix helpful tells me what commands I have to run
>> to update the variables.
>>
>> However, those commands are currently in bash/POSIX(?) format `export
>> VAR=VALUE`. I've modified the `environment-variable-definition` function to
>> support the syntax for Fish shell as well. I don't know if this method of
>> looking at the $SHELL variable is sound, but it works on my machine.
>>
>> Documentation for the `set' function in Fish: https://fishshell.com/docs/current/commands.html#set
>>
>> PS: this is my first non-package patch for Guix so I apologize if there is something wrong with the patch
>
> I think this patch will cause more trouble than good. Does this patch
> replaces etc/profile with fish syntax script? etc/profile is supposed
> to be in POSIX shell syntax. Fish shell has it's own configuration
> files in /etc/fish/config.fish and /etc/fish/conf.d/*.fish. If you
> really want to generate fish syntax scripts, please put them in
> /etc/fish/config.fish or /etc/fish/conf.d/*.fish.
>

No, this patch doesn't touch etc/profile in any way, it is only concerned with the output that you get from `guix package` on your terminal.
So I guess it's kind of orthogonal to the upgraded fish package that you made?

Toggle quote (10 lines)
> See:
> 1. https://lists.gnu.org/archive/html/guix-devel/2019-01/msg00071.html
> 2. https://lists.gnu.org/archive/html/guix-devel/2019-01/msg00333.html
> 3. https://issues.guix.gnu.org/issue/34153
>
> Have you tried the fish-foreign-env package? You can source bash
> scripts in fish via `fenv source ~/.guix-profile/etc/profile`. That's how
> the fish shell in Guix set up environment variables upon login.
>

Thanks! This is quite nice, I should upgrade my Fish installation.

Toggle quote (9 lines)
> Perhaps we can handle this better. Do you have any suggestions? My
> idea:
>
> 1. The fish-foreign-env package is very useful but it's not installed by
> default in order to avoid polluting user's profiles according to the
> Guix convention. In this case I think it's a useful utility rather than
> pollution. Do you want it to be installed together with the fish
> package by default?

Well, you added it as part of the fish dependency, right? I think it makes sense, especially on Guix SD.

Toggle quote (8 lines)
>
> 2. etc/profile is supposed to be sourced by a login shell. And we
> should avoid sourcing it from a non-login shell. That's why I made the
> fish package in Guix only sources it upon login. That means starting a
> new fish shell won't source it automatically. So please source it
> manually if you need the new environment variables (fenv source
> ~/.guix-profile/etc/profile).

I am not knowledgeable enough to comment on this, but it seems to me that it is done similarly if you are using bash: e.g. you have to source the
profile yourself if you want to get it in a non-login shell.

Toggle quote (6 lines)
>
> 3. We can write better documentations explaining how to deal with fish
> shell in Guix. There's an old discussion about having a Guix wiki or
> a Guix book.
>

Yeah, more documentation is always better :)

Best regards,
-Dan

Toggle quote (5 lines)
>
> --
> Meiyo Peng
> https://www.pengmeiyu.com/
>
M
M
Meiyo Peng wrote on 4 Jun 2019 05:53
(name . Dan Frumin)(address . dfrumin@cs.ru.nl)(address . 36021@debbugs.gnu.org)
87muiy0zxd.fsf@riseup.net
Hi Dan,

Dan Frumin writes:

Toggle quote (26 lines)
>>> Some background on this patch:
>>> Right now whenever I do any Guix operation that requires me to modify
>>> environment variables (e.g. installing a Guile library requires me to update
>>> $GUILE_LOAD_PATH afterwards), Guix helpful tells me what commands I have to run
>>> to update the variables.
>>>
>>> However, those commands are currently in bash/POSIX(?) format `export
>>> VAR=VALUE`. I've modified the `environment-variable-definition` function to
>>> support the syntax for Fish shell as well. I don't know if this method of
>>> looking at the $SHELL variable is sound, but it works on my machine.
>>>
>>> Documentation for the `set' function in Fish: https://fishshell.com/docs/current/commands.html#set
>>>
>>> PS: this is my first non-package patch for Guix so I apologize if there is something wrong with the patch
>>
>> I think this patch will cause more trouble than good. Does this patch
>> replaces etc/profile with fish syntax script? etc/profile is supposed
>> to be in POSIX shell syntax. Fish shell has it's own configuration
>> files in /etc/fish/config.fish and /etc/fish/conf.d/*.fish. If you
>> really want to generate fish syntax scripts, please put them in
>> /etc/fish/config.fish or /etc/fish/conf.d/*.fish.
>>
>
> No, this patch doesn't touch etc/profile in any way, it is only concerned with the output that you get from `guix package` on your terminal.
> So I guess it's kind of orthogonal to the upgraded fish package that you made?

Alright. Then that's fine. But there is an on going discussion about
change content of the message in bug #35942. I suggest we hold this
patch until that bug is closed.

Toggle quote (12 lines)
>> See:
>> 1. https://lists.gnu.org/archive/html/guix-devel/2019-01/msg00071.html
>> 2. https://lists.gnu.org/archive/html/guix-devel/2019-01/msg00333.html
>> 3. https://issues.guix.gnu.org/issue/34153
>>
>> Have you tried the fish-foreign-env package? You can source bash
>> scripts in fish via `fenv source ~/.guix-profile/etc/profile`. That's how
>> the fish shell in Guix set up environment variables upon login.
>>
>
> Thanks! This is quite nice, I should upgrade my Fish installation.

Your fish shell is quite old. Older than Guix 1.0.0!

Toggle quote (11 lines)
>> Perhaps we can handle this better. Do you have any suggestions? My
>> idea:
>>
>> 1. The fish-foreign-env package is very useful but it's not installed by
>> default in order to avoid polluting user's profiles according to the
>> Guix convention. In this case I think it's a useful utility rather than
>> pollution. Do you want it to be installed together with the fish
>> package by default?
>
> Well, you added it as part of the fish dependency, right? I think it makes sense, especially on Guix SD.

Yes, it's a dependency of fish. But it's not a propagated input. We
can change it into a propagated input if most users agree.

Toggle quote (20 lines)
>> 2. etc/profile is supposed to be sourced by a login shell. And we
>> should avoid sourcing it from a non-login shell. That's why I made the
>> fish package in Guix only sources it upon login. That means starting a
>> new fish shell won't source it automatically. So please source it
>> manually if you need the new environment variables (fenv source
>> ~/.guix-profile/etc/profile).
>
> I am not knowledgeable enough to comment on this, but it seems to me that it is
> done similarly if you are using bash: e.g. you have to source the profile
> yourself if you want to get it in a non-login shell.
>
>>
>> 3. We can write better documentations explaining how to deal with fish
>> shell in Guix. There's an old discussion about having a Guix wiki or
>> a Guix book.
>>
>
> Yeah, more documentation is always better :)


--
Meiyo Peng
D
D
Dan Frumin wrote on 4 Jun 2019 09:48
(address . 36021@debbugs.gnu.org)
7f3c985d-2f61-e148-0438-d1dbee2df899@cs.ru.nl
Hi,

On 04-06-19 05:53, Meiyo Peng wrote:
Toggle quote (36 lines)
> Hi Dan,
>
> Dan Frumin writes:
>
>>>> Some background on this patch:
>>>> Right now whenever I do any Guix operation that requires me to modify
>>>> environment variables (e.g. installing a Guile library requires me to update
>>>> $GUILE_LOAD_PATH afterwards), Guix helpful tells me what commands I have to run
>>>> to update the variables.
>>>>
>>>> However, those commands are currently in bash/POSIX(?) format `export
>>>> VAR=VALUE`. I've modified the `environment-variable-definition` function to
>>>> support the syntax for Fish shell as well. I don't know if this method of
>>>> looking at the $SHELL variable is sound, but it works on my machine.
>>>>
>>>> Documentation for the `set' function in Fish: https://fishshell.com/docs/current/commands.html#set
>>>>
>>>> PS: this is my first non-package patch for Guix so I apologize if there is something wrong with the patch
>>>
>>> I think this patch will cause more trouble than good. Does this patch
>>> replaces etc/profile with fish syntax script? etc/profile is supposed
>>> to be in POSIX shell syntax. Fish shell has it's own configuration
>>> files in /etc/fish/config.fish and /etc/fish/conf.d/*.fish. If you
>>> really want to generate fish syntax scripts, please put them in
>>> /etc/fish/config.fish or /etc/fish/conf.d/*.fish.
>>>
>>
>> No, this patch doesn't touch etc/profile in any way, it is only concerned with the output that you get from `guix package` on your terminal.
>> So I guess it's kind of orthogonal to the upgraded fish package that you made?
>
> Alright. Then that's fine. But there is an on going discussion about
> change content of the message in bug #35942. I suggest we hold this
> patch until that bug is closed.
>


Sure, makes sense to me.

Toggle quote (52 lines)
>>> See:
>>> 1. https://lists.gnu.org/archive/html/guix-devel/2019-01/msg00071.html
>>> 2. https://lists.gnu.org/archive/html/guix-devel/2019-01/msg00333.html
>>> 3. https://issues.guix.gnu.org/issue/34153
>>>
>>> Have you tried the fish-foreign-env package? You can source bash
>>> scripts in fish via `fenv source ~/.guix-profile/etc/profile`. That's how
>>> the fish shell in Guix set up environment variables upon login.
>>>
>>
>> Thanks! This is quite nice, I should upgrade my Fish installation.
>
> Your fish shell is quite old. Older than Guix 1.0.0!
>
>>> Perhaps we can handle this better. Do you have any suggestions? My
>>> idea:
>>>
>>> 1. The fish-foreign-env package is very useful but it's not installed by
>>> default in order to avoid polluting user's profiles according to the
>>> Guix convention. In this case I think it's a useful utility rather than
>>> pollution. Do you want it to be installed together with the fish
>>> package by default?
>>
>> Well, you added it as part of the fish dependency, right? I think it makes sense, especially on Guix SD.
>
> Yes, it's a dependency of fish. But it's not a propagated input. We
> can change it into a propagated input if most users agree.
>
>>> 2. etc/profile is supposed to be sourced by a login shell. And we
>>> should avoid sourcing it from a non-login shell. That's why I made the
>>> fish package in Guix only sources it upon login. That means starting a
>>> new fish shell won't source it automatically. So please source it
>>> manually if you need the new environment variables (fenv source
>>> ~/.guix-profile/etc/profile).
>>
>> I am not knowledgeable enough to comment on this, but it seems to me that it is
>> done similarly if you are using bash: e.g. you have to source the profile
>> yourself if you want to get it in a non-login shell.
>>
>>>
>>> 3. We can write better documentations explaining how to deal with fish
>>> shell in Guix. There's an old discussion about having a Guix wiki or
>>> a Guix book.
>>>
>>
>> Yeah, more documentation is always better :)
>
>
> --
> Meiyo Peng
> https://www.pengmeiyu.com/
>
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 36021@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 36021
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch