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

Ensure the path contains the Ozy bin directory when running anything through Ozy #55

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

yeetfield
Copy link
Member

@yeetfield yeetfield commented Feb 10, 2023

Tested by removing ~/.ozy/bin/ from my path and running a binary which activated the conda installer.

On main, this produces:

Running conda installer for [binary]
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', src/installers/installer.rs:28:38
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

And on this branch it works.

@yeetfield yeetfield requested a review from apmorton February 10, 2023 18:07
@yeetfield yeetfield self-assigned this Feb 10, 2023
@apmorton
Copy link
Contributor

This isn't exactly what I was thinking - but maybe its better?

I was thinking we add the path explicitly for subprocesses when we are installing thing using ozy - not when transparently passing through to actual apps.

@mattgodbolt thoughts on unconditionally prepending the ozy path like this?

@yeetfield
Copy link
Member Author

I think in the interest of keeping the critical path light, I'd prefer to do it the way you've recommended. It should be straightforward to move this into App::ensure_installed

@mattgodbolt
Copy link
Contributor

No strong opinions here - I can't think there'd be any downsides to unconditionally prepending though.

@yeetfield yeetfield merged commit 8c1320d into main Feb 10, 2023
@apmorton
Copy link
Contributor

This doesn't do it "only when installing" - this adds it to the current processes environment when it happens to be installing, which then means it's sometimes added to the real apps path - which is worse than either of the options.

We should either:

  • always add it
  • add it only to the child processes launched by run_subcommand_for_installer

If nobody can come up with a real world downside to prepending it always, let's just do that like you had originally.

@mattgodbolt mattgodbolt deleted the tl/ensure-path-is-set branch October 4, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants