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
cmake: add __structuredAttrs support to setup hooks #299622
base: staging
Are you sure you want to change the base?
Conversation
38308e0
to
c7bc255
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/structured-cmakeflags-attrs/6261/4 |
72d67fe
to
584ad4d
Compare
Could you please split the As for name, I believe CMake calls the Other questions:
|
I'll fix
Oops! I mistook the relation between flag order and precedence. I'll fix it.
Regarding the precedence, I'll try to keep the original precedence in this PR. However, I personally think we should allow custom-specified If we decide to add the attr-based interface, I hope that the attrs-based approach becomes the typical approach, and
It's easier to override an Nix Language attribute set or a Bash associative array. The latter looks like
developers don't even have to remember the order of the flags. On the other hand, it's much easier to check the existence of a flag and its current value in an attribute set / an associative array, than in a list / an array / a string. Attribute sets are also friendlier when it comes to package overriding. |
Tthis interface is inspired by numtide/devshell's Considering that it makes the interface more complex, I don't have a strong opinion toward keeping it if that sounds too much. We could also add it in the future, as it is more like an add-on independent to the core functionality of this setup hook. |
584ad4d
to
f6edaa2
Compare
Re-implement the The changes made to the build environment, comparing to the current setup hook implementation, is that the flags provided by the setup hook is injected into I personally think that the precedence should be reversed, but I won't change it here to limit the scope of this PR. |
pkgs/by-name/cm/cmake/package.nix
Outdated
passthru = { | ||
tests = | ||
let | ||
shellcheckBashSourceFile = |
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 function deserves its own file. It can be useful for other setup hooks.
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.
The tests are merely shell-checking? Why?
In the worst hypothesis this files will never need a change before Cmake 4.
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.
The tests are merely shell-checking? Why?
In the worst hypothesis this files will never need a change before Cmake 4.
There was no test at all.
This PR merely fixes the shell script. All the CMake logic stays unchanged, including the CMakeList.txt patching, name extraction, and the result CMake command being executed.
Should I add some instrumentation tests to ensure that the script handles the specified flags correctly?
To be fair, the ShellCheck tests are not trivial as it seems, considering the vast quality improvement of cmake/setup-hook.sh
just to pass the check.
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, why are you adding these tests?
Usually what I expect from a test in passthru.tests
is that it fails (with a loud scream) when an update is going to break something.
The typical example is live555
: it can possibly compile and run fine, but it is a dependency of vlc
.
If the API of live555 changes and vlc does not catch this change, then it is not recommended to merge a new live555.
Then we added vlc as passthru.tests of live555.
On the other hand, the tests you added here do nothing besides linting our own code.
It merely checks if we did a good job writing our own shell script utilities.
And it is unlikely these scripts will ever change - maybe it will happen only when a new release of cmake exposes new functionalities (and/or removes some).
That being said, these tests do not look so useful. When a new release of cmake arrives, my last concern is if the setup hook code still shell-checks.
@jtojnar According to the In the current interface, one overrides a CMake cache variable already specified by a flag by appending the new flag to |
db6ef5c
to
d8b1ee7
Compare
d8b1ee7
to
8d7d598
Compare
Fix cmakeFlags when __structuredAttrs = true is specified. Support specifying CMake -D flags through attributes cmakeDFlagAttrs and cmakeDFlagAttrsEval. Support specifying CMake backend through attribute cmakeBackend.
Supress unnecessary ShellCheck checks.
8d7d598
to
b10b393
Compare
b10b393
to
45a5ba8
Compare
Write CMake command arguments to the output file - Check the precedence of each way of CMake definitions specification. - Check if the result is equivalent with/without structured attributes.
45a5ba8
to
80a12ec
Compare
I'm putting off the ShellCheck-based tests while waiting for #300538, and add some tests that checks the existence and precedence of CMake flags. The new test cases ( The change of the script reflects the Nixpkgs infrastructure changes. I would expect further adaption to meet the need of structural build specification. The behavior of the CMake binary itself is guarded during the bootstrapping process, as a large number of |
OfBorg build on x86_64-darwin failed because of "No space left on device". |
A bad thought crossed my mind: |
Adding ShellCheck tests will surely cost more time to build It does sound (more than a bit) too much just to make sure that the code gets linted. I'm not going to add those tests in this PR. To clarify, this is unrelated to the "no space left" issue on macOS OfBorg machine, as I have already removed the |
Description of changes
Summary
Provide structural attributes support to the setup hooks of CMake and lint them with ShelllCheck.
Fix #289037.
### New interfaceProvide interface to structurally specify CMake-D
flags, i.e.cmakeDFlagAttrs
andcmakeDFlagEvalAttrs
. Not only can it be specified by phases/hooks run beforecmakeConfigurePhase
, but also as Nix Language attributes when__structuralAttrs = true
.Update: The interface change is hold back here, and will be discussed in a subsequent PR.
Questions
cmakeDFlagAttrs
andcmakeDFlagEvalAttrs
?cmakeDFlagAttrs
is used inside the Nix Language expression? That would be quite convenient in Nix expression level, but require an extra layer of conversion. This PR currently doesn't provide such support.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.