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

Add both_library() to build both shared and static library #2711

Merged
merged 4 commits into from Apr 4, 2018

Conversation

Projects
None yet
8 participants
@xclaesse
Contributor

xclaesse commented Nov 29, 2017

Also support default_library='both' to make library() build both shared
and static.

Closes #484

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Nov 29, 2017

Contributor

There is one suboptimal case, the object returned value of library() has different API if built with default_library='both'. Not sure how to make this nicer.

Contributor

xclaesse commented Nov 29, 2017

There is one suboptimal case, the object returned value of library() has different API if built with default_library='both'. Not sure how to make this nicer.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Nov 30, 2017

Contributor

There is one suboptimal case, the object returned value of library() has different API if built with default_library='both'. Not sure how to make this nicer.

Fixed that by making BothLibraryHolder subclass BuildTargetHolder and passing it the shared lib target. That means passing it to 'link_with' will link with the shared library.

Contributor

xclaesse commented Nov 30, 2017

There is one suboptimal case, the object returned value of library() has different API if built with default_library='both'. Not sure how to make this nicer.

Fixed that by making BothLibraryHolder subclass BuildTargetHolder and passing it the shared lib target. That means passing it to 'link_with' will link with the shared library.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Nov 30, 2017

Contributor

Hmm, testing this on glib, I changed all shared_library() to both_library() and libgio fails with this message:

ninja: error: 'gio/gio-2.0@sha/meson-generated_.._gnetworking.h.o', needed by 'gio/libgio-2.0.a', missing and no known rule to make it

everything works if I keep shared_library() just for libgio.

Contributor

xclaesse commented Nov 30, 2017

Hmm, testing this on glib, I changed all shared_library() to both_library() and libgio fails with this message:

ninja: error: 'gio/gio-2.0@sha/meson-generated_.._gnetworking.h.o', needed by 'gio/libgio-2.0.a', missing and no known rule to make it

everything works if I keep shared_library() just for libgio.

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Nov 30, 2017

Member

I like the way this has been implemented, it matches what I imagined this feature would be like and I was going to work in it myself, so thanks and good job!

There's just one hitch: the return value of library(). Right now you are defaulting to the shared library for the object to link_with:, and people don't always want that.

