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

Exported global vs universal variables and exec don't play well together #5258

Closed
mqudsi opened this issue Oct 14, 2018 · 15 comments · Fixed by #6218
Closed

Exported global vs universal variables and exec don't play well together #5258

mqudsi opened this issue Oct 14, 2018 · 15 comments · Fixed by #6218
Labels
Milestone

Comments

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Oct 14, 2018

Warning: This one is messy and I'm not 100% on what the correct behavior is.

When fish is executed exports are imported as global variables. As a result, exported global variables end up outliving their scope, which is defined as being the current session. When exec fish is called, the environment block is persisted (which is by design and a correct implementation of exec to the best of my knowledge), and the global variable carries into the new fish session.

Because of shadowing rules, the scope of a global variable also means global variables "win over" universal variables. Which is fine within a session, regardless of whether values are exported or not:

mqudsi@ZBook /m/c/U/m/r/fish-shell> set -gx FOO foo
mqudsi@ZBook /m/c/U/m/r/fish-shell> set -Ux FOO "universal foo"
mqudsi@ZBook /m/c/U/m/r/fish-shell> set -gx FOO "global foo"
mqudsi@ZBook /m/c/U/m/r/fish-shell> set -Ux FOO "universal foo"
set: Universal variable 'FOO' is shadowed by the global variable of the same name.
mqudsi@ZBook /m/c/U/m/r/fish-shell> echo $FOO
global foo
mqudsi@ZBook /m/c/U/m/r/fish-shell> env | grep FOO
FOO=global foo

If you start a new fish shell in a separate terminal right now and execute echo $FOO, the value will be universal foo.

But back in this same terminal/session, execute exec fish instead, then run echo $FOO and the result will be global foo and not universal foo.

This is fish's env design meets variable scopes, and it's not clear who the winner should be. Personally, I think exported variables are for interacting with the outside world and when it's fish-to-fish communication (within a school of fish, if I may), fish's architecture should win out. The scope of global variables is clearly defined as ending when the session is over, and exec fish definitely starts a new session (if the variable weren't exported, it wouldn't have been preserved), so the newly-instantiated universal variable should win out.

