build(meson): add compile_library option

[Tachi107]

Still a lot of stuff to do.

#140 (comment)

[marzer]

Do you want me to offer feedback as you work on the draft, or just wait until you mark it as ready?

[Tachi107]

Feedback is always welcome, and now should be almost ready (I only need to define TOML_API)
Edit: and provide a meaningful commit message

[Tachi107]

Changes so far:

• Added two meson.build files, one used when using toml++ as an header-only lib, one when using it as a compiled library. This is just for readability, because otherwise the main meson.build file would've got too large
• Renamed the CMake config files used by meson by appending .meson before .in, since the cmake/ dir contains files shared by the two build systems, with similar names, and it is now easier to recognize who uses what
• created a toml.cpp file in a lib directory that is used to compile the implementation of the library
• Improved the tomlplusplusConfig.cmake.meson.in file to handle the compiled library

[Tachi107]

Now failing due to a warning related to type attributes being in the wrong place, but I'll be really happy to hear some feedback :)

[marzer]

There we go, just a few nitpicks.

Something else that occurs to me; is this also adding support for compiling as static library? If not, it probably should, since adding shared-lib support is going to open me up to feature requests along the lines of "shared, but no static option??? plz???". Though, maybe that's better left to a later batch of work.

[Tachi107]

Something else that occurs to me; is this also adding support for compiling as static library?

Yep

[marzer]

Hmmn, I see there's some issues with TOML_API + extern templates. I'm not sure GNU visibility works with extern templates like dllexport does on windows - you might have to disable the the extern template definitions for GNU compilers. Fortunately there's an internal escape hatch for this already: #define TOML_EXTERN_TEMPLATES 0

[Tachi107]

Now there are only two main issues to fix. The first one is the use of TOML_API in some templated classes, that GCC warns about:

tomlplusplus/include/toml++/impl/value_extern.inl

Lines 11 to 24 in 7b61012

	TOML_EXTERN
	template class TOML_API value<std::string>;
	TOML_EXTERN
	template class TOML_API value<int64_t>;
	TOML_EXTERN
	template class TOML_API value<double>;
	TOML_EXTERN
	template class TOML_API value<bool>;
	TOML_EXTERN
	template class TOML_API value<date>;
	TOML_EXTERN
	template class TOML_API value<time>;
	TOML_EXTERN
	template class TOML_API value<date_time>;

GCC says: "type attributes ignored after type is already defined"

The other one is related to a message sent by ci_single_header_check.py

Potentially missing #undefs:
	#undef TOML_SHARED_LIB
	#undef TOML_SHARED_LIB_BUILDING

Inspecting generate_single_header.py, I see that only "macros that are meant to stay public" should still be defined after the build.

TOML_SHARED_LIB is public; do I have to add it to that ignore_list?

TOML_SHARED_LIB_BUILDING is instead "private", but I would say that it less than private; it has to be defined only while building the shared library, and should never be set by the source files as they have no way of knowing if they are in the middle of the compilation of a shared library or not (kinda like _WIN32, it gets automatically defined based on the compiling environment). How should I handle it? Removing its definition from preprocessor.h would solve the issue, but it seems that you have different preferences so I don't know what to do :)

[Tachi107]

Hmmn, I see there's some issues with TOML_API + extern templates. I'm not sure GNU visibility works with extern templates like dllexport does on windows - you might have to disable the the extern template definitions for GNU compilers. Fortunately there's an internal escape hatch for this already: #define TOML_EXTERN_TEMPLATES 0

Oh, I didn't noticed that you wrote this as I was writing #142 (comment) at the same time :/

[marzer]

Yeah, for our purposes they're both 'public' w.r.t. that script. I'm happy for them to go in the ignore list.

and should never be set by the source files as they have no way of knowing if they are in the middle of the compilation of a shared library or not

It being set by a global header in the library itself is totally fine in the 'default' case. It's essentially an undocumented config option. I don't mind if it leaks.

...having said that, you also make a reasonable point. Maybe TOML_SHARED_LIB_BUILDING should use #ifdef after all. I would still like TOML_SHARED_LIB to remain a boolean, though, as that better indicates the intent - "are you being used as a shared lib?" "yes/no"

[Tachi107]

It being set by a global header in the library itself is totally fine in the 'default' case. It's essentially an undocumented config option. I don't mind if it leaks.

