Meson: Regression breaking build of glib

[xclaesse]

This PR in Meson master breaks build of glib: https://github.com/mesonbuild/meson/pull/6179 beacuse it adds many \ in its config.h.

[xclaesse]

The reason is glib does this:

gl_extern_inline = '''
/*some comment*/
#if
#define blablabla
#endif
'''
glib_conf.set('gl_extern_inline', gl_extern_inline)

It assumes that the multi line string will be added verbatim in the config.h

[xclaesse]

@Jehan could you check if it can be fixed please?

[xclaesse]

as far as I understand, it's the intended behaviour of the PR, so the only thing we can do is reverse the commit. Or assume we are not backward compatible and provide a way to support glib's use-case.

[xclaesse]

I would recommend adding something like set('var', value, multiline : true).

[Jehan]

Can I get the accurate example of what glib was going and what they were hoping to get?

Because from the above example:

gl_extern_inline = '''
/*some comment*/
#if
#define blablabla
#endif
'''
glib_conf.set('gl_extern_inline', gl_extern_inline)

I think it would generate this in config.h (without the commit adding the \):

#define gl_extern_inline
/*some comment*/
#if
#define blablabla
#endif

Is it what they wanted to generate? If so, it looks like a very hacky misuse of the set() function (where we want to actually set a key-value pair; and here they would use the wrong newlines as a trick to output random other code, completely bypassing the key-value concept).

[Jehan]

P.S.: if I understood well what glib is trying to do, I think a new function should be created for what they do. Something like:

glib_conf.write('''
#if bla
#define random code
with(multiline);
/*whatever*/
#endif
''')

But then keep the set() function for what it is (a key-value pair, which means in the particular case when this becomes a C macro that we have to add \ in newlines).

[xclaesse]

I think it would generate this in config.h (without the commit adding the \):

#define gl_extern_inline
/*some comment*/
#if
#define blablabla
#endif

Exactly, it's a ugly hack to inject random code into the configured header. I agree ideally we should have a dedicated function for this, the problem now is backward compatibility. What I would recommend:
1) Add new function conf.write(text)
2) Add new kwarg conf.set('var', 'foo\nbar', newline : true)
3) conf.set('var', 'foo\nbar') should print a warning pointing to write() func and newline kwarg.

[Jehan]

Exactly, it's a ugly hack to inject random code

Yeah that's what I thought. 🤭

the problem now is backward compatibility

So really what glib does is just not right (they were using the fact that the set() function used to be bugged and now are unhappy because it is fixed! :P). So my opinion is that they should fix their build code to do their code injection in a proper way (there are other ways to do this, even while waiting for easier conf.write() to be implemented).

I am really unsure that adding a newline arg is really meaningful. Seriously the old way is just wrong. It doesn't mean a thing in the set semantic which is about creating a key-value pair. Really I am all for backward compatibility in the case of usage change, but backward compatibility for saving bugs and even creating weird new API (the newline parameter) feels very strange to me.

So my counter-recommendation would be to just implement a conf.write() and update glib (and any other project which might have used this hack) to use this conf.write() function or alternative code injection methods.

Now obviously you have a much bigger vote than mine in meson so if you really want to do this, so be it. I just wanted to voice my concerns, with the hope it gets some traction too. 😛

[xclaesse]

The problem is there are no other way of injecting code in the header file, as far as I know. Yes that hack is ugly, but meson has no other practical way.

Forcing to use new API is not acceptable solution for glib, because its maintainer wants to keep the minimum meson version required to the one shipped in debian stable (0.49.2). I personally disagree with that, but it's a reasonable argument.

I totally agree with you that it's abusing set(), and I totally agree adding a kwarg sucks. Everything sux. (╯°□°)╯︵ ┻━┻

[xclaesse]

I merged the revert PR for now, as @jpakkane said on IRC. Leaving this issue open to find what we can do now.

[xclaesse]

@Jehan btw, instead of new kwarg as I suggested above, it could be a new method. conf.set_multiline()? Naming is always hard...

[Jehan]

Everything sux. (╯°□°)╯︵ ┻━┻

Ahahah! Sure! 🤣

The problem is there are no other way of injecting code in the header file, as far as I know.

