Skip to content
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

both_libraries() does not deal with dependencies correctly #5452

Closed
le-jzr opened this issue Jun 6, 2019 · 20 comments · Fixed by #12632
Closed

both_libraries() does not deal with dependencies correctly #5452

le-jzr opened this issue Jun 6, 2019 · 20 comments · Fixed by #12632

Comments

@le-jzr
Copy link

le-jzr commented Jun 6, 2019

Or dependencies don't deal with both_libraries() correctly. Depends on the viewpoint.

I'm trying to convert a complicated cross-compiling project into Meson.
We have a bunch of libraries and a bunch of executables. The libraries have dependencies on other libraries. Some executables need to be build statically, so link a static version of libraries that also need to be available as shared for other executables.
The libraries are all represented with declare_dependency() objects for convenience.

Since declare_dependency() does not handle the "bothness" of a library, I tried creating separate shared and static dependencies from both_libraries().
However, when trying to use both_libraries().get_static_lib() in a dependency object, it causes shared libraries to be added to the linker command line of the target executable, resulting in failed build. If separate shared_library() and static_library() are used instead of both_libraries(), everything works as expected.

EDIT: Removed last sentence. Using same name for shared and static library actually works, I just made a mistake and effectively tried using static_library() alongside both_libraries().

@xclaesse
Copy link
Member

xclaesse commented Jun 6, 2019

That sounds like a bug... But it's weird because we do have unit test for that I think. Ideally you should try to extend this test case to demonstrate the bug:
https://github.com/mesonbuild/meson/blob/master/test%20cases/common/184%20bothlibraries/meson.build

@le-jzr
Copy link
Author

le-jzr commented Jun 6, 2019

Here is a minimal test case demonstrating the issue:

project('both libraries linking test', 'c')

both_libs = both_libraries('mylib', 'libfile.c')
both_libs2 = both_libraries('mylib2', 'libfile.c', link_with : both_libs)

exe_dep_static = executable('prog-static-dep', 'main.c',
                         link_args : [ '-static' ],
                         c_args : ['-DSTATIC_COMPILATION'],
                         link_with : both_libs2.get_static_lib(),
)