...having said that, you also make a reasonable point. Maybe TOML_SHARED_LIB_BUILDING should use #ifdef after all. I would still like TOML_SHARED_LIB to remain a boolean, though, as that better indicates the intent - "are you being used as a shared lib?" "yes/no"

I mean, for this simple case I believe that an #ifdef is just enough and doesn't complicate things, and doesn't expose a macro defined only because shared libraries work in an extraordinarily strange way on Windows... It's not only an implementation detail, it is an implementation detail of a particular platform/process that has little to do with this library in particular, and has no meaning outside of the compilation phase.

Please remove TOML_EXTERN_TEMPLATES here. It was never necessary for the test+example applications before, so it shouldn't be now.

I don't really understand what that define does... But now something is necessary to tell GCC to ignore the TOML_API macro when applied to extern variables. The issue was there before this PR, but TOML_API wasn't defined to anything when using GCC so nothing triggered it. If users were to define TOML_API themselves when using GCC they would've encountered the same issue (the one I described in #142 (comment))

[Tachi107]

We still have one issue left with symbol visibility: when linking against the compiled libraries, a few symbols are missing. Here's the output of LLD:

ld.lld: error: undefined symbol: vtable for toml::v3::table
>>> referenced by main.cpp
>>>               CMakeFiles/linkami.dir/main.cpp.o:(toml::v3::table::table())
>>> referenced by main.cpp
>>>               CMakeFiles/linkami.dir/main.cpp.o:(toml::v3::table::~table())
>>> the vtable symbol may be undefined because the class is missing its key function (see https://lld.llvm.org/missingkeyfunction)

ld.lld: error: undefined symbol: vtable for toml::v3::node
>>> referenced by main.cpp
>>>               CMakeFiles/linkami.dir/main.cpp.o:(toml::v3::node::node())
>>> the vtable symbol may be undefined because the class is missing its key function (see https://lld.llvm.org/missingkeyfunction)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

And the one from gold:

CMakeFiles/linkami.dir/main.cpp.o:main.cpp:function toml::v3::node::node(): error: undefined reference to 'vtable for toml::v3::node'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function
CMakeFiles/linkami.dir/main.cpp.o:main.cpp:function toml::v3::table::table(): error: undefined reference to 'vtable for toml::v3::table'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function
CMakeFiles/linkami.dir/main.cpp.o:main.cpp:function toml::v3::table::~table(): error: undefined reference to 'vtable for toml::v3::table'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function
collect2: error: ld returned 1 exit status

[Tachi107]

Linking issues do not occur when setting the gnu_symbol_visibility to default (i.e. everything public), so this is almost surely because TOML_API is missing somewhere

[marzer]

I think just unconditionally outlining the destructor is the 'standard' way to fix this, but I'm no longer at a PC to test. Happy to investigate tomorrow.

[Tachi107]

Ok, I'll give it a shot myself. Have a good night!

[marzer]

Fyi: circle ci didn't run your new job because you didn't add it to the workflow bit at the bottom of the script

(Good idea to add the lib target to the tests though)

[marzer]

Relevant docs: https://circleci.com/docs/2.0/workflows/

[marzer]

Sorry @Tachi107 I'm not going to get a chance to help with the linker thing today after all, won't be able to until Sunday or Monday at the earliest.

[Tachi107]

I think this is almost ready. If you're ok with the changes I could edit the commit history and keep only a few meaningful commits (like "depreate TOML_API", "add compile_library option", "switch to GitHub Actions", etc).

Normally I would squash everything in one single commit, but since this PR is so huge I believe that having four or five different commits in the master branch would be better.

[marzer]

Yeah, sure, that sounds like a good idea.

[Tachi107]

@marzer mostly done (I'm pinging you just in case you're not subscribed to this thread).

I only need to move the generate_dox and deploy_dox jobs to GitHub Actions, but I don't fully understand how they work, or better, what they do.

If what they need to do is to build the html files and deploy them to GitHub Pages then I could add a job similar to the one I added in pistacheio/pistache/.github/workflows/pistache.io.yaml

Edit: done, I believe it should work fine.

[Tachi107]

I've also updated the badge in the Readme to point to the GitHub CI job, and now I really think that this is complete. When merging, be careful not to squash the commits :)

And now the Windows job gives issues when it is triggered by a PR... I'll see how to fix it.

[Tachi107]

Ok, I've fixed that too, and now this should be ready for real :D

[marzer]

Thanks for all your work on this!