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

Use $PREFIX to resolve /usr/bin/env #15896

Closed
wants to merge 2 commits into from

Conversation

JerwuQu
Copy link
Contributor

@JerwuQu JerwuQu commented May 29, 2023

This solves #14146.

Alternative behavior (i.e. not present in this PR):

  • Always prefer $PREFIX over /usr/bin/env when set.
  • Allow the user to specify a file to check, rather than using $PREFIX and $PREFIX/bin/env.
  • Search for env in $PATH. NOTE: I personally think this would be the most reliable option, but it also requires a lot more code and file system shenanigans. If going down this route, we'd probably want to cache that file (or better, the CrossTarget) for the lifetime of the zig process, since currently this function is called multiple times for a build.
  • Some other env var?

The suggested behavior was chosen because it's the least intrusive, only being applied when /usr/bin/env is not found or fails, and because it works straight away in Termux.

Works for me, but additional testing would be appreciated.

@mitchellh
Copy link
Contributor

I know this isn’t the goal of this PR, but would be nice if we somehow fixed this patch Nix requires too: https://github.com/NixOS/nixpkgs/blame/e6e389917a8c778be636e67a67ec958f511cc55d/pkgs/development/compilers/zig/0.10.nix#L48-L51

I dont’ think it does, and that’s okay (because that’s not the goal of this PR or the linked issue), but they’re so close!

@JerwuQu
Copy link
Contributor Author

JerwuQu commented May 29, 2023

Ah! I haven't used nix much myself, but looking at that patch, you'd probably be able to export PREFIX=${coreutils} and have this PR work for that too. Is this reasonable, or what would The Nix Way of doing it?

@BratishkaErik
Copy link
Contributor

BratishkaErik commented May 29, 2023

  • Allow the user to specify a file to check, rather than using $PREFIX and $PREFIX/bin/env.

I'd vote for this approach, since PREFIX is (iirc) used widely by build systems/sandbox/etc. and it's meaning may differ drastically when switching between them. Something like ZIG_ELF_FILE_PATH or ZIG_ENV_BINARY_PATH (imho) will be more suitable + it can still have these attributes:

it's the least intrusive, only being applied when /usr/bin/env is not found or fails,

and because it works straight away in Termux.

IDK how it is customary to do in Termux repos, but I'd suggest exporting this variable in appropriate system-wide files (preferably) or simply making /usr/bin/zig a script with similar content:

#!/bin/sh
ZIG_ELF_FILE_PATH=somewhere /path/to/zig "$@"

@venomega
Copy link

what if you just get the output of which env ?

@BratishkaErik
Copy link
Contributor

  • Allow the user to specify a file to check, rather than using $PREFIX and $PREFIX/bin/env.

I'd vote for this approach, since PREFIX is (iirc) used widely by build systems/sandbox/etc. and it's meaning may differ drastically when switching between them. Something like ZIG_ELF_FILE_PATH or ZIG_ENV_BINARY_PATH (imho) will be more suitable + it can still have these attributes:

it's the least intrusive, only being applied when /usr/bin/env is not found or fails,

and because it works straight away in Termux.

IDK how it is customary to do in Termux repos, but I'd suggest exporting this variable in appropriate system-wide files (preferably) or simply making /usr/bin/zig a script with similar content:

#!/bin/sh
ZIG_ELF_FILE_PATH=somewhere /path/to/zig "$@"

Nevermind, Andrew does not want new env-variable, so your approach with already existing env-var is now more likely to be merged.

@andrewrk
Copy link
Member

I know this isn’t the goal of this PR, but would be nice if we somehow fixed this patch Nix requires too: https://github.com/NixOS/nixpkgs/blame/e6e389917a8c778be636e67a67ec958f511cc55d/pkgs/development/compilers/zig/0.10.nix#L48-L51

@mitchellh I'm confused why NixOS requires this patch. I've been using NixOS for many years and /usr/bin/env has always been there for me, like a good friend when I need to move furniture.

Furthermore, NixOS does not populate PREFIX. Are you suggesting to look for it in PATH?

@andrewrk
Copy link
Member

This PR will need to get redone because it now has conflicts and needs to have some additional CI checks run. Additionally, I have clarified how the implementation should work here: #14146 (comment)

I will now close this, however, the tracking issue is still open and anyone is free to pick it up and keep working on it in a new PR.

@andrewrk andrewrk closed this Jan 24, 2024
@erikarvstedt
Copy link
Contributor

I'm confused why NixOS requires this patch. I've been using NixOS for many years and /usr/bin/env has always been there for me

@andrewrk, /usr/bin/env is present on NixOS, but it's missing in Nix build environments (the sandboxes where pkgs are built).

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.

6 participants