-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
ConfigHeader: Add support for meson style templates #23943
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
783a2ee
to
9854d63
Compare
I've done some thorough testing of this with porting over all of the meson config headers from GTK and its several dependencies. All of the resulting headers are identical to the corresponding ones in my /usr/include (aside from the "This file was generated by ConfigHeader using the Zig Build System." comment). |
the big issue with mesondefine is that they don't strictly specify how its suppose to work and there is little to no regression testing, so a change upstream could cause interop issues with zig in subtle ways. |
Meson's documentation (https://mesonbuild.com/Configuration.html) defines how mesondefine is to be expanded:
If there were regressions, it would be a bug on meson's part, and it would break many projects using meson so I doubt any accidental regressions would become "features" especially considering the logic is fairly straightforward (the only 4 possibilities are in the block above; you'd have to break almost every project by changing the behavior of any one of them). |
lib/std/Build/Step/ConfigHeader.zig
Outdated
switch (value) { | ||
.undef, .defined => {}, | ||
.boolean => |b| { | ||
try buffer.appendSlice(if (b) "true" else "false"); |
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.
meson uses the uppercase spelling of True and False
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.
Looking closer at the current upstream source code, it appears meson fires an exception if a boolean value added with set
(the equivalent meson's of addValue) is used as a variable. Since every other configuration header style handles this case by substituting a 0 or a 1 in place of a boolean, and because meson has a dedicated function for adding boolean variables (set10
vs set
) that also substitutes 0's and 1's like that, I think I'll have it follow that behavior for now.
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.
you'd think that but in python bool
is an integer subclass
>>> var = True
>>> isinstance(var, int)
True
meson does str(var)
if var
is an integer instance so it will become True
or False
for boolean values... fun stuff
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.
Huh, that seems incredibly odd considering True
and False
aren't even keywords in C. Now the question is whether or not to use the behavior of meson's set10
, inserting 1's and 0's for variables (which is most likely the expected result), or the behavior of meson's set
, inserting True
and False
(which is the function which other types' expansions are based on).
I think there's a good argument for following the set10
behavior (which is what this PR currently does), since it's always possible for a Zig project to use the string or ident values "True"
and "False"
(or .True
, .False
) to get the behavior of set
instead of the intuitively expected set10
behavior.
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.
it would also change how bools are handled compared to meson
and its always possible for a Zig project to use 1
or 0
explicitly instead of relying on implicit boolean conversion
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.
talked to meson developers and the consensus is that this is a bug and AT replacement should error on boolean values
9854d63
to
6716481
Compare
…ation In meson, true, false, and null values for #mesondefine determines whether it expands to a #define, #undef, or commented #undef respectively. Additionally, meson does no string escaping and expands values as-is, without wrapping them in quotes to create a C string.
6716481
to
96ff024
Compare
Revival of #18079
I rebased the two commits and fixed the tests which now compile and run correctly.