cmake: setup-hook.sh: split out cmakeDefaultFlags#335682
cmake: setup-hook.sh: split out cmakeDefaultFlags#335682ShamrockLee wants to merge 4 commits intoNixOS:stagingfrom
cmakeDefaultFlags#335682Conversation
52ec97d to
f118c78
Compare
|
@ofborg build cmake |
f118c78 to
adeb434
Compare
|
@ofborg build cmakeMinimal |
adeb434 to
1f1e1d3
Compare
1f1e1d3 to
a097601
Compare
|
I adjusted the style and changed the order to group all the non-conditional flags. @wolfgangwalther, what do you think about the current style? |
wolfgangwalther
left a comment
There was a problem hiding this comment.
The first two commits LGTM. We'll need to assume that the order of flags for cmake doesn't matter, though.
The last commit says:
This enables developers to get the default CMake flags provided by Nixpkgs without having to use cmakeConfigurePhase.
I don't understand the reason for that last commit, yet. Why is that important, what would you do with those flags?
In a build flow where other build tools control CMake, the package developer needs to configure the command-line arguments for CMake without executing CMake itself. A Bash function to provide the flags without running the whole Update: Such use cases are essentially edge cases. I shouldn't emphasize them too much. |
dd0682d to
a3cfdba
Compare
wolfgangwalther
left a comment
There was a problem hiding this comment.
First two commits LGTM. Only eye-balled the changes, did not build anything with it.
Last commit I can't say much about the usefulness, but it shouldn't do any harm.
|
Would it sound more clearly if the function is called |
Format with `shfmt -i 4 -w pkgs/by-name/cm/cmake/setup-hook.sh`. Keeping the code auto-formatted makes refactoring easier.
Collect the default CMake configure flags collected by cmakeConfigurePhase into a Bash array named cmakeDefaultFlags.
a3cfdba to
3cc89ba
Compare
|
Rebased and resolved merge confilcts. |
3cc89ba to
4d373dc
Compare
|
I split the source-independent @wolfgangwalther @AndersonTorres How do you fell about the new implementation? I could roll back to the previous one if the latter is preferred. |
cmakeDefaultFlags
b46374b to
470e053
Compare
470e053 to
fefeb96
Compare
wolfgangwalther
left a comment
There was a problem hiding this comment.
My feedback from earlier still applies:
First two commits LGTM. Only eye-balled the changes, did not build anything with it.
Last [two] commit[s] I can't say much about the usefulness, but [they] shouldn't do any harm.
|
OfBorg fails to build The |
Description of changes
This PR extracts the default CMake flags pretending code into its own Bash function
genCmakeDefaultFlags(). Instead of pretending the flags tocmakeFlags, it collects them into a global Bash arraycmakeDefaultFlags.Original discussions:
#318614 (comment)
#299622 (comment)
Cc: @AndersonTorres @jtojnar @wolfgangwalther
Things done
cmakeMinimal)nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.