But at the same time, if the situation were the following (after having declared `set -Ux FOO "universal foo"):

Starting from sh:

$ export FOO="global foo"
$ exec fish
~> echo $FOO

I think it does make sense for global foo to win out since there's no other way of telling fish to start up with that variable assigned to that value, effectively overriding the universal binding with a (temporary) global one.

(Of course that means that there's no "automatic" way of overriding the default value of an environment variable set by the environment prior to invocation of fish without explicitly doing it in config.fish so that it's reset each time fish starts up.)

@faho faho added the RFC label Oct 15, 2018
@faho
Copy link
Member

@faho faho commented Oct 15, 2018

I'm not quite sure what you're proposing here?

when it's fish-to-fish communication (within a school of fish, if I may), fish's architecture should win out.

Do you want us to detect that we're about to run fish, and then remove the exported variables?

That just seems like it complicates the rules (quite a bit if you want it to also work for wrapper scripts and such) for little gain, because exec fish isn't that common a thing to do. Also, isn't exec fish in a certain sense a continuation of the current session?

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 16, 2018

Also, isn't exec fish in a certain sense a continuation of the current session?

I should hope not, as that would make our behavior wrong everywhere (no other session-specific values are saved). I actually use exec fish for the sole purpose of abandoning all changes and starting a new session, reloading my config.fish, dumping temporary functions, undoing changes to complete, etc. without having to close the terminal and start a new one, i.e. to start a new session.

Do you want us to detect that we're about to run fish, and then remove the exported variables?

I don't know. Perhaps there is a cleaner way? Can we detect that the parent process was fish and simply not import exported variables in conflict with existing universal variables?


Actually, taking a step back here. On startup, can't a universally exported variable just supercede any variables defined in the environment? After all, surely the point of universally setting a variable is to override its value, and doing so with the export flag means to override an exported such variable?

e.g. would it make sense for the following behavior:

mqudsi@Blitzkrieg /m/d/r/fish> set -gx foo "global foo"
mqudsi@Blitzkrieg /m/d/r/fish> set -Ux foo "universal foo"
set: Universal variable 'foo' is shadowed by the global variable of the same name.
mqudsi@Blitzkrieg /m/d/r/fish> exec fish
Welcome to fish, the friendly interactive shell
mqudsi@Blitzkrieg /m/d/r/fish> echo $foo
universal foo # exported universal variable won out

and

mqudsi@Blitzkrieg /m/d/r/fish> set -gx foo "global foo"
mqudsi@Blitzkrieg /m/d/r/fish> set -U foo "universal foo"
set: Universal variable 'foo' is shadowed by the global variable of the same name.
mqudsi@Blitzkrieg /m/d/r/fish> exec fish
Welcome to fish, the friendly interactive shell
mqudsi@Blitzkrieg /m/d/r/fish> echo $foo
global foo # exported variable won out over un-exported universal variable

(regardless of whether the export was defined in fish or in a different shell/environment before fish was execd/spawned)

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 17, 2018

I missed the obvious here: this all applies to regular recursive invocations of fish and not just the much rarer exec fish cases.

@faho
Copy link
Member

@faho faho commented Oct 17, 2018

this all applies to regular recursive invocations of fish and not just the much rarer exec fish cases.

Sure, but those are literally child processes of the current session, so the logic that they should inherit exported variables applies even more.

I don't know. Perhaps there is a cleaner way? Can we detect that the parent process was fish and simply not import exported variables in conflict with existing universal variables?

And that's cleaner? Even if it were possible (which I don't think it is), that's even worse than special-casing a child fish.

And I believe you're vastly underestimating the effort required here. Think of wrapper-scripts and different fish binaries and symlinks and.... All those cases would have to not behave terribly.

surely the point of universally setting a variable is to override its value

Universal variables are very weak, all other scopes beat them, so they can't really be used to override anything.

e.g. would it make sense for the following behavior:

The problem there is that in the "parent" the universal variable loses, but then suddenly in the child it wins?


What I would like here is to get away from the abstract examples, from the "foo"s and "bar"s and get to the actual use. Maybe then we can figure out how to do it nicer.

What did you do, and how did it in your opinion go wrong?

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 18, 2018

$EDITOR was being set by my environment to some value (well, an empty ``) before fish was started. I wanted to globally set its value on this machine without poisoning my git-shared config.fish, so I did what I always do: I set the value as a universal variable.

But fish imports the existing EDITOR value as a global variable which superseded my universal declaration.

I still think the user explicitly universally setting FOO (or EDITOR, if you will) should not be overruled by fish on import of environment variables.

A user-defined global variable should mask a user-defined universal variable, but fish shouldn't silently mask a universal variable the user went out of their way to define.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Oct 18, 2018

How do we know if a global variable is "user-defined" or not?

@faho
Copy link
Member

@faho faho commented Oct 18, 2018

Okay, this actually has an FAQ entry (http://fishshell.com/docs/current/faq.html): "Why doesn't set -Ux (exported universal variables) seem to work?".

It's indeed a common problem, and I'm open to suggestions for improvement.

I don't think we can know if a global variable is "user-defined", and I don't think we should special-case a child or parent fish (in fact I don't even think that would help much - @mqudsi: In your example the broken $EDITOR could have also come from the terminal or tmux).

The best I can come up with is to let universal variables take precedence over "imported" variables. I.e. at startup, just don't import variables from the environment for which a universal exists.

This is almost @mqudsi's idea: "On startup, can't a universally exported variable just supercede any variables defined in the environment?", with the key difference that the export-bit doesn't matter.

There are two problems with this:

  • What happens if you want to override a variable that exists as a universal? E.g. env XDG_CONFIG_HOME=something fish.

  • What happens if you delete the universal variable? Do you then suddenly get a global variable defined (unclean) or is it then undefined (loses information)?

I think that might actually be worth it. I consider both of those problems to be fairly small and unimportant, so that trading them for what we have now would be a net improvement.

The former is only really a problem for XDG_CONFIG_HOME, because you can change other variables once you are in the fish session (i.e. from the commandline or in the script), and it's only a problem if someone decides to define that as a universal. Presumably it could be fixed by adding an option to pick the config directory.

The latter is something that I believe happens very infrequently.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 24, 2018

I would be fine with that. @ridiculousfish?

@neersighted
Copy link
Contributor

@neersighted neersighted commented Oct 25, 2018

I currently make use of this snippet to solve what is essentially the same issue. I find performance acceptable, but making it a built in (optional? somehow?) behavior would be appreciated.

# ignore parent exports in favor of universal exports
for export in (set -Ux)
  set -eg (string split ' ' $export)[1]
end

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Oct 27, 2018

Environment variables are conceptually high-priority and it feels wrong to me to override them. They're commonly used for scoped configuration changes, including the XDG_CONFIG_HOME example.

Note that with the current behavior, it's possible though awkward to manually allow the uvar to take precedence, as neersighted illustrates. But if uvars always took precedence, incoming environment variables would be unrecoverable. So I think the default should remain as it is.

One possibility is to make this an option to set, for example set -U --force EDITOR vim which would allow this to be set per variable. That seems mostly harmless to me since it's just shorthand for something the user could do anyways.

But before we go down this path, how does this actually happen? Who was setting $EDITOR - is login doing that, is it because fish was being invoked from bash...? Are there other variables that are affected by this?

@faho
Copy link
Member

@faho faho commented Oct 28, 2018

But before we go down this path, how does this actually happen?

One possibility is stacking fish, and the path-var changes make that slightly worse.

If I now do set -Ux EDITOR emacs -nw (the export-bit is necessary for e.g. git to pick it up), then it'll be exported as "emacs -nw" (which, again, is what it should be for external commands to grok it).

So a child fish will import it as "emacs -nw". So if you then use $EDITOR as a command it'll break.

@neersighted
Copy link
Contributor

@neersighted neersighted commented Oct 29, 2018

I use nested fish quite heavily (fish inside tmux inside fish), which is why I use this snippet. It has lots of sharp edges, but I find careful use of universal exported variables fits my use case. I love the idea of a --force for set; if I were to implement this in fish, this is how I would do it.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Nov 17, 2018

@ridiculousfish I'm honestly none too sure at this point what was the original cause of that, but I've pinpointed what actually bothers me about this the most, and I feel this is the case that best demonstrates the crux of the problem:

~> set -Ux foo foo
~> exec fish
~> set -Ux foo bar
~> echo $foo
foo

In this case, at no point was a global variable ever (manually) defined. fish shadowed my variable declaration itself / for itself, and then gave that priority over internal fish variables.

Note that with the current behavior, it's possible though awkward to manually allow the uvar to take precedence, as neersighted illustrates. But if uvars always took precedence, incoming environment variables would be unrecoverable. So I think the default should remain as it is.

Not necessarily. I think I am leaning heavily towards a solution adding a read-only scope imported or similar that has an even lower priority than universal. It would also make it clearer where these variables came from.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Nov 17, 2018

So a child fish will import it as "emacs -nw". So if you then use $EDITOR as a command it'll break.

@faho you mention this only in passing but this is actually a very important point. As it currently stands, this is a regression in fish 3.0 that we should fix before shipping, since we use $EDITOR as a command in fish itself.

@tcorbettclark
Copy link

@tcorbettclark tcorbettclark commented Jul 22, 2019

I currently make use of this snippet to solve what is essentially the same issue. I find performance acceptable, but making it a built in (optional? somehow?) behavior would be appreciated.

# ignore parent exports in favor of universal exports
for export in (set -Ux)
  set -eg (string split ' ' $export)[1]
end

I have a particular use-case of wishing to preserve the universal property of SSH_AUTH_SOCK variable so that the ssh-agent continues to work correctly everywhere, including across tmux and Python virtualenvs. However I also want it updated when logging in again and it changes, at which point the global SSH_AUTH_SOCK is correct and the universal SSH_AUTH_SOCK is now stale.

The above doesn't work with the second of these two cases. In fact it looses the new values.

I solve this by running an "uplift" function on the key SSH variables on every fish invocation.

function uplift-variable-to-universal
    for x in $argv
        if set -q $x
            set -l temp $$x
            set -el $x
            set -eg $x
            set -eU $x
            set -Ux $x $temp
        end
    end
end

Then in my fish config.fish I have:

# Certain SSH variables are (correctly) exported to subprocesses e.g. to forward
# the ssh agent. But when that subprocess is a fish shell we want them to remain Universal
# so that when they change they change everywhere (including tmux and Python virtualenvs).
# A new ssh login may validly change them, and when this happens the fish shell will set the
# new values in Global scope.
# Both of these use-cases are handled by always running following on every new fish shell.
# It "uplifts" the nearest scope values into Universal scope only.
uplift-variable-to-universal SSH_AUTH_SOCK SSH_CLIENT SSH_CONNECTION

No doubt the above can be written more cleanly or generically - I'm no fish expert.

This fetches the nearest scope value, removes all scope versions, and then sets it to universal. The result is that either that we have copied the global to the universal (which was already the same), or the global was the new (in which case we have just updated it).

I appreciate this is not dealing with the generic subject matter of this topic, but it does provide a specific use-case to think about and a possible approach.

krobelus added a commit to krobelus/fish-shell that referenced this issue Oct 17, 2019
Universal exported variables (created by `set -xU`) used to show up
both as universal and global variable in child instances of fish.

As a result, when changing an exported universal variable, the
new value would only be visible after a new login (or deleting the
variable from global scope in each fish instance).

Additionally, something like `set -xU EDITOR vim -g` would be imported
into the global scope as a single word resulting in failures to
execute $EDITOR in fish.

We cannot simply give precedence to universal variables, because
another process might have exported the same variable.  Instead, we
only skip importing a variable when it is equivalent to an exported
universal variable with the same name.  We compare their values after
joining with spaces, hence skipping those imports does not change the
environment fish passes to its children. Only the representation in
fish is changed from `"vim -g"` to `vim -g`.

Closes fish-shell#5258.
This eliminates the issue fish-shell#5348 for universal variables.
krobelus added a commit to krobelus/fish-shell that referenced this issue Oct 18, 2019
Universal exported variables (created by `set -xU`) used to show up
both as universal and global variable in child instances of fish.

As a result, when changing an exported universal variable, the
new value would only be visible after a new login (or deleting the
variable from global scope in each fish instance).

Additionally, something like `set -xU EDITOR vim -g` would be imported
into the global scope as a single word resulting in failures to
execute $EDITOR in fish.

We cannot simply give precedence to universal variables, because
another process might have exported the same variable.  Instead, we
only skip importing a variable when it is equivalent to an exported
universal variable with the same name.  We compare their values after
joining with spaces, hence skipping those imports does not change the
environment fish passes to its children. Only the representation in
fish is changed from `"vim -g"` to `vim -g`.

Closes fish-shell#5258.
This eliminates the issue fish-shell#5348 for universal variables.
krobelus added a commit to krobelus/fish-shell that referenced this issue Oct 18, 2019
Universal exported variables (created by `set -xU`) used to show up
both as universal and global variable in child instances of fish.

As a result, when changing an exported universal variable, the
new value would only be visible after a new login (or deleting the
variable from global scope in each fish instance).

Additionally, something like `set -xU EDITOR vim -g` would be imported
into the global scope as a single word resulting in failures to
execute $EDITOR in fish.

We cannot simply give precedence to universal variables, because
another process might have exported the same variable.  Instead, we
only skip importing a variable when it is equivalent to an exported
universal variable with the same name.  We compare their values after
joining with spaces, hence skipping those imports does not change the
environment fish passes to its children. Only the representation in
fish is changed from `"vim -g"` to `vim -g`.

Closes fish-shell#5258.
This eliminates the issue fish-shell#5348 for universal variables.
krobelus added a commit that referenced this issue Oct 19, 2019
Universal exported variables (created by `set -xU`) used to show up
both as universal and global variable in child instances of fish.

As a result, when changing an exported universal variable, the
new value would only be visible after a new login (or deleting the
variable from global scope in each fish instance).

Additionally, something like `set -xU EDITOR vim -g` would be imported
into the global scope as a single word resulting in failures to
execute $EDITOR in fish.

We cannot simply give precedence to universal variables, because
another process might have exported the same variable.  Instead, we
only skip importing a variable when it is equivalent to an exported
universal variable with the same name.  We compare their values after
joining with spaces, hence skipping those imports does not change the
environment fish passes to its children. Only the representation in
fish is changed from `"vim -g"` to `vim -g`.

Closes #5258.
This eliminates the issue #5348 for universal variables.
@zanchey zanchey added this to the fish 3.1.0 milestone Oct 19, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 17, 2021
Prior to this change, there was a separate "universal" scope above the global
scope. It was possible to have a universal variable which was shadowed by a
global of the same name.

This changes "universal" to be a *variable property*, like "exports." Only
globals may have this property set. "Universal variable" now means "global
variable that is also marked as universal." `set --universal` now implies the
`--global` option.  When you create, modify, or erase a universal global, all
other sessions change their corresponding global as well.

The most important implication is that *universal variables now take precedence
over inherited environment variables*. This addresses fish-shell#5258 and fish-shell#5348. In
principle this allows variables like PATH to be universal; however that's not
recommended (yet).

For compatibility, we retain `set --query --universal` which now means "is
there a global variable with the universal property."

One important implication is that `set -qg` will now succeed for universal
variables:

    > set --universal  myvar someval
    > set --query --global myvar # succeeds

Functions which expected this to fail (`abbr` is an example) need to be
updated.

Fixes fish-shell#5348. Fixes fish-shell#5258. Fixes fish-shell#7379. Fixes fish-shell#7317.
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 18, 2021
Prior to this change, there was a separate "universal" scope above the global
scope. It was possible to have a universal variable which was shadowed by a
global of the same name.

This changes "universal" to be a *variable property*, like "exports." Only
globals may have this property set. "Universal variable" now means "global
variable that is also marked as universal." `set --universal` now implies the
`--global` option.  When you create, modify, or erase a universal global, all
other sessions change their corresponding global as well.

The most important implication is that *universal variables now take precedence
over inherited environment variables*. This addresses fish-shell#5258 and fish-shell#5348. In
principle this allows variables like PATH to be universal; however that's not
recommended (yet).

For compatibility, we retain `set --query --universal` which now means "is
there a global variable with the universal property."

One important implication is that `set -qg` will now succeed for universal
variables:

    > set --universal  myvar someval
    > set --query --global myvar # succeeds

Functions which expected this to fail (`abbr` is an example) need to be
updated.

Fixes fish-shell#5348. Fixes fish-shell#5258. Fixes fish-shell#7379. Fixes fish-shell#7317.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants