Skip to content

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

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

Conversation

GalaxyShard
Copy link
Contributor

Revival of #18079

I rebased the two commits and fixed the tests which now compile and run correctly.

@GalaxyShard GalaxyShard force-pushed the meson-config-headers branch from 783a2ee to 9854d63 Compare May 23, 2025 07:17
@GalaxyShard
Copy link
Contributor Author

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

@Jan200101
Copy link
Contributor

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.

@GalaxyShard
Copy link
Contributor Author

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:

#define TOKEN     // If TOKEN is set to boolean true.
#undef TOKEN      // If TOKEN is set to boolean false.
#define TOKEN 4   // If TOKEN is set to an integer or string value.
/* undef TOKEN */ // If TOKEN has not been set to any value.

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

switch (value) {
.undef, .defined => {},
.boolean => |b| {
try buffer.appendSlice(if (b) "true" else "false");
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@GalaxyShard GalaxyShard Jun 3, 2025

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.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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

talked to meson developers and the consensus is that this is a bug and AT replacement should error on boolean values

Considering that, when it comes to replacements, meson's set is exclusively for non-boolean values and set10 is for boolean values, I think it makes sense for Zig's addValue to mimic set for non-boolean values and set10 for boolean values that are passed to it because Zig does not have different addValue functions for different data types; it handles each type based on compile-time logic.

I disagree that it changes how boolean values are handled; my perspective is that Zig is simply choosing the behavior of the appropriate underlying meson function based on the data type.

Thought question: why error on boolean values when we know how to handle them unambiguously?

and its always possible for a Zig project to use 1 or 0 explicitly instead of relying on implicit boolean conversion

Although it's true that Zig projects can use 1 or 0 explicitly, meson projects don't; they use boolean values.

conf.set10('foo', some_boolean_value)
conf.set('bar', 1024)

Currently the above can be translated to something like this:

const conf = b.addConfigHeader(.{
    .style = .{ .meson = b.path("header.h.meson") },
    .include_path = "header.h",
}, .{
    .foo = some_boolean_value,
    .bar = 1024,
});

If we error on boolean values used in replacements, this would need to be changed to something like the following Zig code:

const conf = b.addConfigHeader(.{
    .style = .{ .meson = b.path("header.h.meson") },
    .include_path = "header.h",
}, .{
    .foo = if (some_boolean_value) 1 else 0,
    .bar = 1024,
});

It's not a huge difference but it's less intuitive and doesn't match the meson code as closely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`conf.set('foo', true) behaves uniquely in the context of mesondefine
here is a table copied from the docs:

conf_data.set('FOO', '"string"') => #define FOO "string"
conf_data.set('FOO', 'a_token')  => #define FOO a_token
conf_data.set('FOO', true)       => #define FOO
conf_data.set('FOO', false)      => #undef FOO
conf_data.set('FOO', 1)          => #define FOO 1
conf_data.set('FOO', 0)          => #define FOO 0

I think changing the behavior from set to set10 depending on if its used in variable substituion could cause confusion.

@GalaxyShard GalaxyShard force-pushed the meson-config-headers branch from 9854d63 to 6716481 Compare June 2, 2025 06:06
@GalaxyShard GalaxyShard force-pushed the meson-config-headers branch from 6716481 to 96ff024 Compare June 2, 2025 06:09
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.

4 participants