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
spidermonkey_68: fix build on armv7l #86532
Conversation
41c75f2
to
13286a1
Compare
// emulation here. | ||
|
||
-#if defined(__linux__) && defined(__arm__) | ||
+#if 0 |
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.
Could we not check for __ANDROID__
? It would make it clearer what’s going on, and mean that if somebody (in theory) built this for Android they’d get the right thing.
@@ -17,6 +17,13 @@ in stdenv.mkDerivation rec { | |||
outputs = [ "out" "dev" ]; | |||
setOutputFlags = false; # Configure script only understands --includedir | |||
|
|||
patches = optionals stdenv.isAarch32 [ |
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.
Applying patches conditionally is an anti pattern because they bit rot.
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.
While true, I feel like there's an argument to be made for avoiding dirtying derivations on platforms that do not need the patch, avoiding the usual staging churn, useless recompiles, etc. It's not like it's a used codepath on those platforms, so the patch could still bitrot even if it applies cleanly! I'd be surprised if this isn't pulled in as part of some obscure hydra job anyway?
The build for |
There's a PR for that too: #114942. The patch there is slightly different. We should choose one approach and apply it to both versions. |
Not that I care much but I'd be inclined to just go with the debian patch? either way yeah +1 for consistency |
I marked this as stale due to inactivity. → More info |
spidermonkey_68 is now removed: #153451 |
Motivation for this change
Build fix.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)