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
boost: Clean, reducing duplication #33188
Conversation
28fe756
to
49abae8
Compare
I differed the native derivations of the default version, and nothing interesting is different: just argument order and extra |
Looking to finally merge #26805, so Boost maintainer, whoever you are, I hope you aren't on an internet-free vacation! (This blocks that.) |
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.
I've not reviewed this too thoroughly but looks fine generally, you can even probably drop the PIC stuff, see inline comments.
|
||
withToolset = stdenv.lib.optionalString (toolset != null) "--with-toolset=${toolset}"; | ||
cxxflags = optionalString (enablePIC) "-fPIC"; |
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.
You should be able to drop all PIC handling, "fixed" by hardening ages ago
|
||
genericB2Flags = [ | ||
linkflags = optionalString (enablePIC) "-fPIC"; |
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.
dito
else | ||
""; | ||
cflags = concatStringsSep " " | ||
(optional (enablePIC) "-fPIC" ++ |
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.
dito
@globin Ah, you mentioned that before, and I guess I forgot! turns out the option was unused so it was dead code anyways. |
49abae8
to
820f264
Compare
This was motivated originally by my cross work, but that goal requires a few more commits to other things. Still, it's good to start the cleanup now / get things out of the way.
820f264
to
76b5904
Compare
, enableRelease ? true | ||
, enableDebug ? false | ||
, enableSingleThreaded ? false | ||
, enableMultiThreaded ? true | ||
, enableShared ? !(hostPlatform.libc == "msvcrt") # problems for now | ||
, enableStatic ? !enableShared | ||
, enablePIC ? false |
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.
This is unused, and hardening provides it anyways
, enableRelease ? true | ||
, enableDebug ? false | ||
, enableSingleThreaded ? false | ||
, enableMultiThreaded ? true | ||
, enableShared ? !(hostPlatform.libc == "msvcrt") # problems for now | ||
, enableStatic ? !enableShared | ||
, enablePIC ? false | ||
, enableExceptions ? false |
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.
this is unused
Motivation for this change
This was motivated originally by my cross work, but that goal requires a few more commits to other things. Still, it's good to start the cleanup now / get things out of the way.
Things done
Native is almost exactly tested as part of #26805. However, In that version I had this setup hook replicating some logic with
findInputs
downstream that's since been removed. I removed the setup hook for this PR so now this is technically untested, but I doubt anything changed.Actually its even better, the setup hook traverses propagated dependencies, but no version of boost has any, so its demonstrably a no-op!
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)