Add <lang>_(static|shared)_args

[dcbaker]

This adds an option to pass c_static_args and c_shared_args. I thought about the two basic approaches we've discussed, which is adding a dictionary argument type to c_args, or adding additional keyword arguments.

I went with the additional keyword arguments for a couple of reasons:

1. This is a thing that exists only in the interpreter level, once the interpreter is done and the build layer doesn't understand that it was created via a build_target() or library() or both_library() or a specific target, so what is passed the build layer will still need to be Mapping[str, str | File]
2. because of that, using nested dictionaries complicates the implementation considerably, both in tracking when an argument was added, and what values are allowed; ie, it is less code and simpler code to implement separate arguments.

When I implemented the dictionary approach, even more code was required, and tracking the correct since values turned out to be much harder, plus there would be no good way to add new since values to the dictionary itself, our current infrastructure doesn't support that and would have to be extended.

[eli-schwartz]

because of that, using nested dictionaries complicates the implementation considerably, both in tracking when an argument was added, and what values are allowed; ie, it is less code and simpler code to implement separate arguments.

Fair enough! This seems like a more than adequate approach.

[xclaesse]

This goes full circle, I made a similar PR 5 years ago... IIRC one reason for not going that path is because there are other kwargs that needs to be different for static/shared, like name_suffix. That said, since nobody had a better idea in all those years, at some point we should just move ahead and do something, so I'm in favor of this PR.

From implementation POV, I think it's missing setting reuse_object_files=False in build_both_libraries() in the case any <lang>_(static|shared)_args is not empty.

[xclaesse]

For reference, the alternative proposal using dict is in #11501, and an older attempt at #4159.

[bruchar1]

Why file?

[xclaesse]

Checking how <lang>_args works, some parts of the code seems to allow File, but documentation does not mention that, and I see many code paths that does not seems to support it at first glance.

Allowing File does make sense, I guess it's common to pass a file in compiler arguments, but doc needs to be updated and it would have to be catched as new feature by KwargInfo. It would also need to ensure we have a unit test for that, because from reading the code I'm not convinced it works. Probably we need a convertor in KwargInfo to only get strings?

[eli-schwartz]

Passing arguments as files is a niche use case because the main time you do that is for sources (which of course supports files() objects). There are some cases where you want to pass non-source files, but the main example I can think of is linker scripts and particularly -Wl,--version-script=<file> which simply doesn't work as a discrete file object.

You could pass ['-T', files('foobar.ld')] though, if you're doing firmware rather than boring old "shared libraries with versioned symbols".

For compile time options there's ['-include', files('sharedmacros.h')] but use of '-include' is typically done at the project scope.

[dcbaker]

I guess that should actually be list[str | file]. And that the documentation for <lang>_args is wrong too, since it says list[str], but should also be list[str | file]

I fixed both in the latest version

[xclaesse]

I would leave this out of this PR because I'm not convinced it actually works, or add a unit test with a File in c_args to convince me...

If I understand correctly, we use that there: https://github.com/mesonbuild/meson/blob/master/mesonbuild/backend/ninjabackend.py#L2923, and will crash there: https://github.com/mesonbuild/meson/blob/master/mesonbuild/backend/backends.py#L959. But I may be missing something in how that code works.

[xclaesse]

ahah, yes, as predicted:

  File "/home/xclaesse/programmation/meson/mesonbuild/backend/ninjabackend.py", line 2923, in _generate_single_compile_target_args
    commands += self.escape_extra_args(target.get_extra_args(compiler.get_language()))
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/xclaesse/programmation/meson/mesonbuild/backend/backends.py", line 959, in escape_extra_args
    if arg.startswith(('-D', '/D')):
       ^^^^^^^^^^^^^^
AttributeError: 'File' object has no attribute 'startswith'

[dcbaker]

So this is extra awesome, because this totally works for Vala. So it has to be handled here, since we have to allow it or we'd regress working code...

There is even a manual tests/7 vala composite widgets that tests this, though it doesn't seem we have an automated test that does. So we can't take this out, it has to remain. Sigh