It covers the most common use-case to link to the shared library and build the static library only for installation, or to link to a few internal utils. However, it is not uncommon to want to link the entire project (and subprojects) statically so your tools are standalone, while also providing shared libraries for installation (I've seen this done in embedded projects, in projects that are used in the initrd, and on Windows).

I think instead of having both as the default-library option, we should have shared-static and static-shared. Then, the held_object on BothLibraryHolder can be changed depending on which one is selected. Eventually, this explicitness will also help with building Rust targets which have even more types.

Member

nirbheek commented Nov 30, 2017

I like the way this has been implemented, it matches what I imagined this feature would be like and I was going to work in it myself, so thanks and good job!

There's just one hitch: the return value of library(). Right now you are defaulting to the shared library for the object to link_with:, and people don't always want that.

It covers the most common use-case to link to the shared library and build the static library only for installation, or to link to a few internal utils. However, it is not uncommon to want to link the entire project (and subprojects) statically so your tools are standalone, while also providing shared libraries for installation (I've seen this done in embedded projects, in projects that are used in the initrd, and on Windows).

I think instead of having both as the default-library option, we should have shared-static and static-shared. Then, the held_object on BothLibraryHolder can be changed depending on which one is selected. Eventually, this explicitness will also help with building Rust targets which have even more types.

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Nov 30, 2017

Member

ninja: error: 'gio/gio-2.0@sha/meson-generated_.._gnetworking.h.o', needed by 'gio/libgio-2.0.a', missing and no known rule to make it

I've seen this bug before elsewhere. It's because when figuring out the list of sources to use in extract_all_objects(), we don't skip over headers. See the sources classification in backends/ninjabackend.py:NinjaBackend.generate_target(). Specifically, look at how we handle the contents oftarget_sources and generated_sources.

This file classification needs to be factored out and probably moved to the BuildTarget object so it can also be used by extract_all_objects().

Member

nirbheek commented Nov 30, 2017

ninja: error: 'gio/gio-2.0@sha/meson-generated_.._gnetworking.h.o', needed by 'gio/libgio-2.0.a', missing and no known rule to make it

I've seen this bug before elsewhere. It's because when figuring out the list of sources to use in extract_all_objects(), we don't skip over headers. See the sources classification in backends/ninjabackend.py:NinjaBackend.generate_target(). Specifically, look at how we handle the contents oftarget_sources and generated_sources.

This file classification needs to be factored out and probably moved to the BuildTarget object so it can also be used by extract_all_objects().

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Nov 30, 2017

Contributor

I think instead of having both as the default-library option, we should have shared-static and static-shared. Then, the held_object on BothLibraryHolder can be changed depending on which one is selected. Eventually, this explicitness will also help with building Rust targets which have even more types.

I would rather call them both-shared and both-static? Naming stuff is harder than writing code...

Contributor

xclaesse commented Nov 30, 2017

I think instead of having both as the default-library option, we should have shared-static and static-shared. Then, the held_object on BothLibraryHolder can be changed depending on which one is selected. Eventually, this explicitness will also help with building Rust targets which have even more types.

I would rather call them both-shared and both-static? Naming stuff is harder than writing code...

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Nov 30, 2017

Member

[bikeshed mode on]

I would rather call them both-shared and both-static? Naming stuff is harder than writing code...

That makes me think "this option says 'both [are] shared'?" What I was thinking with Rust was you'd have shared-static-rlib or rlib-shared-static or rlib-static and so on, so it's more flexible that way for building more than two libraries at once.

Member

nirbheek commented Nov 30, 2017

[bikeshed mode on]

I would rather call them both-shared and both-static? Naming stuff is harder than writing code...

That makes me think "this option says 'both [are] shared'?" What I was thinking with Rust was you'd have shared-static-rlib or rlib-shared-static or rlib-static and so on, so it's more flexible that way for building more than two libraries at once.

@jibsen

This comment has been minimized.

Show comment
Hide comment
@jibsen

jibsen Nov 30, 2017

Contributor

However, it is not uncommon to want to link the entire project (and subprojects) statically so your tools are standalone, while also providing shared libraries for installation (I've seen this done in embedded projects, in projects that are used in the initrd, and on Windows).

I've got at least one project like this, where I build a statically linked executable and a dll.

One thing I'd like to note is that since default_library is global, this means if you have one subproject you'd like to build as both, you'll have to build your entire project as both.

Contributor

jibsen commented Nov 30, 2017

However, it is not uncommon to want to link the entire project (and subprojects) statically so your tools are standalone, while also providing shared libraries for installation (I've seen this done in embedded projects, in projects that are used in the initrd, and on Windows).

I've got at least one project like this, where I build a statically linked executable and a dll.

One thing I'd like to note is that since default_library is global, this means if you have one subproject you'd like to build as both, you'll have to build your entire project as both.

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Nov 30, 2017

Member

One thing I'd like to note is that since default_library is global, this means if you have one subproject you'd like to build as both, you'll have to build your entire project as both.

You can set it per-subproject by using the default_options: kwarg to subproject() and dependency().

Member

nirbheek commented Nov 30, 2017

One thing I'd like to note is that since default_library is global, this means if you have one subproject you'd like to build as both, you'll have to build your entire project as both.

You can set it per-subproject by using the default_options: kwarg to subproject() and dependency().

@jibsen

This comment has been minimized.

Show comment
Hide comment
@jibsen

jibsen Nov 30, 2017

Contributor

You can set it per-subproject by using the default_options: kwarg to subproject() and dependency().

That was what I initially thought, but apparently it can only override the default values in the subprojects meson_options.txt file. See #2612 (and #2619 which updates the docs).

Contributor

jibsen commented Nov 30, 2017

You can set it per-subproject by using the default_options: kwarg to subproject() and dependency().

That was what I initially thought, but apparently it can only override the default values in the subprojects meson_options.txt file. See #2612 (and #2619 which updates the docs).

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Nov 30, 2017

Member

Hmm, I don't like how we're handling this default_options stuff. It violates the principle of least-surprise and we constantly get issues and questions about it.

Member

nirbheek commented Nov 30, 2017

Hmm, I don't like how we're handling this default_options stuff. It violates the principle of least-surprise and we constantly get issues and questions about it.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Nov 30, 2017

Contributor

I updated my patch to include doc changes, and use the shared-static/static-shared default_library.

But now that I think about it, I'm wondering if it should be a separate option:
default_library=['shared', 'static', 'both']
default_both_library=['shared', 'static']

Contributor

xclaesse commented Nov 30, 2017

I updated my patch to include doc changes, and use the shared-static/static-shared default_library.

But now that I think about it, I'm wondering if it should be a separate option:
default_library=['shared', 'static', 'both']
default_both_library=['shared', 'static']

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Dec 1, 2017

Contributor

In this last version of the patch, I splitted the setting, to have "both_library" setting that says if it should link static or shared by default. I don't really like that setting name, suggestions?

Also it should fix a bug that it was actually building everything twice because I didn't reset the sources arg when doing the static build. I think that's what was failing CI.

Contributor

xclaesse commented Dec 1, 2017

In this last version of the patch, I splitted the setting, to have "both_library" setting that says if it should link static or shared by default. I don't really like that setting name, suggestions?

Also it should fix a bug that it was actually building everything twice because I didn't reset the sources arg when doing the static build. I think that's what was failing CI.

@jpakkane

This comment has been minimized.

Show comment
Hide comment
@jpakkane

jpakkane Dec 2, 2017

Member

Maybe we could leave out the possibility of building both types with library? Having the return type change based on options is a bit spooky-action-at-a-distancy.

Member

jpakkane commented Dec 2, 2017

Maybe we could leave out the possibility of building both types with library? Having the return type change based on options is a bit spooky-action-at-a-distancy.

@jpakkane

This comment has been minimized.

Show comment
Hide comment
@jpakkane

jpakkane Dec 2, 2017

Member

Thinking about it further, maybe both library holder could have a method get_default_library_type or somesuch that would basically reduce to something like this:

get_option('default_library') == static ? b.get_static_library() : b.get_shared_library()
Member

jpakkane commented Dec 2, 2017

Thinking about it further, maybe both library holder could have a method get_default_library_type or somesuch that would basically reduce to something like this:

get_option('default_library') == static ? b.get_static_library() : b.get_shared_library()
@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Dec 3, 2017

Contributor

Maybe we could leave out the possibility of building both types with library? Having the return type change based on options is a bit spooky-action-at-a-distancy.

Having a different meaning depending on an option is the whole point of library(). Also the return type is not really different, it can be used exactly the same way than shared/static library. The extra get_static/shared_lib() methods should only be used when using both_library() explicitly.

Thinking about it further, maybe both library holder could have a method get_default_library_type or somesuch that would basically reduce to something like this:

get_option('default_library') == static ? b.get_static_library() : b.get_shared_library()

I'm not sure when that will be useful, since it already can be used as-is in link_with and it will be using the default. But it can easily be added indeed. get_default_lib() to match the naming of the 2 other methods.

Contributor

xclaesse commented Dec 3, 2017

Maybe we could leave out the possibility of building both types with library? Having the return type change based on options is a bit spooky-action-at-a-distancy.

Having a different meaning depending on an option is the whole point of library(). Also the return type is not really different, it can be used exactly the same way than shared/static library. The extra get_static/shared_lib() methods should only be used when using both_library() explicitly.

Thinking about it further, maybe both library holder could have a method get_default_library_type or somesuch that would basically reduce to something like this:

get_option('default_library') == static ? b.get_static_library() : b.get_shared_library()

I'm not sure when that will be useful, since it already can be used as-is in link_with and it will be using the default. But it can easily be added indeed. get_default_lib() to match the naming of the 2 other methods.

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Dec 3, 2017

Member

Thinking about it further, maybe both library holder could have a method get_default_library_type or somesuch that would basically reduce to something like this

Is this because you want to allow people to be explicit about what they are doing or something else? Because the held_object in BothLibraryHolder is the default library type.

Member

nirbheek commented Dec 3, 2017

Thinking about it further, maybe both library holder could have a method get_default_library_type or somesuch that would basically reduce to something like this

Is this because you want to allow people to be explicit about what they are doing or something else? Because the held_object in BothLibraryHolder is the default library type.

@jibsen

This comment has been minimized.

Show comment
Hide comment
@jibsen

jibsen Dec 5, 2017

Contributor

Now that we have array options, what if default_library were an array option instead of a combo, and you could do default_library : ['static', 'shared'] (and the library linked to by default is the one listed first)?

Contributor

jibsen commented Dec 5, 2017

Now that we have array options, what if default_library were an array option instead of a combo, and you could do default_library : ['static', 'shared'] (and the library linked to by default is the one listed first)?

@jibsen

This comment has been minimized.

Show comment
Hide comment
@jibsen

jibsen Dec 5, 2017

Contributor

On second thought, that would probably result in issues with checking the value since there is no way to tell if it is a string or an array.

Contributor

jibsen commented Dec 5, 2017

On second thought, that would probably result in issues with checking the value since there is no way to tell if it is a string or an array.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Dec 6, 2017

Contributor

Hmmm, CI is still failing and I have no clue why. Any MSVC expert here?

Contributor

xclaesse commented Dec 6, 2017

Hmmm, CI is still failing and I have no clue why. Any MSVC expert here?

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Dec 6, 2017

Member

CI is failing because you're not using __declspec(dllexport) on the function prototypes while building. Because of that, link.exe does not generate an import library at all since there are no exported symbols.

I'll add a commit to fix it.

Member

nirbheek commented Dec 6, 2017

CI is failing because you're not using __declspec(dllexport) on the function prototypes while building. Because of that, link.exe does not generate an import library at all since there are no exported symbols.

I'll add a commit to fix it.

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Dec 6, 2017

Member

So, while adding that I realized that we need a way to specify in a declare_dependency() what flags to use while linking to a library statically and what flags to use when linking dynamically. This is also needed for the current library() implementation that can be either static or shared.

The reason is that on Windows, for importing functions from DLLs you only need to specify __declspec(dllexport) while building, but for importing variables (constants), you always need to specify __declspec(dllimport) otherwise you will get a pointer to the variable instead of the variable itself. This importing is optional for functions.

This means that headers of such libraries support both dynamic and static linking by adding a define like GST_STATIC_COMPILATION, etc, that you must set while compiling with the headers used by that library.

We need something like:

libfoo_dep = declare_dependency(link_with: bothlib,
                                static_c_args: ['-DFOO_STATIC_COMPILATION'],
                                c_args: ['other', 'flags', 'here'])

The static args would be added after c_args so that you can do the reverse; f.ex. -DDLL_EXPORT in c_args: and -UDLL_EXPORT in static_c_args:.

Member

nirbheek commented Dec 6, 2017

So, while adding that I realized that we need a way to specify in a declare_dependency() what flags to use while linking to a library statically and what flags to use when linking dynamically. This is also needed for the current library() implementation that can be either static or shared.

The reason is that on Windows, for importing functions from DLLs you only need to specify __declspec(dllexport) while building, but for importing variables (constants), you always need to specify __declspec(dllimport) otherwise you will get a pointer to the variable instead of the variable itself. This importing is optional for functions.

This means that headers of such libraries support both dynamic and static linking by adding a define like GST_STATIC_COMPILATION, etc, that you must set while compiling with the headers used by that library.

We need something like:

libfoo_dep = declare_dependency(link_with: bothlib,
                                static_c_args: ['-DFOO_STATIC_COMPILATION'],
                                c_args: ['other', 'flags', 'here'])

The static args would be added after c_args so that you can do the reverse; f.ex. -DDLL_EXPORT in c_args: and -UDLL_EXPORT in static_c_args:.

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Dec 6, 2017

Member

Note: I do not think that this feature blocks this PR. It's an independent feature, but something to keep in mind.

Member

nirbheek commented Dec 6, 2017

Note: I do not think that this feature blocks this PR. It's an independent feature, but something to keep in mind.

@xclaesse

I'm not sure this is the right place to testing that, seems unrelated to the both_library() mechanism. What about adding that into "5 linkstatic" test instead ?

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Dec 6, 2017

Member

What changes do you mean? Github is not showing me the context.

Member

nirbheek commented Dec 6, 2017

What changes do you mean? Github is not showing me the context.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Dec 6, 2017

Contributor

your latest patch: "tests/common/173: Import constants from a library"

Contributor

xclaesse commented Dec 6, 2017

your latest patch: "tests/common/173: Import constants from a library"

@nirbheek

This comment has been minimized.

Show comment
Hide comment
@nirbheek

nirbheek Dec 6, 2017

Member

Well, it's a good example for how to handle that case with MSVC when using BothLibrary, which is why I added it.

Member

nirbheek commented Dec 6, 2017

Well, it's a good example for how to handle that case with MSVC when using BothLibrary, which is why I added it.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Jan 5, 2018

Contributor

There is something weird when testing this on glib. In glib master we just changed all shared_library() to library() so we can make static builds.

If I compile with this command, it works fine:
meson --default-library=static builddir && ninja -C builddir

This one works fine as well and produce both .a and .so files and .c files are built only once:
meson --default-library=both builddir && ninja -C builddir

I can even install then build a static app against libgio with that command:
gcc test.c -static $(pkg-config gio-2.0 --cflags --libs --static)

$ ldd ./a.out 
	not a dynamic executable

But this one fails to link glib's executables:
meson --default-library=both --default-link=static builddir && ninja -C builddir
meson-both-static.txt

It's weird because it's mostly _get_type() symbols missing, and glib doesn't use --version-script when building with meson.

Contributor

xclaesse commented Jan 5, 2018

There is something weird when testing this on glib. In glib master we just changed all shared_library() to library() so we can make static builds.

If I compile with this command, it works fine:
meson --default-library=static builddir && ninja -C builddir

This one works fine as well and produce both .a and .so files and .c files are built only once:
meson --default-library=both builddir && ninja -C builddir

I can even install then build a static app against libgio with that command:
gcc test.c -static $(pkg-config gio-2.0 --cflags --libs --static)

$ ldd ./a.out 
	not a dynamic executable

But this one fails to link glib's executables:
meson --default-library=both --default-link=static builddir && ninja -C builddir
meson-both-static.txt

It's weird because it's mostly _get_type() symbols missing, and glib doesn't use --version-script when building with meson.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Jan 5, 2018

Contributor

OOh, I think those symbols comes from generated sources. Could be related to PR #2717.

Contributor

xclaesse commented Jan 5, 2018

OOh, I think those symbols comes from generated sources. Could be related to PR #2717.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Jan 17, 2018

Contributor

ping?

Contributor

xclaesse commented Jan 17, 2018

ping?

@jpakkane

This comment has been minimized.

Show comment
Hide comment
@jpakkane

jpakkane Jan 17, 2018

Member

Using object files from one target on the other is a good optimization but unfortunately there is a catch (as there always is). That means the static library will get built with -fPIC even if b_staticpic is false.

Member

jpakkane commented Jan 17, 2018

Using object files from one target on the other is a good optimization but unfortunately there is a catch (as there always is). That means the static library will get built with -fPIC even if b_staticpic is false.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Jan 17, 2018

Contributor

b_staticpic is True by default, and it's up to the user to accept the catch. But I agree using default_library=both and b_staticpic=false should be incompatible. I guess func_both_lib() could raise an exception (or warn) if b_staticpic=false?

Also, just adding the catch in the doc seems enough, right?

Contributor

xclaesse commented Jan 17, 2018

b_staticpic is True by default, and it's up to the user to accept the catch. But I agree using default_library=both and b_staticpic=false should be incompatible. I guess func_both_lib() could raise an exception (or warn) if b_staticpic=false?

Also, just adding the catch in the doc seems enough, right?

@jpakkane

This comment has been minimized.

Show comment
Hide comment
@jpakkane

jpakkane Jan 17, 2018

Member

Or detect the case and then just build both libraries from source?

Member

jpakkane commented Jan 17, 2018

Or detect the case and then just build both libraries from source?

@jpakkane

This comment has been minimized.

Show comment
Hide comment
@jpakkane

jpakkane Feb 4, 2018

Member

Looks good, but it really should be both_libraries and not both_library.

Member

jpakkane commented Feb 4, 2018

Looks good, but it really should be both_libraries and not both_library.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Feb 4, 2018

Contributor

I called it both_library() to symetry with shared_library(), static_library() and library(). But indeed that could be a plurial. Added a commit to rename it. Thanks.

Contributor

xclaesse commented Feb 4, 2018

I called it both_library() to symetry with shared_library(), static_library() and library(). But indeed that could be a plurial. Added a commit to rename it. Thanks.

@tp-m

This comment has been minimized.

Show comment
Hide comment
@tp-m

tp-m Feb 5, 2018

Contributor

Would shared_and_static_library() be too verbose?

Contributor

tp-m commented Feb 5, 2018

Would shared_and_static_library() be too verbose?

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Feb 5, 2018

Contributor

Grammatically that would be a plural too. I don't think that function will be used much anyway, the main use-case is using library() and with --default-library=both argument. Personally I'm fine with both_libraries().

Contributor

xclaesse commented Feb 5, 2018

Grammatically that would be a plural too. I don't think that function will be used much anyway, the main use-case is using library() and with --default-library=both argument. Personally I'm fine with both_libraries().

@tp-m

This comment has been minimized.

Show comment
Hide comment
@tp-m

tp-m Feb 5, 2018

Contributor

I wasn't really commenting on the singular/plural aspect, shared_and_static just seemed more concise and more discoverable to me than both and I hadn't seen it suggested before yet :)

Contributor

tp-m commented Feb 5, 2018

I wasn't really commenting on the singular/plural aspect, shared_and_static just seemed more concise and more discoverable to me than both and I hadn't seen it suggested before yet :)

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 5, 2018

Member

Could do dual_library; not convinced that form needs to be plural, but also not sure that's the most descriptive either.

Member

QuLogic commented Feb 5, 2018

Could do dual_library; not convinced that form needs to be plural, but also not sure that's the most descriptive either.

@jibsen

This comment has been minimized.

Show comment
Hide comment
@jibsen

jibsen Feb 6, 2018

Contributor

Have you considered if modules somehow relate to this? Since you are using 'both' I was thinking what if a third type of library came along.

Contributor

jibsen commented Feb 6, 2018

Have you considered if modules somehow relate to this? Since you are using 'both' I was thinking what if a third type of library came along.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Feb 6, 2018

Contributor

The reason we have library() is to be able to switch all your libraries between shared or static build, but I don't think it makes sense to switch all your libraries into modules.

But it makes sense to build both a module and static library. For that case I planned to add an extra PR to add a module:true argument to library() which would be the same as using shared_module(), except that if setting default_library=both then it will build a module and static lib.

Contributor

xclaesse commented Feb 6, 2018

The reason we have library() is to be able to switch all your libraries between shared or static build, but I don't think it makes sense to switch all your libraries into modules.

But it makes sense to build both a module and static library. For that case I planned to add an extra PR to add a module:true argument to library() which would be the same as using shared_module(), except that if setting default_library=both then it will build a module and static lib.

@jpakkane

This comment has been minimized.

Show comment
Hide comment
@jpakkane

jpakkane Apr 2, 2018

Member

We should aim to minimize the number of new options whenever possible since having a gazillion of them is poor usability. Is the default link one really necessary? Could we not make it always match default library? When is the case where different values would be necessary?

Member

jpakkane commented Apr 2, 2018

We should aim to minimize the number of new options whenever possible since having a gazillion of them is poor usability. Is the default link one really necessary? Could we not make it always match default library? When is the case where different values would be necessary?

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Apr 2, 2018

Contributor

Most of the time you want default-library=both and default-link=shared. Also note that I'm using default-link in PR #2816. The general idea is when we have both shared and static libs available, meson needs to make a decision which one to use.

Contributor

xclaesse commented Apr 2, 2018

Most of the time you want default-library=both and default-link=shared. Also note that I'm using default-link in PR #2816. The general idea is when we have both shared and static libs available, meson needs to make a decision which one to use.

Xavier Claessens added some commits Jan 5, 2018

Xavier Claessens
pkgconfig generator: Only skip dependencies when using shared_library()
It is weird and inconsistent to have different pc file depending on
default_library value when using library() or build_target(). We should
skip dependencies only when user explicitly want shared library only.
Xavier Claessens
Add both_libraries() to build both shared and static libraries
Also support default_library='both' to make library() build both shared
and static libraries.

Closes #484
@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Apr 3, 2018

Contributor

After discussion on IRC, I decided to drop default-link commit because there is no consensus yet on how to select between shared and static libraries. For now both_libraries() will always return a buildtarget that represents the shared library. Users can manually choose which one to link with code like:

lib = library()
if get_option('default_library') == 'both'
  lib_dep = lib.get_static_lib()
else
  lib_dep = lib
endif

app = executable(..., dependencies : lib_dep)
Contributor

xclaesse commented Apr 3, 2018

After discussion on IRC, I decided to drop default-link commit because there is no consensus yet on how to select between shared and static libraries. For now both_libraries() will always return a buildtarget that represents the shared library. Users can manually choose which one to link with code like:

lib = library()
if get_option('default_library') == 'both'
  lib_dep = lib.get_static_lib()
else
  lib_dep = lib
endif

app = executable(..., dependencies : lib_dep)

@jpakkane jpakkane merged commit aef1a81 into mesonbuild:master Apr 4, 2018

3 checks passed

ci/sideci No issues left; 2 issues fixed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@xclaesse xclaesse deleted the xclaesse:both-library branch Apr 4, 2018

@jon-turney

This comment has been minimized.

Show comment
Hide comment
@jon-turney

jon-turney Apr 13, 2018

Contributor

The Cannot extract objects from a target that itself has extracted objects error this PR adds is reported when trying to build dpdk (See https://travis-ci.org/jon-turney/meson-corpus-test/jobs/366177420)

@xclaesse I assume that's a genuine instance of this error, but you might want to confirm that.

Contributor

jon-turney commented Apr 13, 2018

The Cannot extract objects from a target that itself has extracted objects error this PR adds is reported when trying to build dpdk (See https://travis-ci.org/jon-turney/meson-corpus-test/jobs/366177420)

@xclaesse I assume that's a genuine instance of this error, but you might want to confirm that.

@xclaesse

This comment has been minimized.

Show comment
Hide comment
@xclaesse

xclaesse Apr 13, 2018

Contributor

Thanks, it's already reported in issue #3401.

Contributor

xclaesse commented Apr 13, 2018

Thanks, it's already reported in issue #3401.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment