Allow passing dict to some library() args

[xclaesse]

There are currently 2 use cases where we want to pass different arguments to library() function if it's shared or static, see #4133 and #3304. A complete solution like the one proposed in #4019 (comment) is probably too complicated and controversial.

This PR allow syntax like this:

cargs = {
  'static_library' : ['-DSTATIC_COMPILATION']
}
suffix = {
  'static_library' : 'lib'
}
mylib = library('foo', sources, c_args : ['-DFOO', cargs], name_suffix : suffix)

This is very similar to my old PR #3320, but using a dict instead of a new object type.

[xclaesse]

If the idea is approved, I'll add more tests & doc.

[ePirat]

As already stated in #3820, I think this is a bit weird way of doing it and complicates things quite a lot.
The example you posted is IMO hard to understand/error prone as it gets rid of the nicely defined kargs and instead you have a dict with "magic" keys, which I think is not good.

[ePirat]

I understand though that there is a need to solve this, I am just not quite convinced about the specific way it is solved in this PR. That said, I do not yet have a better idea.

[ePirat]

After thinking about this for a while, I could not come up with a better way of doing things, so I guess let's go with this approach. 👍

[xclaesse]

Rebased and added doc. I think this is ready to be merged. @jpakkane any objection?

[jpakkane]

The syntax for this is confusing in ways I cannot really put to words. What about something in the vein of #3820 like:

x = library(...,
  static_kwargs : <dict of stuff>,
  shared_kwargs : <other dict of stuff>,
)

[xclaesse]

How is that better?

[ePirat]

IMO this is better as it's clearer what exactly it does, while for your proposal you have to guess what it does

[xclaesse]

No, it's worst because if you have both c_args : '-DFOO' and shared_kwargs : {'c_args': '-DBAR'} you don't know if one override the other, or if they are appended in which order.

[ePirat]

That's easily solvable by naming it either static_override_kwargs or static_append_kwargs depending which approach we want to take for them. But anyway, this is just my opinion, of course.

[jpakkane]

If it were called default_shared_kwargs then it would behave the same as default_kwargs and it would be unambiguous.

Or we could make it a hard error to have a kwarg both in the function and the dict. Or something else.

[xclaesse]

Oh and if we merge default_kwargs it gets even worse, which one override the other? I would say shared_kwarg should override default_kwarg but implementing that won't be easy if default_kwarg is implemented in a generic way.

[xclaesse]

So I really don't see how that solution is any better.

[dcbaker]

What if we did something like:

cargs = {
  'always': ['-DFOO'],
  'static_library' : ['-DSTATIC_COMPILATION']
}
suffix = {
  'static_library' : 'lib'
}
mylib = library('foo', sources, c_args : cargs)

specifically enforcing that if something is passed as a dict it must be only a dict, no ambiguity, and no chances of having two dicts passed at the same time.

[jpakkane]

Passing default_kwargs and shared/static_defaults_kwargs must be a hard error. No questions about that, otherwise it gets too complicated.

[xclaesse]

I already didn't see how that proposal was better, and given those constraints IMHO it's clearly worse. In addition, my PR can be merged today, while yours needs extra work for little to none benefice, for a feature we have been wanting for months. That really seems bikesheding to me.

[dcbaker]

We're talking about language syntax, we don't get to take it back if we make a mistake here, we get stuck with it. I think its worth thinking about the corner cases.

In the vein of thinking about corners, what happens in your PR if I do this:

cargs = {
  'static_library' : ['-DSTATIC_COMPILATION']
}
othercargs = {
  'static_library' : ['-DSOMETHING']
}

mylib = library('foo', sources, c_args : ['-DFOO', cargs, other_cargs])

[xclaesse]

They are appended in the order you give them. See it as it's flattened just like arrays everywhere.

[mstorsjo]

Does this proposal also allow conditionally limiting what sources to include when linking a library, depending on the library type?

My use case is that when including a windows resource file when building a shared library (to provide metadata about the library), it only makes sense to do this if actually making a shared library. For libraries whose type depend on the --default-library option, one will also end up including the same resource file in the static libraries, which doesn't make much sense. (Including resource files in static libraries can also break creating the library, when targeting non-x86 platforms.)

