-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Restore cross-patch-shebangs branch #49608
Restore cross-patch-shebangs branch #49608
Conversation
cc @LnL7 |
11edcdd
to
21a236d
Compare
@GrahamcOfBorg build stdenv |
Success on aarch64-linux (full log) Attempted: stdenv Partial log (click to expand)
|
Timed out, unknown build status on x86_64-linux (full log) Attempted: stdenv Partial log (click to expand)
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: stdenv Partial log (click to expand)
|
@@ -278,8 +278,8 @@ in rec { | |||
# enables patchShebangs above. Unfortunately, patchShebangs ignores our $SHELL setting | |||
# and instead goes by $PATH, which happens to contain bootstrapTools. So it goes and | |||
# patches our shebangs back to point at bootstrapTools. This makes sure bash comes first. |
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.
Edit this comment to match the new reality, too? I also feel like if we can use your disallowRequisites
patch with this, we can skip pkgs.bash
in either list, which would be best. I'd be very down to collaborate getting all the stdenv deps to use strictDeps
:).
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.
Yeah that sounds good! It might have to go in another PR for now - I'm more worried about fixing cross compilation right now.
One thing these are all going to need though is bash in the buildInputs. I think there's quite a few of those that don't have it right now.
I think we could also do an override here that does something like:
stdenv = super.stdenv // { mkDerivation = args: super.stdenv.mkDerivation (args // { strictDeps = true; }); };
To avoid individually setting all of these.
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.
Well I do want to audit those packages anyways. I think I've decided this PR and the disallowRequites
one are good as is, and then all the per-package cleanup can happen in a 3rd PR.
This reverts commit 9c4b11e.
In strictDeps=false, autoPatchshebangs should use --build (corresponding to PATH) to lookup commands. This restores the previous behavior of patchshebangs so that we don’t break stuff that isn’t careful in the buildInputs vs. nativeBuildInputs distinction. Unfortunately this won’t work under cross compilation.
patch shebangs needs to be in build inputs for it to get into HOST_PATH.
21a236d
to
068db20
Compare
068db20
to
2f2e635
Compare
Motivation for this change
This was merged, then reverted. This should address the issues that originally came up.