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

[Feature Request] don't inject envman if ~/.local/bin is already in the path #322

Closed
adamcstephens opened this issue Nov 12, 2021 · 14 comments
Labels
enhancement New feature or request

Comments

@adamcstephens
Copy link

Hello. I have ~/.local/bin in my PATH already, but webinstall is injecting the envman inclusion anyway. It took me some digging to figure out this came from webinstall, as the message # Generated for envman. Do not edit. does not indicate the true source.

Would it be possible to check the existing PATH and not inject these lines?

@coolaj86
Copy link
Member

Because there's no standard way to define or check for PATH updates - especially across shells and operating systems - pathman/envman is really the only sane way to go.

HOWEVER, I think it would be very reasonable to always include a Files section in the README listing any files on the system that may be modified, and always include ~/.config/envman/PATH.env.

What do you think about that?

@coolaj86
Copy link
Member

P.S. What problem were you experiencing that made it important to not have ~/.local/bin in your PATH a second time? Generally this is benign.

@adamcstephens
Copy link
Author

webinstall isn’t just adding ~/.local/bin a second time, though I’d ask why bother modifying user’s config files to do so. webinstall is adding an entire new include location that could have arbitrary PATHs added to it. I manage my config files in git, so it was quite obvious to me that this was added. The problem is it’s an unnecessary addition, and I’d either have to manage this new file or blindly trust whatever PATHs are in it.

I’m excited about this project. I like the idea of a single installer script that can install multiple different tools, given that this is how projects are shipping now.

I’d be happy to prepare a PR if you’re ok with this request.

@coolaj86
Copy link
Member

Yeah, that would be great.

I may be better to put that logic into pathman as an option rather than webi, but you investigate and let me know what you think is the best route.

I expect to continue to support bash and fish on all OSes (including Windows), and powershell and cmd.exe on Windows (bonus if powershell works on *nix too, but not a goal).

And I definitely want to be more clear on the installers as to what files are modified, but that’s another topic perhaps.

@coolaj86
Copy link
Member

coolaj86 commented Nov 14, 2021

Many platform-programs, such as node and go - and even should-be-standalone apps, such as gpg - need to have multiple of their own PATH updates, such as ~/.local/opt/go/bin/ and ~/go/bin/.

The idea behind envman / pathman is to create a standard machine-readable location for cross-shell changes - kinda like git config x.y z

@ajeetdsouza
Copy link
Contributor

@coolaj86

I've outlined this in previous discussions as well, but (like a lot of users) I maintain my dotfiles using git, and I hate when tools change things around without asking me. To me, the best tool would be a tool that:

  1. Installs the binary to $LOCATION
  2. Checks that $LOCATION is present in $PATH
  3. If not, asks whether it can edit my config files to add it automatically (it should list the files that would be edited)
  4. The section added to my config file should mention that it was created by webi

I'm not very happy with the fact that that the webi/envman/pathman binaries are installed on my system when all I wanted was to install 1 application. If you want to promote webi and install it by default, that's quite understandable, but I feel like the rest can be done without installing more stuff.

@adamcstephens
Copy link
Author

I completely agree with @ajeetdsouza, and it was their app zoxide which brought me here.

@coolaj86
Copy link
Member

coolaj86 commented Nov 15, 2021

The problem of asking

Asking is a problem for two reasons:

  1. curl | bash steals the file descriptor of the tty and it's really hard to find a cross-platform, cross-shell way to get it back
  2. By default, webi should do the thing that Just Works™ without additional user interaction

That said, what's good for average Joe macOS users shouldn't have to annoy power users.

Possible options

That said, I'm very open to adding an option --no-path-updates and --no-webi.

The evil of pathman (a.k.a. envman)

pathman (which was intended to be renamed envman) has been a necessary evil.

However, I think it may be possible to do what it needs to done for webi just using bash at this point. That said, I want to stick with the philosophy of in producing machine-readable changes.

I think adding a by webi via pathman to the comment would be great.

In some future, webi should be able to uninstall go and remove ~/.local/opt/gobin/ from the PATH when go is uninstalled.

The evil of webi

It's not so much a matter of promoting webi as it is that it's not possible to determine all info that needs to be known by curl alone, so there's a bootstrap script. It turns out to be nice to be able to not have to type out the url when installing multiple things, so the script stayed.