[dcbaker]

But maybe we can handle this in the interpreter instead of sending it to the backend at all....

[dcbaker]

Okay. So I did some git archeology. This was explicitly turned on for Vala in 0.25 (see: cd40187). That means that turning it off would constitute a regression in my mind, since the explicit purpose of the patch was to allow this. So, I've allowed this for Vala, and only for Vala. And I've moved the conversion code to the Interpreter level, so that the backend/middle layer doesn't have to be aware of this (except for creating the dependency)

[bruchar1]

LGTM. I really need this feature.

[eli-schwartz]

s/both_library/both_libraries/g

?

[eli-schwartz]

Why are we adding <lang>_<static/shared>_args to the static/shared library functions as well? Shouldn't this just be for the target types that are able to support both and have to differentiate?

[dcbaker]

I didn't see any reason not to add it here, the implementation has to handle it either way (and it's simpler from an implementation point of view this way), but I can drop it from shared_library and static_library if you'd prefer.

[dcbaker]

I didn't see any reason not to add it here, the implementation has to handle it either way (and it's simpler from an implementation point of view this way), but I can drop it from shared_library and static_library if you'd prefer.

[eli-schwartz]

@dcbaker I think so, yes. If it's easier for the implementation to know about it I don't mind, but I'd like to discourage people from going out of their way to define a static_library and then passing c_static_args instead of c_args as it feels like confusing UX.

Unless someone disagrees with me and would like to argue in favor of it being a UX enhancement, in which case I'd love to hear those arguments (or even concede the point as "I don't feel strongly enough against it as long as someone else feels strongly in favor".)

I would however like to avoid the situation where UX features are added based on the rationale that the implementation is simpler, rather than because it's a desired feature. There are a bunch of reasons why I feel this way, but maybe it will be convincing if I point out that that's how we ended up with build_target in its current state, complete with now-deprecated java support.

[dcbaker]

@eli-schwartz done. Now allowed only for library, both_library, and build_target

[dcbaker]

Although I just noticed that both_libraries isn't actually tested... sigh

That is fixed as well

[dcbaker]

Docs issue fixed locally

[eli-schwartz]

I feel like this belongs in commit 2, since even there we are producing this in a list comprehension plus repeating it for rust_args separately.

But also, the per-commit history shows an oddity, you add the initial version for most languages with File support and then remove it when migrating to the shared _BASE_LANG_KW.

[dcbaker]

I feel like that got squashed into the wrong place... sigh

[eli-schwartz]

This is fantastic stuff.

[eli-schwartz]

Couldn't we use TYPE_kwargs here?

[dcbaker]

I don't think that's accurate though, since TYPE_kwargs includes several Interpreter types in its values

[eli-schwartz]

Well, it is narrower than Any which explicitly allows anything. However that may not be too big of a deal... I guess it is easier to grep later on than incorrect use of TYPE_kwargs... and it allows checking the keys of course.

[bb010g]

These options should have corresponding options in declare_dependency(). A foo_lib = library('foo', c_static_args : ['-DFOO_STATIC_BUILD']) needs to pair with a foo_dep = declare_dependency(compile_static_args : ['-DFOO_STATIC_BUILD'], link_with : [foo_lib]) to be robust when foo_lib's type changes to static. Alternatively, returned lib objects need to allow querying for their type, so that foo_lib can pair with foo_dep = declare_dependency(compile_args : foo_lib.target_type() == 'static' ? ['-DFOO_STATIC_BUILD'] : [], link_with : [foo_lib]) instead. Note that a compile_static_args option would mirror pkgconf's Cflags.private property; see also #3111.

[dcbaker]

@bb010g I'm workingo n that. Unfortunately declare_dependecy() doesn't currently have the concept of per-language arguments, and needs additional work to get there before we can add these.

[bb010g]

@dcbaker Are we likely to see compile_static_args be merged as a complement to the existing compile_args before per-language arguments are implemented? I'm disappointed that subprojects targeting v1.3.0 won't be able to use <lang>_static_args for dependencies they provide, at least without going through .pc file generation.