marzer / tomlplusplus

Header-only TOML config file parser and serializer for C++17.
https://marzer.github.io/tomlplusplus/
MIT License
1.47k stars 141 forks source link

Shared lib not being installed? #201

Closed vlad0x00 closed 10 months ago

vlad0x00 commented 10 months ago

Environment

toml++ version and/or commit hash: dbc4bcecf20b7c45fe9d39ca2aa4b28c35e4e83a

Compiler: N/A

C++ standard mode: N/A

Target arch: N/A

Library configuration overrides: DTOML_HEADER_ONLY=0

Relevant compilation flags: N/A

Describe the bug

I've included tomlplusplus as a meson subproject using WrapDB as described in README: meson wrap install tomlplusplus. By default, it builds as a shared library but the built shared library is not installed and so I can't link with it during run time. The issue seems to be in tomlplusplus/src/meson.build:

tomlplusplus_lib = library(
    meson.project_name(),
    files('toml.cpp'),
    cpp_args: lib_internal_args,
    gnu_symbol_visibility: 'hidden',
    include_directories: include_dir,
    install: not is_subproject,
    version: meson.project_version(),
    override_options: global_overrides
)

Installation is disabled if tomlplusplus is a subproject: install: not is_subproject This is not an issue if I build tomlplusplus as a static library, but this line gnu_symbol_visibility: 'hidden' is forcing my main project to also build with hidden symbols, otherwise the compiler complains. Perhaps building my project with hidden symbols is not a bad idea, but I'm not sure if forcing that on the user is ideal.

Steps to reproduce (or a small repro code sample)

Additional information

marzer commented 10 months ago

Ah, so it looks like we have two separate things for me to address here:

1. gnu_symbol_visibility: 'hidden'

I did not realize meson applies that even if you choose to build as a static lib. That's... not particularly useful? I suppose there are scenarios where you are building one shared lib from separate compilations of a bunch of static libs, and having those static libs be part of the final library API, though that seems niche, heh. I can make that setting conditional

2. install: not is_subproject

The rationale here was that if you bring toml++ in as a subproject you may not want your project to clobber an existing system install of it against the local user's wishes, if they had a different or customized version of it as their system-wide build. I can change that, though it just means that you should probably check for a local install of toml++ in your project's meson.build before falling back to the subproject.

aside: DTOML_HEADER_ONLY=0

If you're using the library as a meson subproject you shouldn't need to explicitly set this anywhere; it will be done for you when you include the toml++ dependency: https://github.com/marzer/tomlplusplus/blob/dbc4bcecf20b7c45fe9d39ca2aa4b28c35e4e83a/src/meson.build#L10

marzer commented 10 months ago

Ok, after reading this thread, it seems my intuition was correct; I'll leave the 'not is_subproject' as-is, but fix the gnu_symbol_visibility behaviour when using the library as static, sound good?

vlad0x00 commented 10 months ago

That sounds reasonable to me. Given that symbol visibility should be consistent across the project and subprojects, it sounds sensible that the project should decide what the visibility should be which they can control via, e.g. add_global_arguments('-fvisibility=hidden', ...). Up to you, but one alternative option that might be useful is adding a visibility option to meson_options.txt since I think it's not possible to control a subproject's symbol visibility via options in the subproject declaration.

Also, thanks for making this great library!

marzer commented 10 months ago

Given that symbol visibility should be consistent across the project and subprojects

Only for static libs, though. I believe I can safely leave this enabled when the library is built in shared mode, and have no impact on any parent project or subproject. For static libs there's almost no reason you'd set the symbol visibility to hidden anyways, so yeah I think the path of least resistance is to be more explicit about applying this to shared-only and expose some sort of configurable if it becomes an issue for someone in static mode (though you're the first to report that the symbols were hidden in static mode so I suspect it won't 😅)

Also, thanks for making this great library!

🙇🏼 glad you like it :)

marzer commented 10 months ago

@vlad0x00 I've pushed a fix for the gnu_symbol_visibility issue, though it won't be propagated via the wrapfile on the wrapdb until I make a new release and someone updates it. In the mean-time you can replace your wrapfile with this:

[wrap-git]
url = https://github.com/marzer/tomlplusplus.git
revision = 882d9d1c34077d733559ec55d31c67799002239f
depth = 1
clone-recursive = false

[provide]
tomlplusplus  = tomlplusplus_dep
vlad0x00 commented 10 months ago

Woo! Thanks