@adamcstephens
Copy link
Author

I think adding a by webi via pathman to the comment would be great.

It looks like this will require modifying pathman, since that is where the existing comment is being generated.

https://git.rootprojects.org/root/pathman/src/commit/2c283a28649a692fea764894415cfa6b74fe2f5b/envpath/manager.go#L185
https://git.rootprojects.org/root/pathman/src/commit/2c283a28649a692fea764894415cfa6b74fe2f5b/envpath/manager.go#L216

Combining a more appropriate comment with outputting that this is happening would seem an appropriate first step for me. The latter could be done by not redirecting the pathman output to a file:

"$HOME/.local/bin/pathman" add "$1" | grep "export" 2> /dev/null >> "$_webi_tmp/.PATH.env" || true

I've looked through the code, but will not be submitting a PR after a further review. This issue is not an appropriate forum for a difference of vision, but if you're interested in a more detailed set of feedback let me know.

@coolaj86
Copy link
Member

I've looked through the code, but will not be submitting a PR after a further review.

Are you talking about the Go code, the Bash, or the Node that templates that Bash?

The Go code should be pretty straightforward to add a flag to update the comment. It would also be nice to change the output options.

The bash code has gotten grosser over time. Just lots of edge cases and... it's bash.

Now that the use cases have been explored, however, I think that there's a fair amount of code that can be eliminated and, ultimately, I think it could work to move toward mostly pre-generated installer scripts.

if you're interested in a more detailed set of feedback let me know

Let's do it: #329

@ajeetdsouza
Copy link
Contributor

ajeetdsouza commented Nov 15, 2021

curl | bash steals the file descriptor of the tty and it's really hard to find a cross-platform, cross-shell way to get it back

I see. A separate curl followed by bash would have worked (or bash -c), but that would not be as elegant or memorable, I suppose.

By default, webi should do the thing that Just Works™ without additional user interaction

While I understand that you'd prefer to do what works by default, I would have preferred a prompt for "Would you like to add these paths automatically to your .bashrc? [Y/n]", where the default answer is yes.

However, I think it may be possible to do what it needs to done for webi just using bash at this point. That said, I want to stick with the philosophy of in producing machine-readable changes.

I'm not sure how pathman would help here.

Suppose I have two programs (say X and Y) installed to ~/.local/bin. Now I uninstall X. Does pathman remember that Y is still installed there, so it should not remove the path?

Even if it does do that, suppose I have started relying on ~/.local/bin being in my path. A lot of utilities install themselves there, this is a pretty common path to use. Now I install Y as well, and webi removes the path. All of a sudden, I can't access any of the programs in my directory, causing confusion.

This is why I suggest that users should be allowed to maintain their PATHs in their own config files. We could automatically add a line to the user's config file, but after that the user should be allowed to manage the paths themselves. Moreover, this can/should be achieved inside bash / powershell itself, without the need for pathman. pathman does seem a bit overkill here.

That said, I'm very open to adding an option --no-path-updates and --no-webi.

This sounds great.

I think adding a by webi via pathman to the comment would be great.

Agreed.

@coolaj86 coolaj86 added the enhancement New feature or request label Nov 21, 2021
@ajeetdsouza
Copy link
Contributor

@coolaj86 here's another issue I got regarding this: ajeetdsouza/zoxide#318

This has also been mentioned in a discussion: #348

For automated installers, a major problem that users have is trust. While I appreciate the goal of never using sudo, I think that the obvious next step is to not automatically add extra binaries that the user didn't ask for, and to avoid modifying user configuration files. If I may suggest a course of action:

  • webi should not be included at all. If users use webi enough, they'll install it themselves.
  • pathman should be an opt-in thing, rather than something to explicitly disable with a command-line flag.

I'd appreciate it if you could tell me if this is a planned feature for Webi v2, because all 3 of these users have come from zoxide, which lists webi as the primary mode of install.

@adamcstephens
Copy link
Author

Personally I reverted to the old zoxide install script and patched it to work, allowing me to eliminate webi from my setup. As long as these issues exist, I have no interest in webi installer

@adamcstephens adamcstephens closed this as not planned Won't fix, can't repro, duplicate, stale Jan 16, 2023
@coolaj86
Copy link
Member

coolaj86 commented May 28, 2023

Fixed in #612, just for you @adamcstephens! 😘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants