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
Low-impact cross fixes, batch 2 #33480
Conversation
Thank you for doing this! |
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.
Just need to check with @bgamari on that one thing bit then we are good.
@@ -17,7 +17,7 @@ let | |||
hardeningEnable = [ "pie" ]; | |||
installPhase = '' | |||
mkdir -p $out/bin | |||
gcc -Wall -O2 -DWRAPPER_DIR=\"${parentWrapperDir}\" \ | |||
${pkgs.stdenv.cc.targetPrefix}cc -Wall -O2 -DWRAPPER_DIR=\"${parentWrapperDir}\" \ |
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.
Can use $CC
I'd assume? @bgamari
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.
Ahh, fair point.
@@ -15,7 +15,7 @@ stdenv.mkDerivation rec { | |||
|
|||
configureFlags = stdenv.lib.optional stdenv.isArm "--disable-arm-iwmmxt"; | |||
|
|||
doCheck = true; | |||
doCheck = stdenv.hostPlatform == stdenv.buildPlatform; |
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.
Again stdenv should be changed to cope with this.
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.
Should it? It's not clear to me that stdenv
should dictate this.
pkgs/tools/security/sudo/default.nix
Outdated
@@ -28,6 +28,7 @@ stdenv.mkDerivation rec { | |||
"--with-logpath=/var/log/sudo.log" | |||
"--with-iologdir=/var/log/sudo-io" | |||
"--with-sendmail=${sendmailPath}" | |||
"--enable-tmpfiles.d=no" |
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.
What's the reason for this?
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.
Indeed it could use a comment. I'll have to revert it to recall why this was necessary.
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.
Dropping-- presumably this is forcing the expect value so as to avoid performing a cross-unfriendly check. Perl is on the path to sudo and so perhaps this should be revisited once perl works.
{ | ||
outputs = [ "out" "devdoc" ]; | ||
|
||
doCheck = true; |
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.
Again for stdenv to handle.
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.
Few more catches.
configureFlags = | ||
[ "--with-ssl=${openssl.dev}" "--with-gc=${boehmgc.dev}" ] | ||
++ optionals (stdenv.buildPlatform != stdenv.hostPlatform) [ | ||
"ac_cv_func_setpgrp_void=yes" |
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 we're checking things. I found I needed a lot less of these configure test overrides than @bgamari, so maybe pull this out initially?
|
||
|
||
configureFlags = | ||
stdenv.lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [ "ac_cv_func_malloc_0_nonnull=yes" ]; |
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.
Same might not be needed
@@ -5,6 +5,7 @@ stdenv.mkDerivation { | |||
|
|||
src = kernel.src; | |||
|
|||
nativeBuildInputs = [ gettext ]; | |||
buildInputs = [ coreutils pciutils gettext ]; |
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.
Gettext need not be build input, I'd think?
@dtzWill do you have hydra access? It would make testing whether some of those changes are needed far easier |
37e5757
to
93aa130
Compare
93aa130
to
f866587
Compare
@Ericson2314 nope, no Hydra access. Testing is difficult--many of these packages can't be built even after these changes since they depend on packages that required mass-rebuilds to fix. Hmm. Do you know if you two were testing on the same set of cross configurations? |
@dtzWill mmm. then let's just land without any configure test overrides, and assume will need to more later. The idea is these PRs will land the changes we're sure we need, and then later we will land anything else we end up needing. Sorry landing these things is so complicated! |
No worries, thanks for working with me on this. And the reviewers 👍. I'll think how to best proceed, probably nuking both of these PR's and pulling out things that are less "controversial". I think the current guidelines are something like:
2a) On the build-frontier for cross-compilation as currently shown by Hydra ... sound good? |
For example, the |
@dtzWill Well if it's on the build frontier (I think by which you mean we can build all deps just not it, good term :)) then it's fine if it's a bit weirder, as we can actually confirm the weirdness is needed. |
Motivation for this change
Next batch in spirit of #33474 (picked from #30882).
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)