I didn't mean in meson. With explicit meson API, I guess no (as far as I know either and you probably know better). But there are definitely other ways, for instance more old-school ones with shell code for instance, pipe-appending|prepending, sed or whatnot. Or it could simply be variable replacement, also with configure_file() in meson. I mean there are ways, maybe a bit more cumbersome as long as a conf.write() is not implemented but that can definitely already be done. 🙂

Forcing to use new API is not acceptable solution for glib, because its maintainer wants to keep the minimum meson version required to the one shipped in debian stable (0.49.2). I personally disagree with that, but it's a reasonable argument.

Yeah I do understand the reasoning but I don't agree this is a good reason to keep such a bug and even to create new API specifically to bypass the bug (instead of just fixing their usage and use other implementation to get the exact same result).

What I don't get though is: if they don't want to implement any of the other solutions (which seriously would be the real thing to do here and we would not have to discuss all this), then as long as we release the fixed .set() together with a new .write() API, would it be ok? Sure it would be bothersome too (they'd need to have alternative codes for before and after the version change), but then they'd always have a solution.

If that were acceptable, I would propose to implement this .write() API and when it gets merged, we also bring back the .set() fix. How would it sound?

it could be a new method. conf.set_multiline()? Naming is always hard...

Yeah naming is indeed hard! Though here my main problem is not really about naming but really about the fact that this option (be it as a parameter to set() or as a new function on its own) means nothing semantically. Actually when I read the conf.set('NAME', 'value', multiline=True) proposition, I honestly don't understand what it means. Does it mean "fake code injection" or "set a key-value as per function name" (because there are multilines in both case, so what does multiline=True mean?)? Which was it you hoped it meant?

To be more accurate, the real argument name should be buggy. When I read conf.set('NAME', 'value', buggy=True), I directly understand "ah ok they want the bugged version of the function which does not do what it is meant to do". It looks like I am joking, but I am only half-joking. 😛

Finally there are also the security considerations as @eli-schwartz raised in #6179. Sure there are other ways to inject code (as said many times here) and this is what they want to do here. But the difference when you do it through a function which is meant to do so (like the hypothetical conf.write()) is that it is meant to do so (yeah it is redundant… that's the point!), so you take the required precautions, you double-check the code.

Accepting the same as an unwanted side-effect of a function which was absolutely not meant to do this is dangerous because you don't pay as much attention. So it may end up being used malevolently by people trying to inject code before compilation. Say for instance you really wanted to set a key-value in config.h but someone manages to insert code after newlines in the used value (for instance through environment variables, etc.), then your program may end up calling some unwanted code by just including 'config.h', hence directly built into the program. And this would go completely unnoticed.

[xclaesse]

In any case, I'm not glib maintainer, so I cannot speak for them. FYI, I initially reported the bug against glib, you can comment there to cooperate on a solution with glib maintainer (Philip Withnall) https://gitlab.gnome.org/GNOME/glib/issues/1951.

As I said on IRC, if it was too hard to maintain compat, or if it was not breaking any major project, I wouldn't be reverting this patch. But sadly this is a special case here, we are breaking one of the most widely used library in the free software world... So IMHO we need to go the extra mile to find coordinated solution here.

[eli-schwartz]

It shouldn't have been reverted. It should have been updated to detect multiline replacements that begin with a newline, and raise a deprecation warning but not do the escaping.

Glib can fix its incorrect use of API by doing what it should have done all along: configure_file with input of config.h.in which contains a template for replacements, and also contains custom code which is not configurable.

[xclaesse]

Whatever is the solution, the right fix is to revert until we got a plan agreed with glib maintainer. I personally don't have the time to work on this, so I leave it to you guys to sort it out. Sorry.

[eli-schwartz]

You don't really need to wait on the glib maintainer just to add a deprecation warning while making sure glib's use case works. If the meson developers want to re-add @Jehan's bug fix then that deprecation warning and special case will need to be added regardless of what the blessed new solution is.

[Jehan]

It should have been updated to detect multiline replacements that begin with a newline, and raise a deprecation warning but not do the escaping.

Looks like a very good solution which works to warn for the fact while not breaking glib immediately (but getting them the time to fix their build).

Glib can fix its incorrect use of API by doing what it should have done all along: configure_file with input of config.h.in which contains a template for replacements, and also contains custom code which is not configurable.

Indeed that's what I meant in my previous comment when talking about variable replacement with configure_file. You said it with the right words. :-)

[Jehan]

I don't mind adding the deprecation warning/special casing when starting with a newline. Should I do a patch? Unless you want to do it, @eli-schwartz (I'm not fighting for the patch, I have a lot of work already 😉 and I'm really unsure when I'll have the time to do so)?