[textshell]

I strongly dislike the variants using some form of 'kwargs'. I think we should not design meson so that basic use cases need use the reflection like syntax. It's well very advanced scripting in complex projects, but simple projects should work without.

I'm not happy about the magic dictionary either. That feels a lot like having found an hammer and now trying to fix everything with that.

But i do like the way that the dictionary approach has a simple additive feel to it. But i feel that using dictionary in that way leads to very hard to understand extensions in the future.

I always liked that meson if fairly explicit and i think we should err on the side of being to wordy here.

Maybe all that is needed is some more semantic parts in the mix.

cargs = target_variants(static_library: ['-DSTATIC_COMPILATION'])

mylib = library('foo', sources, c_args : ['-DFOO', cargs], name_suffix : only_if(type: 'static_library', value:'lib'))

I don't mind having 'static_library' as string, but just having it as bare key in a dictionary seems way to implicit. I think we should aim for developers without much meson knowledge to be able to understand working build definitions by just reading them.

[xclaesse]

Maybe all that is needed is some more semantic parts in the mix.

cargs = target_variants(static_library: ['-DSTATIC_COMPILATION'])

mylib = library('foo', sources, c_args : ['-DFOO', cargs], name_suffix : only_if(type: 'static_library', value:'lib'))

That sounds like functionally the same thing, it's basically either of those:

• cargs = {'static_library': ['-DSTATIC'], 'shared_library': ['-DSHARED']}
• cargs = target_variants(static_library: ['-DSTATIC'], shared_library: ['-DSHARED']).
  If everyone agrees the new function syntax is better, I don't have strong opinion against, updating the PR shouldn't be too hard.

only_if(type: 'static_library', value:'lib') not sure what you're suggesting there, it's going back to static_library as a string, which is exactly what you wanted to avoid. I think that would rather work like this: library(..., name_suffix : target_variants(static_library: 'lib')).

In that proposal, target_variants() would be some kind of dictionary constructor where only a few keys are allowed. Values won't be checked in that function but rather where they are actually used, because it can be used in different places, c_args, name_suffix, etc.

[ePirat]

The target_variants (or similar named function) approach seems good to me. So I won't object to such a solution.

[xclaesse]

btw, I would call the function target_arguments() personally. But as always, better name suggestions are welcome. Or even library_arguments() since I think its use-cases are limited to library() call as far as I can anticipate.

[drobilla]

Apologies for resurrecting what seems to be a contentious bikesheddy PR, but are there alternative solutions to do this? As far as I can tell, one needs to explicitly build both static and shared libraries, avoid library() (essentially a toy feature) and reimplement default_library manually, which is pretty unpleasant.

For the record, though this is often presented as a Windows issue (because it's mandatory there), it's pretty common to use private visibility in shared libraries on all platforms. Libraries that care about dynamic loading and binary compatibility and so on use private default visibility and explicit exports, because it's important to have a clean symbol table in some domains (and it's just general best practice). It's really weird to have gnu_symbol_visibility suggest that this is supported, while library() isn't capable of it in practice.

[ePirat]

@drobilla I am not really sure I understand your issue? Why can't you use library() given that gnu_symbol_visibility should work just fine with it?

[drobilla]

@ePirat Because I can't detect in the code that a shared (or static) library is being built, so I can't define the export macros (or not). It works "just fine" in that it will hide all the symbols and make the library unusable, I suppose...

[ePirat]

@drobilla Ah indeed, if you support building both with library() this is indeed impossible. If not, you can just check the default_library option if is static or shared. But yeah, for both this is right now impossible.

[ePirat]

Or maybe I missed some change for this, given this issue is already quite old… Maybe @jpakkane or @xclaesse have an update on this.

[xclaesse]

I think the difference is on Linux you can use __attribute__ ((visibility ("default"))) regardless whether you build shared or static libraries. But on Windows you can use __declspec(dllexport) only when building a shared library and not when building static library.

That means that most projects that build with gnu_symbol_visibility: 'hidden' must define a macro used in the declaration of each public symbol that expands to something different on linux, windows/shared and windows/static.

That being said, I'm no sure to really understand those details myself.

[drobilla]

