Regression breaking build of glib

[xclaesse]

This PR in Meson master breaks build of glib: #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.