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

Improve deployment as subdirectory #56

Merged
merged 3 commits into from
Nov 3, 2020
Merged

Conversation

fmoessbauer
Copy link
Contributor

I'm really sorry, that my last PR broke the subdirectory integration support.
For that, we have to include the generated header in the public "build interface" as well... Doing CMake right is ridiculously hard.

As the this issue now happened the second time, I added a test to check the deployment as well:

  • as a subdirectory
  • using the install target

I'm still a bit unsure where to place these scripts. Maybe we should unify that, once you are done with reworking the testing anyways.

@offa offa self-assigned this Nov 2, 2020
@offa offa added the cmake label Nov 2, 2020
@offa offa added this to the next milestone Nov 2, 2020
@offa
Copy link
Owner

offa commented Nov 2, 2020

I'm really sorry, that my last PR broke the subdirectory integration support.
For that, we have to include the generated header in the public "build interface" as well... Doing CMake right is ridiculously hard.

No Problem. A general CMake question: Is it possible to map $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src> through a target? So eg. internal parts can consume it using target_link_libraries()?

As the this issue now happened the second time, I added a test to check the deployment as well:

as a subdirectory
using the install target

Adding a test is a good idea! 👍 It's hard to notice otherwise. But instead of executing those step in each compiler build, it's probably a good idea to add a dedicated "deployment" build. Or some more if necessary – eg. subdirectory deployment and make install separated if that makes sense. Those build can skip the tests and stick to a single compiler.

So we keep jobs doing only one task at a time and the deployment tests don't have to forcefully fit into the build & test cycle. Furthermore it becomes easy to spot whether a build failure is deployment or build related.

I'm still a bit unsure where to place these scripts. Maybe we should unify that, once you are done with reworking the testing anyways.

Agreed. For now I'd say the scripts/ directory or alternatively a test_package subdirectory. We can move it later anyway 😄.

CMakeLists.txt Outdated Show resolved Hide resolved
script/include_library/main.cpp Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@offa offa mentioned this pull request Nov 2, 2020
16 tasks
@fmoessbauer
Copy link
Contributor Author

No Problem. A general CMake question: Is it possible to map $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src> through a target? So eg. internal parts can consume it using target_link_libraries()?

I don't think that is possible. If you export something as PUBLIC, all transitive targets inherit this. I thought about that as well for making an "internal" lib / object lib for testing.

Adding a test is a good idea! It's hard to notice otherwise. But instead of executing those step in each compiler build, it's probably a good idea to add a dedicated "deployment" build. Or some more if necessary – eg. subdirectory deployment and make install separated if that makes sense. Those build can skip the tests and stick to a single compiler.

So we keep jobs doing only one task at a time and the deployment tests don't have to forcefully fit into the build & test cycle. Furthermore it becomes easy to spot whether a build failure is deployment or build related.

As its relatively cheap I would keep it this way. By that, we also test the various install options w/ w/o boost. And another problem is that we have to test the subdir option before we install the target, as otherwise we cannot be sure that the the headers are actually taken from the subdir and not from the system include.

Anyways, feel free to change the CI stuff ;)

@offa offa merged commit 8b5c4ad into offa:master Nov 3, 2020
@offa
Copy link
Owner

offa commented Nov 3, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants