-
Notifications
You must be signed in to change notification settings - Fork 68
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
fix: do not assume that /usr/bin/env exists on macOS #216
base: main
Are you sure you want to change the base?
Conversation
Force-pushed to fix issues reported by |
Not a fan of this. Why not just create or symlink to |
In general, only build directory is writable in sandbox, and a limited set of host directories is readable. While it should be possible to pass /usr/bin/env as an impure derivation dependency, as far as I know, this is usually used only for bootstrapping stdenv, and not in the packages themselves. Note that for Node.js, Alternatively, Nixpkgs can patch shebang lines in |
For example, when building in Nix sandbox, /usr/bin/env is not available. This change also makes gyp tool execution consistent with gyp-win-tool that uses sys.executable on Windows.
@cclauss, hi, are there any updates on this issue? I’d like to move forward with NixOS/nixpkgs#261820 and NixOS/nixpkgs#262124, and ideally I don’t want to apply this patch without getting it merged upstream (see also my previous comment). |
Why not set up |
Sandbox in the Nix term means that only packages built by Nix (which are in |
If a sandbox is going to provide a macOS-like environment, it should provide |
It depends on what you think a build environment should look like.
Both models are valid, tested in practice (165,000 PRs in Homebrew Core, 270,000 PRs in Nixpkgs), and can be used to build a multitude of things. |
@cclauss @legendecas, thanks for taking a look at this, can you please explain what concerns do you have with this change? I understand that we can disable/relax sandbox to a certain extent, but that is really the last resort unless there is some technical reason why this change is not sound. Gyp already uses E.g. ( gyp-next/pylib/gyp/generator/ninja.py Lines 2147 to 2157 in 4a8e328
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t this change be problematic if the python executable path contains some shell special char (e.g. ;
or some white space)?
Yes, but I don't think the existing code base handles that well too. Some of the formatting arguments should be using |
I hesitate because the assumption is correct for the vast majority of our Mac users and the others can use the |
IMO as long as NixOS community keeps sending patches and having support for their use case is not something that adds maintenance burden for us, there are no downsides for us. AFAICT this patch does not break the common use-case we actively support, it does not make the code less maintainable, so I think we should land it. |
This changes fixes build error when
/usr/bin/env
is not available (for example, when building in Nix sandbox). It makes gyp-generated build configurations invoke Python directly on the gyp tools (similar togyp-win-tool
that uses sys.executable on Windows).See also NixOS/nixpkgs#261820 and NixOS/nixpkgs#248708