Skip to content

std.Build.Step.ConfigHeader: handle undefined keys and values correctly #24014

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jan200101
Copy link
Contributor

@Jan200101 Jan200101 commented May 29, 2025

We intend to mimic the behavior of cmake which means we need to handle undefined keys and values the same

Zulip thread where I brought this up
https://zsf.zulipchat.com/#narrow/channel/454371-std/topic/ConfigHeader.20strictness/near/520861756

Here is a repo that where the output of cmake and zig is compared https://github.com/Jan200101/cmakedefine-test

on master running zig_diff.sh erors out while with this PR it outputs a matching file (minus header comment)

the cmakedefine standalone test was also modified to include a CMakeLists build solution to generate the expected headers and to test for undefined keys.

@Jan200101 Jan200101 force-pushed the PR/cmakedefine-unerror branch 2 times, most recently from 9849d8e to 640e40f Compare May 29, 2025 19:24
We intend to mimic the behavior of cmake which means we need to handle undefined keys and values the same
@Jan200101 Jan200101 force-pushed the PR/cmakedefine-unerror branch from 640e40f to 730dd8f Compare May 29, 2025 19:40
@GalaxyShard
Copy link
Contributor

These changes open up a bit of a footgun in the configuration header API by removing what are effectively safety checks. Although CMake doesn't implement these checks and silently/implicitly drops variables which were never specified, in my experience Zig's behavior of erroring unless the variables are explicitly set or nulled/undefined is quite useful for catching configuration mistakes.

Relevant comment from the associated issue: #21265 (comment)

It's worth mentioning that the behavior introduced by this PR was considered a bug in #20230 and this PR would revert #20234.

@Jan200101
Copy link
Contributor Author

I see those reasons and agree that it would be useful to be more strict but not as a default, the way cmake config headers are parsed is established by cmake and changing it at best shows you what config values weren't set and at worst breaks compatibility with complex cmake projects where the config header is generated at configure time with changing keys.

@GalaxyShard
Copy link
Contributor

GalaxyShard commented Jun 1, 2025

I see those reasons and agree that it would be useful to be more strict but not as a default, the way cmake config headers are parsed is established by cmake and changing it at best shows you what config values weren't set and at worst breaks compatibility with complex cmake projects where the config header is generated at configure time with changing keys.

As a counterargument, I would say that the headers are still parsed the same way; the output file from Zig and the output file from CMake is (able to be made) identical, provided that the options are specified the same (aside from the comment Zig adds at the top). The only change here is how the keys/values are specified in the build.zig file, which already differs from CMake because it's Zig code and not CMake code that specifies the keys/values.

If the keys change at build time due to having a generated file act as a input configuration header (which, I can't say I've ever seen something like that before, though it should be possible in both Zig and CMake), it is possible to specify and not specify certain keys in the build.zig by (conditionally) using addValue(s) instead of specifying all the keys upfront in addConfigHeader. If this conditional logic is tedious then I might suggest an alternative solution to the problem: add functions like "addValue(s)MaybeUnused" that allows those specific values to be unused, this way all the conditional values can be specified upfront even if some of them will remain unused due to them being removed from the input configuration file during build-time.

I imagine if a complex CMake project was continuously adding and removing keys based on compile-time logic, it would be useful to know when upstream decides to change the keys and breaks your build.zig rather than Zig continuing with the compilation as if it was fine. Likewise if a simple CMake project happens to rename one of its keys or add a new one, or someone simply forgot to specify one of the keys, an error message would be very helpful (and has been for me, based on past experience).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants