cmake: add __structuredAttrs support to setup hooks

[ShamrockLee]

Description of changes

Summary

Provide structural attributes support to the setup hooks of CMake and lint them with ShelllCheck.

Fix #289037.

### New interface

Provide interface to structurally specify CMake -D flags, i.e. cmakeDFlagAttrs and cmakeDFlagEvalAttrs. Not only can it be specified by phases/hooks run before cmakeConfigurePhase, 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

• Naming.
  • Any suggestions to the new names cmakeDFlagAttrs and cmakeDFlagEvalAttrs?
• Boolean and non-string type management.
  • Should we allow boolean and other non-string type when 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

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[nixos-discourse]

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

[jtojnar]

Could you please split the __structuredAttrs support and cmakeDFlagAttrs feature into separate commits or PRs? The former is trivially mergeable but the second would deserve a bit of discussion.

---

As for name, I believe CMake calls the -D flags definitions.

Other questions:

• How does the dFlags interact with regular flags – which should take precedence when there are collisions.
• With indexed arrays/strings, it is quite easy to add a flag to the start to make it overridable. Do we want this feature with the associative arrays? I guess the writer can always check if the key is in the array and not override it then – the prepending would already have to be done in shell.
• Is the eval variant something we want, or is it obscure enough that it can be done in shell when needed. I know it could be useful sometimes but is it worth it the increased API surface and reduced consistency (people might start to expect it in any other flags-type variable)?

[ShamrockLee]

I'll fix __structuredAttrs = true with minimal interface changes in this PR, and place the new interface in a new PR. The ShellCheck linting will still be done here to provide a clean ground for subsequent development.

• With indexed arrays/strings, it is quite easy to add a flag to the start to make it overridable.

Oops! I mistook the relation between flag order and precedence. I'll fix it.

• How does the dFlags interact with regular flags – which should take precedence when there are collisions.

Regarding the precedence, I'll try to keep the original precedence in this PR. However, I personally think we should allow custom-specified cmakeFlags and cmakeFlagsArray to take precedence over the default values given by CMake setup hooks, contrast to the current implementation.

If we decide to add the attr-based interface, I hope that the attrs-based approach becomes the typical approach, and cmakeFlags and cmakeFlagsArray becomes the fall-back approach that takes precedence over attrs-based flag specification, both of which take precedence over the default values provided by cmake.setupHooks.

With indexed arrays/strings, it is quite easy to add a flag to the start to make it overridable. Do we want this feature with the associative arrays? I guess the writer can always check if the key is in the array and not override it then – the prepending would already have to be done in shell.

It's easier to override an Nix Language attribute set or a Bash associative array. The latter looks like

cmakeDFlagAttrs[FLAG_NAME]=NEW_VALUE

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.

[ShamrockLee]

• Is the eval variant something we want, or is it obscure enough that it can be done in shell when needed. I know it could be useful sometimes but is it worth it the increased API surface and reduced consistency (people might start to expect it in any other flags-type variable)?

Tthis interface is inspired by numtide/devshell's env.<name>.eval configuration option. (See the TOML schema and flake-parts option documentation for detail.) The motivation is to reduce the need of cmakeFlagsArray by providing something structural and overridable way to specify a Bash expression that evaluates to the value of a CMake definition flag.

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.

[ShamrockLee]

Re-implement the __structuredAttrs fix with cmakeFlagsArray. The original precedence, i.e. setup-hook-provided flags > cmakeFlags > cmakeFlagsArray, is kept by first prepend cmakeFlagsArray with cmakeFlags (__structuredAttrs handling goes here), and then prepend cmakeFlagsArray with setup-hook-provided. The resulting flags are all included in cmakeFlagsArray, which will then be used in the command executing cmake.

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 cmakeFlagsArray instead of cmakeFlags, and cmakeFlags itself is also injected into cmakeFlagsArray. This should not cause trouble unless cmakeFlags is searched against after cmakeConfigPhase, an extreme case that thankfully doesn't appear in the Nixpkgs codebase. (To validate that, I searched across the code with regular expression cmakeFlags(?! =) in VSCode and went through each of the 341 matches across 112 files.)

I personally think that the precedence should be reversed, but I won't change it here to limit the scope of this PR.

[AndersonTorres]

This function deserves its own file. It can be useful for other setup hooks.

[AndersonTorres]

The tests are merely shell-checking? Why?

In the worst hypothesis this files will never need a change before Cmake 4.

[ShamrockLee]

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.

[AndersonTorres]

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.

[ShamrockLee]

@jtojnar According to the cmake man page 1, the last argument takes precedence over previous ones.

In the current interface, one overrides a CMake cache variable already specified by a flag by appending the new flag to cmakeFlagsArray. The current precedence: cmake-setup-hook-provided default < cmakeFlags < cmakeFlagsArray is quite reasonable IMO.

[ShamrockLee]

I just rebased the changes onto the merge base of origin/staging and origin/nixos-unstable to make merging easier while trying to reduce the CI workload.

I also formatted the newly added .nix file with nixfmt-rfc-style to conform to Nix RFC 166.

Is there anything else I could do to push it forward?

[AndersonTorres]

The passthru.tests should be removed.

Also, the updateScript should be splitted to its own commit.

[ShamrockLee]

The passthru.tests should be removed.

Addressed.

Also, the updateScript should be splitted to its own commit.

This pull request doesn't touch the updateScript. If it is not for adding passthru.tests, we could keep passthru.updateScript intact.

[ShamrockLee]

@AndersonTorres, there is another pull request (#318614) that tries to add support for __structuredAttrs using a set of helper Bash functions to a series of setup hooks, including the CMake one. The author of that pull request, wolfgangwalther, has different opinions about the implementation detail.

Given that #318614 would require more time to review, we could merge this first, and then they could integrate it into their pull request. How do you think about it?

[wolfgangwalther]

Now that #318614 is merged, you could rebase on it and use the cmakeDefaultFlags style mentioned in #318614 (comment)?

[ShamrockLee]

Now that #318614 is merged, you could rebase it and use the cmakeDefaultFlags style mentioned in #318614 (comment)?

The purpose and implementation of the cmakeDefaultFlags changes would differ entirely from this PR. I prefer to implement them in a new PR and preserve the discussions here.

[ShamrockLee]

I'm closing this PR as its original goal is completed by #318614. Other changes proposed in this PR and the discussion will be split into new PRs.