-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: master
Are you sure you want to change the base?
Conversation
9849d8e
to
640e40f
Compare
We intend to mimic the behavior of cmake which means we need to handle undefined keys and values the same
640e40f
to
730dd8f
Compare
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. |
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. |
730dd8f
to
0a162a6
Compare
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). |
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.