[Bug]: export of fish shell environment variables do not always works as intended

[arthurgeek]

Current Behavior (bug)

My fish config contains:

set EXA_STANDARD_OPTIONS --group --header --group-directories-first --icons

but exportify: https://github.com/jetpack-io/devbox/blob/2fee55b15345134123acec9da617b48e0547f715/internal/devbox/envvars.go#L17-L22 exports every env variable enclosed with "". This results in the incorrect behaviour: a variable exported with " will be treated as a single argument, while a variable without " will correctly treat the values as separate arguments. In fish, all variables are lists, so it's not always correct to enclose variables with strings.

The $EXA_STANDARD_OPTIONS var is sent to exa and when exported with ", exa/eza treats the whole value as a single argument, returning an error: eza: Unknown argument --group --header --group-directories-first --icons. If exported without ", it works as expected, because fish treats those as an argument list.

Expected Behavior (fix)

Maintain semantics of user-exported env variables. Or, even better, when using fish, do not re-export env variables that are not relevant to devbox.

If this comment works as intended, there's no need to reexport something that was correctly exported in my fish config:

https://github.com/jetpack-io/devbox/blob/2fee55b15345134123acec9da617b48e0547f715/internal/devbox/shellrc_fish.tmpl#L6-L9

Additional context

$ devbox version -v
Version:     0.8.7
Platform:    darwin_arm64
Commit:      525bb17ed665bb3066fbd3fb9bccec0aa961ecb7
Commit Time: 2024-01-23T19:15:31Z
Go Version:  go1.21.5
Launcher:    0.2.1

[savil]

@arthurgeek thanks for filing this issue. I tried to repro it but was having some diffculty. Could you point out what I should do differently?

Here's what I did:

# set EXA-STANDARD_OPTIONS in config.fish:
> cat ~/.config/fish/config.fish
...
set EXA_STANDARD_OPTIONS --group --header --group-directories-first --icons

# add eza to devbox global
> devbox global add eza

# verify the standard options are set
(devbox) > echo $EXA_STANDARD_OPTIONS
--group --header --group-directories-first --icons

# run eza, which works fine by listing the files in my directory
> eza -al

[savil]

(related: I did run into a few fish-related bugs: #1755 and #1756)

[arthurgeek]

@savil thanks for looking into it!

sorry, I should have been more explicit: $EXA_STANDARD_OPTIONS is not directly used by eza. You have to call the binary with it. I don't have access to a machine installed with devbox right now, but this should give you the exact same behavior:

$ set EXA_STANDARD_OPTIONS --group --header --group-directories-first --icons
$ echo $EXA_STANDARD_OPTIONS
--group --header --group-directories-first --icons
$ eza $EXA_STANDARD_OPTIONS
[works, listing files/directories]
$ export EXA_STANDARD_OPTIONS="--group --header --group-directories-first --icons"
$ echo $EXA_STANDARD_OPTIONS
--group --header --group-directories-first --icons
$ eza $EXA_STANDARD_OPTIONS
eza: Unknown argument --group --header --group-directories-first --icons
$

Note: I'm using export in my example because that's the command devbox runs.

[savil]

Thanks! I can now reproduce it. Looked into it a bit today.

I tried modifying our implementation to not wrap the values in quotations, but that breaks devbox shell because some env-vars produced by nix print-dev-env have values that really do need the double-quote wrapping.

Will look into alternate fixes...

[arthurgeek]

I tried modifying our implementation to not wrap the values in quotations

I don't think this is a good approach. devbox shouldn't make any assumption whether a variable is a list with multiple items (without quotations) or not. Re-exporting everything without quotations could also break where a user variable is expected to be a single-item list.

I'm not familiar with devbox's code or nix, but I think the best alternative is to not copy any variable from the current env that's not relevant to devbox. If a variable it's not used by devbox, then don't copy/re-export it. This way you have full control: you know if a variable should be a list or not.

From what I looked at the code, it copies everything because for other shells when opening devbox shell you need to ensure all the vars from current env are present, to keep a consistent behavior with current env. But since for fish you actually load the user config when running devbox shell, you don't need to re-export everything, you can simply export only the variables that are relevant to devbox.

I think you could modify this behavior when in fish: https://github.com/jetpack-io/devbox/blob/5585d21142957883d1bc570a1c1193eeb83f2766/internal/devbox/devbox.go#L769-L795 perhaps even here: https://github.com/jetpack-io/devbox/blob/5585d21142957883d1bc570a1c1193eeb83f2766/internal/devbox/devbox.go#L1220-L1222

If that's not feasible, then another suggestion would be to use fish's count:

$ set EXA_STANDARD_OPTIONS --group --header --group-directories-first --icons
$ count $EXA_STANDARD_OPTIONS
4
$ set EXA_STANDARD_OPTIONS "--group --header --group-directories-first --icons"
$ count $EXA_STANDARD_OPTIONS
1
$

and re-export with/without quotations depending on the count value.

I don't see any other way to keep variable "semantics". In both cases you're adding code that's specific only to fish, but the first option might lead to less problems down the road, as you're not trying to touch anything that's outside the scope of devbox.

[arthurgeek]

@savil I'm still running into this issue and might have some time to look into fixing it. which option would you prefer I look into? and would you have some pointers to where/how this could be implemented? I would need to know which variables are devbox-specifics and from the ones that aren't, which ones need to be reexported with new data (such as $PATH, it seems like)