The behaviour I expect is either the static version of first library being linked into the executable, or none at all (depending on what the intended behaviour is, which I'm not clear on). What actually happens is that Meson puts the shared version of first lib into the executable link.

EDIT: further simplified

@shdnx
Copy link

shdnx commented Nov 5, 2021

I think this is because you define both_libs2 with link_with : both_libs, which I think ends up choosing the shared or static library based on your default_library Meson option.

@eli-schwartz
Copy link
Member

The original report states that there is a bug using both_libraries().get_static_lib().

The minimal reproducer just links to both_libraries() and it is expected that meson prefers the shared library when given a choice. So, it is not a minimal reproducer at all.

Can someone demonstrate that there is in fact a bug here, and how to trigger it?

@le-jzr
Copy link
Author

le-jzr commented Nov 5, 2021

I thought it is clear from the text that the behavior I expect is not what actually happens. If the "both"-ness does not transitively apply to link arguments, then the static part of both_libs2 is not static at all. This contradicts my expectation of how it should work, since it basically nullifies all utility of the both_libraries() function. In my case, it forces me to explicitly make separate static and shared versions of every library in the dependency chain, which makes me question whether both_libraries() is intended to function this way.

@xclaesse
Copy link
Member

xclaesse commented Nov 5, 2021

That's a very common misunderstanding of default_library and both_libraries. They control only whether you build a static and/or shared library, but does not control that library dependencies.

What I could suggest adding in the Meson API is dep.as_static(), similar to dep.as_link_whole() that would switch all libraries to their static counterpart.

@eli-schwartz
Copy link
Member

both_libraries() has a great deal of utility. It can be used for installing both static and shared versions of a library to the system installation path.

As you indicated in your original report, the expectation if you want to incorporate the static version would be to do:

both_libs1 = both_libraries('mylib', 'libfile.c')
static_libs1 = both_libs1.get_static_lib()
both_libs2 = both_libraries('mylib2', 'libfile.c', link_with : static_libs1)

However, in your followup reply, you did not use the .get_static_lib() method you mentioned in your first post.

@eli-schwartz
Copy link
Member

If the "both"-ness does not transitively apply to link arguments, then the static part of both_libs2 is not static at all.

I don't actually understand what this means, can you elaborate?

@le-jzr
Copy link
Author

le-jzr commented Nov 5, 2021

To clarify, the issue here is what is supposed to happen when you link both_libraries() to both_libraries(). The argument that Meson prefers shared libraries by default is inapplicable, since both_libraries() explicitly overrides that default in the first place to build both shared and static version of a library. It doesn't make sense to me for a static version of a library to always link to shared dependencies, since to me the purpose of static library is to allow a static executable to be built, and this can't be done through both_libraries().

@le-jzr
Copy link
Author

le-jzr commented Nov 5, 2021

As you indicated in your original report, the expectation if you want to incorporate the static version would be to do:

both_libs1 = both_libraries('mylib', 'libfile.c')
static_libs1 = both_libs1.get_static_lib()
both_libs2 = both_libraries('mylib2', 'libfile.c', link_with : static_libs1)

This would conversely result in the shared version of both_libs2 always including both_libs1 statically, which is not desired.

@shdnx
Copy link

shdnx commented Nov 5, 2021

I agree that this greatly limits the usefulness of both_libraries().
We have the exact same situation as @le-jzr, where only the leaf libraries (the ones that don't depend on anything else) can use both_libraries(), and everything else has to separately build a static_library() and shared_library(), just so that the executables can be built using the static libraries, and our Python bindings can be built using the shared libraries.

@le-jzr
Copy link
Author

le-jzr commented Nov 5, 2021

That's a very common misunderstanding of default_library and both_libraries. They control only whether you build a static and/or shared library, but does not control that library dependencies.

To me it feels like a bug, since it contradicts my intuition (and if it's a "very common misunderstanding", not just mine).
However, if it is the consensus of Meson authors that this behavior is optimal, then feel free to close this ticket.

@shdnx
Copy link

shdnx commented Nov 5, 2021

Adding dep.as_static() and dep.as_shared() as @xclaesse suggests makes a lot of sense to me, and would have parity with how both_libraries() results in a variable that has a get_shared_lib() and get_static_lib().

@le-jzr
Copy link
Author

le-jzr commented Nov 5, 2021

Wouldn't that just push the issue one level up? From a brief look at the documentation, as_link_whole() doesn't seem to work transitively.

@shdnx
Copy link

shdnx commented Nov 5, 2021

Indeed, it would have to work transitively. So:

lib_x_lib = both_libraries(...)

lib_x_dep = declare_dependency(
    link_with: lib_x_lib,
    ...
)

lib_y_lib = both_libraries(
    dependencies: lib_x_dep,
    ...
)

lib_y_dep = declare_dependency(
    link_with: lib_y_lib,
    dependencies: lib_x_dep,
    ...
)

# links to lib_x and lib_y static libraries only
exe_a = executable(
    dependencies: lib_y_dep.as_static(),
    ...
)

# links to lib_x and lib_y shared libraries only
exe_b = executable(
    dependencies: lib_y_dep.as_shared(),
    ...
)

@eli-schwartz
Copy link
Member

Ah, okay -- thanks for elaborating. So you want both_libraries('lib2', ...) to detect that it is being linked to both_libraries('lib1', ...), and then specialize the dependency to link the static version of lib2 in a different manner from how it links the shared version.

Unfortunately, this is not currently how that is expected or designed to work. Perhaps it could be made to be. Here is some more food for thought:

curl_dep = dependency('libcurl')
lib1 = both_libraries('lib1', 'lib1.c', dependencies: curl_dep)
executable('exe1', 'exe1.c', link_with: lib1.get_static_lib())

Should the executable "know" that it is linking with the static fork of lib1, and follow that to statically link with libcurl?

In general, both_libraries is intended as it was originally implemented, to be sugar for defining a static_library() and a shared_library() with the exact same arguments, hence why they both use the same .get_*_lib() attribute of their dependency.

This is a bit problematic in additional ways beyond your ticket -- it's also currently impossible to build a both_libraries(), or even an agnostic library(), with different c_args specific to shared vs. static editions, something that is unfortunately often needed by Windows builds using declspec.

@le-jzr
Copy link
Author

le-jzr commented Nov 6, 2021

Should the executable "know" that it is linking with the static fork of lib1, and follow that to statically link with libcurl?

Good question. That would depend on whether the object returned by dependency() represents the "abstract" dependency itself (i.e. both static and shared version at the same time) or just the version that was decided upon at the time dependency() is called. Since there is no dep.get_static_lib() or dep.get_shared_lib(), I suspect it's the latter.

both_libraries() wraps both versions of the library and allows extracting a particular one via methods of the returned objects, but as you say, there's currently no way provide different arguments for the static and shared version. So an easy way to solve that issue and my use case at the same time would be to add two arguments to both_libraries() (and possibly library() as well). Something like args_static: dict accepting the same keys as arguments to static_library(), and args_shared: dict accepting the same keys as shared_library(), with either only applying to the respective build version.

However, the more I'm thinking about the original issue, the more it seems to me that my project was just slightly pathological in requiring some executables to be fully statically linked, while building others as dynamically linked, both in the same Meson script.

@eli-schwartz
Copy link
Member

If implemented (and it doesn't seem unreasonable), this should be feasible to do without adding new API, at least if we limit it to the case of chaining both_libraries().

Handling different c_args for static/shared is a whole different ballgame, as evidenced by #3304 #4019 #4159 et al.

@eli-schwartz
Copy link
Member

For something a bit more manual, you can offload at least part of the duplication of values by using one of the following currently existing options:

making a dict of kwargs to pass to a library invocation, and then passing the dict as

static_library('foo', kwargs: foo_args_dict)
shared_library('foo', kwargs: foo_args_dict)

Or use

foreach libtype: ['static_library', 'shared_library']
    lib1 = build_target(libtype, ....)
    lib2 = build_target(libtype, ...., link_with: lib1)
endforeach

Both options are a bit more noisy than having both_libraries(..., link_with: both_libraries(...)) infer your meaning.

@bruchar1
Copy link
Member

I think #12632 should address this issue.

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 a pull request may close this issue.

5 participants