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

Add fish_add_path, a simple way to add to $PATH #7028

Closed
wants to merge 15 commits into from

Conversation

faho
Copy link
Member

@faho faho commented May 21, 2020

Description

fish_add_path [paths...]
fish_add_path (-h | --help)
fish_add_path [(-g | --global) | (-U | --universal) | (-P | --path)] 
    [(-m | --move)] [(-a | --append) | (-p | --prepend)] 
    [(-v | --verbose) | (-n | --dry-run)] [paths...]

This is a function you can either execute once, interactively, or
stick in config.fish, and it will do the right thing.

Some options are included to choose some slightly different behavior,
like setting $PATH directly instead of $fish_user_paths, or moving
already existing components to the front/back instead of ignoring
them, or appending new components instead of prepending them.

The defaults were chosen because they are the most safe, and
especially because they allow it to be idempotent - running it again
and again and again won't change anything, it won't even run the
actual set because it skips that if all components are already in.

Fixes #6960.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

This is a function you can either execute once, interactively, or
stick in config.fish, and it will do the right thing.

Some options are included to choose some slightly different behavior,
like setting $PATH directly instead of $fish_user_paths, or moving
already existing components to the front/back instead of ignoring
them, or appending new components instead of prepending them.

The defaults were chosen because they are the most safe, and
especially because they allow it to be idempotent - running it again
and again and again won't change anything, it won't even run the
actual `set` because it skips that if all components are already in.

Fixes fish-shell#6960.
@faho faho added this to the fish 3.2.0 milestone May 21, 2020
Copy link
Member

@zanchey zanchey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. The --help flag won't work until the documentation is written, of course, and I think it might be sensible to update the tutorial as well.

@faho
Copy link
Member Author

faho commented May 21, 2020

Caveat: We do not realpath the existing $PATH, so we can add a component that's already in by another name. I don't think that's much of an issue - it's more important that a path is searched or not, not with which name it is. It's also mostly for idempotency (to avoid adding a component again and again), so worst case we add a path once unnecessarily - which doesn't hurt much.

TODO: We need to absolutize non-existent paths still. realpath needs to be changed to do this.

EDIT: This is done, in a slightly cheesy way - we just prepend $PWD for relative paths iff realpath fails (which is when more than the last component doesn't exist). That should always work, and should only look weird if the path contains ".." components. But tbh don't do that.

faho added 8 commits May 21, 2020 17:20
realpath needs all but the last component to exist, so if we do

    fish_add_path non/existent/path

it fails and we fall back to the path we have been given.

Since relative $PATH components are a terrible idea, we at least make
it absolute by prepending $PWD.

This will lead to weird results if you add

    fish_add_path ../non/existent

($PWD/../non/existent)

but at that point it's GIGO.
And link fish_add_path to it.

Weird that we didn't already have one.
Nothing too much, and these are tricksy, precious.
Turns out in macOS on Travis that's really /private/etc
@faho
Copy link
Member Author

faho commented May 21, 2020

Okay, so things up for discussion:

  • The name (maybe just add_path? or addpath?)
  • The semantics, in particular
  • Warning when a path doesn't exist
  • Opening it up to other variables - it could also e.g. add to LD_LIBRARY_PATH, but the realpath stuff is path-specific so it wouldn't work for adding deduplicated entries to arbitrary lists unless there was a way to disable that, and at that point it feels awkward to use (fish_add_path --variable=mylist --no-realpath foo bar baz just looks weird)

We hadn't actually checked that $fish_user_paths works this way.

It should, but as it turns out this is actually the first we're
testing it.
@zanchey
Copy link
Member

zanchey commented May 22, 2020

I agree with #6960 (comment) that fish_add_path is the way to go.

I don't think this is needed for other path variables.

@faho faho mentioned this pull request May 24, 2020
@faho
Copy link
Member Author

faho commented May 25, 2020

Okay, I would really like to remove the warning, as we've heard from another person who has trouble ssh-ing to a machine because of the $fish_user_paths warning in fish 2.7.

Adding a warning that isn't often triggered to something where you only see it after logging back in is awful, especially in conjunction with ssh where that can block login.

@krobelus
Copy link
Member

Okay, I would really like to remove the warning

Seems good, the warning can be a false positive. I guess if we wanna be fancy we can warn only when status current-filename is Standard input.

Printing an error, unexpectedly, from config.fish can result in
problems, especially since e.g. scp starts a shell and doesn't expect
any other output.
@faho
Copy link
Member Author

faho commented May 29, 2020

Okay, merged as 9354dd6..1e17a68

@faho faho closed this May 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2020
@faho faho deleted the add-path branch September 23, 2021 09:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple way to add to the path
3 participants