@xclaesse Yep. In general, anyway, though I think there are some cases where you don't want to blindly set visibility to public (e.g. in case the library is used inside another library which doesn't want to export it). It does mean you don't need to define different things for the library build itself and the user code, though, that is a Windows thing.

That means that most projects that build with gnu_symbol_visibility: 'hidden' must define a macro used in the declaration of each public symbol that expands to something different on linux, windows/shared and windows/static.

Yeah, this is near-ubiquitous. grep for 'visibility' in /usr/include. The conventions are all over the place but probably most common is two defines: MYLIB_INTERNAL defined when the library itself is being built (so not for users including the header), and MYLIB_SHARED when it's a shared library (or the other way around, MYLIB_STATIC).

Something like:

#if defined(MYLIB_SHARED) && defined(MYLIB_INTERNAL) && defined(_WIN32)
#    define MYLIB_API __declspec(dllexport)
#elif defined(MYLIB_SHARED) && defined(_WIN32)
#    define MYLIB_API __declspec(dllimport)
#elif defined(MYLIB_SHARED) && defined(__GNUC__)
#    define MYLIB_API __attribute__((visibility("default")))
#else
#    define MYLIB_API
#endif

[xclaesse]

@drobilla as far as I understand with GNUC you can # define MYLIB_API __attribute__((visibility("default"))) regardless of MYLIB_SHARED value, which means you can compile all .c files only once for shared and static libraries which is not possible on Windows.

Maybe this PR is trying to provide a generic solution to a specific problem. Since this symbol visibility story is really duplicated in so many projects, I'm wondering if meson could handle all that logic itself and define itself MESON_API_EXPORT and MESON_API_IMPORT directly ? For example in GStreamer https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/master/gst/gstconfig.h.in#L143 would become simply:

# ifdef BUILDING_GST
#  define GST_API MESON_API_EXPORT
# else
#  define GST_API MESON_API_IMPORT
# endif

[drobilla]

@xclaesse You definitely can do that (it is "default" after all), but I'm not entirely sure you always want to. There are also other more fancy visibility attributes.

That aside, this usually goes in public headers so I don't think that will work (meson may not be building whatever code is using it). Even if it did, I think it would be trouble for wraps to have build system specific stuff end up in the code though?

In an ideal world there'd be a standard for this, but oh well...

[xclaesse]

Right, won't work when building an app against your library without meson...

[xclaesse]

Another way of achieving this, what about: add_project_arguments('-DSTATIC_COMPILATION', language: 'c', target_type: 'static_library'). Similarly this could be used for another use-case that came on IRC recently: add -gz to executables: add_project_link_arguments('-gz', target_type: 'executable').

[drobilla]

Simple and useful for other stuff, that's nice. It's a bit less powerful I suppose, but Good Enough for most cases, I think.

[tristan957]

I like this, but I don't think it solves the case where you want some flags to apply to one particular target and not all targets (static libraries for instance) within your project. For me whatever solution is come up with, I would like it to increase the usability of library() when default_library=both

[xclaesse]

@tristan957 the idea is add_project_arguments('-DSTATIC_COMPILATION', language: 'c', target_type: 'static_library') would add that cflag when building the static library when building both. Note that it means it will recompile everything twice, there is no way around that.

[tristan957]

but if I have multiple static_libraries, and one of them need -DX=Y and the other needs -DX=Z, this doesn't work super well.

[xclaesse]

but if I have multiple static_libraries, and one of them need -DX=Y and the other needs -DX=Z, this doesn't work super well.

True, but does that really happen?

[tristan957]

You never know 🤷. I am fine with this approach. I just think someone in the future could be like "I need this", and then the solution would be suboptimal, and then we are back at the same issue of having to come up with a solution again.

[xclaesse]

You never know shrug. I am fine with this approach. I just think someone in the future could be like "I need this", and then the solution would be suboptimal, and then we are back at the same issue of having to come up with a solution again.

We usually don't design for hypothetical future use-cases. We can always extend later. That being said, @dcbaker actually has a real use-case for dict approach now...

[tristan957]

Yep I saw that, and was really exemplary of how the dict approach could be used to its fullest potential.

[bruchar1]

Can this be closed, now that #11981 is merged?