[Jehan]

Hmmm… actually I quickly did the patch adding the warning + special-casing already but I am too tired to actually test it (i.e. build glib now to test). I'll do the test tomorrow and push a merge-request for verification when I will have confirmed it to be good.

Now I'll go to bed! 💤

[eli-schwartz]

I could do it on Sunday or Monday. But it's fine if someone else does it first.

[ePirat]

The problem is there are no other way of injecting code in the header file, as far as I know. Yes that hack is ugly, but meson has no other practical way.

The solution to this is to use a config.h.meson template file and add whatever is needed there? Thats what I do for VLC.

[xclaesse]

The solution to this is to use a config.h.meson template file and add whatever is needed there? Thats what I do for VLC.

Not that easy because that part of the header is optional. It's actually only appended into the header only in some specific cases. Also going back to needing a template file feels like a step backward to me.

[eli-schwartz]

So conditionalize it on a define...

As for being a step backwards, so is reverting the commit that this issue is about.

The only correct solution to this is to deprecate the thing that glib is doing (which it should not have done and should have reported as a bug) and either say "but you could have always used a template" or "okay, let's add a special API for this extremely edge case scenario".

Either way, it's wrong to use set() like this. It was always wrong. Any solution going forward must acknowledge that it was wrong.

For the sake of being polite and allowing people to build old versions of buggy glib using new versions of meson, the courtesy of adding a deprecation shim for people relying on the buggy behavior shall be instituted.

[ePirat]

I absolutely agree with @eli-schwartz on this.

[eli-schwartz]

The question then becomes, acknowledging that conditionally defining large code chunks in config.h is an inelegant thing to do in the first place, does meson want to add a special API to make it easy to mix custom code and configuration_data autogeneration, or does it want to say "just generate it manually, the autogeneration code cannot serve every possible case and that's okay".

[xclaesse]

If we are going to deprecate a behaviour, better have an elegant solution to offer, otherwise it's just going to get people angry. At first glance I don't think template is a solution here, because it has more than one snippet to put into the header file, depending on different conditions.

I personally still believe we should have configuration_data.write() or call it as you want. By far the easiest and most flexible solution for everyone.

[tp-m]

Slightly off topic, and I'm just thinking out loud here, but Meson could consider introducing a concept of 'Epochs' or somesuch, and the active one could be based on the meson version requirement advertised in the meson file if no epoch was picked explicitly. This would make breaking changes easier for those targetting cutting edge meson versions, while providing a smoother upgrade path for other projects. Means more maintenance burden of course, but also makes things easier in some respects.

[Jehan]

If we are going to deprecate a behaviour

Technically we are not really deprecating anything since this behavior was never officially accepted. They were just taking advantage of a bug. 😉

In any case, I opened #6267 with the patch to allow GLib use case with a deprecation warning, while still keeping the fix for general use cases.

I personally still believe we should have configuration_data.write() or call it as you want. By far the easiest and most flexible solution for everyone.

The .write() function is a good idea (I proposed it, not going to say the opposite). But really solutions already exist and are definitely not that hard. It is actually just as easy to run configure_file() with a configuration file. We all do this, here in GIMP, and VLC above said they do too, and even looking into GLib various meson files, I saw they already do this for other similar code pieces (for instance glib/glibconfig.h is generated like this with similar replacements to output some #ifdef code).

I think that you might be overthinking this since it appears GLib maintainers never even said they would not fix this or that they thought the bug was in meson (cf. the GLib side bug report you gave at least; but maybe this was discussed in other places).

[jpakkane]

There are legitimate cases for not quoting newlines. For example defining a multiline raw string literal in C++:

const char []some_string = R"(First line.
Second line.
Third line.
)"

[textshell]

There are legitimate cases for not quoting newlines. For example defining a multiline raw string literal in C++:

const char []some_string = R"(First line.
Second line.
Third line.
)"

But that is not a config.h style #define situation.

