Add support for CMake install to phoenix library

[DaDummy]

This PR adds support for installing the phoenix target into an install prefix using the cmake --install command.

Additionally building tests is made optional and the version number is updated as sanity-fixes.

[lmichaelis]

Hi there, thanks for the PR! I'll check this out within the next couple days once I have some time on my hands :)

Looks good at first glance though!

[lmichaelis]

I've tested your changes like this:

git clone https://github.com/DaDummy/phoenix
cmake -B build -DCMAKE_BUILD_TYPE=Release
cmake --build build
cmake --install build

It works just fine but it seems to mess up which libraries to install since it also installs some dependencies of phoenix:

I don't know much about how this is supposed to work (it seems to be fairly complicated anyways) but that behavior looks wrong to me, especially since it only installs some of the dependencies (fmtlib is missing for example). Can you confirm that it's supposed to be like that and/or if I'm just misunderstanding the --install command?

[DaDummy]

This is because they are declared as PUBLIC dependencies here: https://github.com/lmichaelis/phoenix/blob/8f4b3937a38edfb934c724c2a0699e05535504a3/CMakeLists.txt#L109

The reason only some get installed might come from only some of them having the install-step properly configured.

I wasn't sure what the correct behaviour for phoenix should be and opted to not changing the remainder of the script for now.

If you want I can switch some or all dependencies to PRIVATE if they export no symbols that are needed by a consumer of this library :)

_Edit: Here's the relevant bit in the documentation: https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-a-target-and-or-its-dependents_

[lmichaelis]

Hm setting them to PRIVATE might work for now but if I understand this correctly, it would not work if I wanted to distribute a shared instead of a static library. Regardless, these additions seem like a good thing for now.

I should probably put some more work into ensuring that phoenix can be distributed more easily though (see #47) :)

So if this works for you, I'll go for the merge!

[DaDummy]

Regarding public vs private dependencies:

If a the user of this library needs the dependency to properly use phoenix, it is a public dependency. This applies at least to libfmt as far as I can tell as it is referenced by phoenix headers.

Having just realized this I will continue making install steps available for additional vendored dependencies until the result of the install step works for the project I am currently working on.

Please don't merge this PR yet, I'll report back once I am done with the adjustments and am happy with the results :)

[lmichaelis]

Of course :) The libraries used in public headers are glm and fmt.

It should be possible to get rid of fmt in the public headers though since its only include is in include/phoenix.hh in which it is used for a macro that's only supposed to be used internally anyways. We could conditionally include it (and define the macros) only when building phoenix.

glm is a different case because it is used in phoenix' public interface.

[DaDummy]

Looks like I overestimated what PUBLIC in target_link_libraries does.

Anyhow I have now refactored this PR to only install phoenix plus glm and fmt includes.

The magic sauce is adding EXCLUDE_FROM_ALL to add_subdirectory

Though build now appears to fail due to too low C++ standard. Is this a regression I unknowingly introduced?

[lmichaelis]

Hm apparently no: Default bitfield inizializers are in fact C++20 only: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0683r1.html

That's a mistake I made. I've fixed it in 93392e91. Bumping the CXX_STANDARD won't help here since the docker images I am using only ship C++17 (on purpose).

[DaDummy]

I see. I have reverted my fix attempts and rebased to said commit.

Sadly there appears to be more breakage that is not reported for main.

Looks like tests also depend on lexy. Is that specific to the tests or should I redeclare it as a public dependency?

[lmichaelis]

Sadly there appears to be more breakage that is not reported for main.

That is very interesting indeed. It might have resulted from some sort of build flag pollution coming from the dependencies. I'll fix that later.

Looks like tests also depend on lexy. Is that specific to the tests or should I redeclare it as a public dependency?

The tests shouldn't need lexy since its only used within phoenix' private API in model_script_dsl.hh which is not included anywhere in the tests. Neither can I find an include mentioning lexy in the test sources.

[DaDummy]

I see. I've added mio aswell as that appears to help for *nix platforms.

This way I can limit the scope of this PR to just install + public/private adjustments, leaving the build flag management for follow-up work.

[DaDummy]

The current setup now produces an install folder that allows successfully building our dependency project.

I understand that you'd prefer not to install any of the vendored libraries. Yet since with static linkage the dependencies need to be linked to aswell and since the dependencies are vendored I believe this is currently the optimal setup.

To avoid installing dependencies they'd need to be unvendored and instead be required to be already installed on the target system (and used from there for building)

TL;DR: From my point of view this PR is now ready for merge.

[DaDummy]

Ah nope. I am not done, yet. squish also needs to be provided :/

[DaDummy]

Project links successfully now :)

[lmichaelis]

Okay, looks good now. Thank you very much for looking into this! I would have pushed this out for a long time otherwise :)