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

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)?

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.

• 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.

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.

@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.

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?

The passthru.tests should be removed.

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

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.

@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?

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

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.

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.