so maybe

#define SOME_TEXT R"(First line.
Second line.
Third line.
)"

I'm not sure if that's supposed to work, clang and msvc accept it, gcc (trunk via compiler explorer) doesn't.

Anyway that's still a completly different thing than injecting lines that do not end up as a macro definition.

[xclaesse]

Was going to say the same thing, but @textshell said it better.

IMHO, we should merge the new API first (or together) so the deprecation message here can recommend using that new API instead of recommending using a template.

[xclaesse]

oh, you did it already, forget me :)

[pwithnall]

Hi everyone, thanks for handling this in a backwards-compatible way. One of the GLib maintainers here :wave:

Having read through all the comments here, I think I’m going to leave GLib using the set() bug for now, triggering a deprecation warning, and will look at ways to improve the situation when we can next bump our Meson dependency. Hopefully by that point we can depend on a version of Meson which has handy convenience API for this kind of situation.

As Xavier has said a few times above, there don’t seem to be any straightforward alternatives to using configuration_data.set(). People have suggested using a template file, but we have 200 calls to glib_conf.set(), many of which are conditional on several things. Turning that all into a template file would throw away all the advantages of configuration_data(). Ideally Meson would be able to provide some new API (like write(), or something else) which allows the two approaches (template files, or configure_file()) to be blended.

the thing that glib is doing (which it should not have done and should have reported as a bug)

In our defence, the Meson documentation has _very_ little to say about restrictions on how configuration_data.set() can be used.

For the sake of being polite and allowing people to build old versions of buggy glib using new versions of meson, the courtesy of adding a deprecation shim for people relying on the buggy behavior shall be instituted.

Thank you for being polite and maintaining API stability. (We have a similar policy in GLib: if we find a bug which we want to fix, but some third party code has demonstrably depended on that bug, we can’t fix it.)

I think that you might be overthinking this since it appears GLib maintainers never even said they would not fix this or that they thought the bug was in meson (cf. the GLib side bug report you gave at least; but maybe this was discussed in other places).

Yes, to be clear, the first I heard about this was in https://gitlab.gnome.org/GNOME/glib/issues/1951#note_658386. This comment is my first statement on the issue. :-)

[eli-schwartz]

In our defence, the Meson documentation has _very_ little to say about restrictions on how configuration_data.set() can be used.

It simply links to https://mesonbuild.com/Configuration.html

Which describes "Configuring without an input file" as generating a series of #mesondefine tokens (one per configuration_data dictionary element) and then treating that as the pseudo-input file.

The #mesondefine documentation on that page explicitly describes its use as being to create #define TOKEN value when given integer or string values; I'm quite sure it is excessively stretching the definition of #define TOKEN value to interpret this as valid:

#define gl_extern_inline<newline>
unrelated C code

It's plainly not intended to be used that way, so in the most charitable best-case scenario it is relying on undocumented implementation details and may break at any time without being noticed, as indeed happened.

I don't think it's fair to argue from the position of "you didn't document that we should not rely on undocumented implementation details". You saw a use for something not documented to work, you used it anyway, at the very least it should idealistically have been raised as a discussion point in a meson issue, "can this be documented as something we could rely on, or alternatively what is your best-practices suggestion?"
The response would likely be "this is confusing and feels bad, please don't"...

As Xavier has said a few times above, there don’t seem to be any straightforward alternatives to using configuration_data.set(). People have suggested using a template file, but we have 200 calls to glib_conf.set(), many of which are conditional on several things. Turning that all into a template file would throw away all the advantages of configuration_data(). Ideally Meson would be able to provide some new API (like write(), or something else) which allows the two approaches (template files, or configure_file()) to be blended.

I'm still not sure I see the problem. 99% of the template file can just be some #mesondefine tokens (the same ones that "Configuring without an input file" would have autogenerated for you in the pseudo-input file that you now make explicit), and all the way at the bottom you can add an inline code block containing the former contents of https://gitlab.gnome.org/GNOME/glib/blob/master/glib/gnulib/gl_extern_inline/meson.build, but guarded by:

#if defined(gl_extern_inline)

[stuff]

#endif

You should not even need to modify any existing files in git, other than the one call to:

configure_file(output : 'config.h', configuration : glib_conf)

Plus add an all-new config.h.in.