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

Refactor CMake builds #159

Open
wants to merge 3 commits into
base: shared_build_test
Choose a base branch
from
Open

Conversation

Flamefire
Copy link
Collaborator

In preparation to fix #155 this creates scripts for the CMake tests/builds. In those we can then fix the issue and in the future add further improvements, like using CCache.

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #159 (17f6483) into shared_build_test (2c9a822) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           shared_build_test      #159   +/-   ##
===================================================
  Coverage             100.00%   100.00%           
===================================================
  Files                      2         2           
  Lines                     18        18           
  Branches                   7         7           
===================================================
  Hits                      18        18           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c9a822...17f6483. Read the comment docs.

@Flamefire Flamefire force-pushed the shared_build_refactor branch 5 times, most recently from d4af331 to f9b53fc Compare April 28, 2022 13:58
Convert the header-only library to a compiled library which additionally depends (privately) on another compiled library (Boost.Atomic chosen as it has few other dependencies).
This reproduces the runtime link failure of #155 when compiling shared libraries.
@Flamefire Flamefire force-pushed the shared_build_test branch from 033084a to 2c9a822 Compare May 2, 2022 14:49
Flamefire added 2 commits May 2, 2022 16:52
Allows reuse in other scripts
Allow reuse and a centralized place for improvements
@Flamefire Flamefire force-pushed the shared_build_refactor branch from f9b53fc to 17f6483 Compare May 2, 2022 14:52
@Flamefire
Copy link
Collaborator Author

@pdimov In the next step I'd add setting LD_LIBRARY_PATH / PATH to ci/cmake_install_test.sh and CMAKE_RUNTIME_OUTPUT_DIRECTORY to the subdir test, as done in Flamefire/locale@2e722c0

I think using those scripts (instead of putting it all in the CI file) is better, as it allows reusing the code used in "Run CMake tests" step for installing Boost. Maybe we can even avoid to build it twice (once for the test, once for install) by reusing the build folder.
It also allows us to add fixes (like this) for all users of those instead of requiring them to update their CI files.

I initially had them as we need to know the install folder in the step after Boost is installed. I also found that using the home folder doesn't really work, as the MinGW translation of paths on Windows breaks setting the PATH variable properly (need C:/ as the prefix in PATH, not /c/ but $HOME uses /c/)
So packing it all in 1 script seemed to be more useful.

Any comments?

@pdimov
Copy link
Member

pdimov commented May 2, 2022

I prefer to test and install separately because the former uses -DBUILD_TESTING=ON and the latter does not.

@Flamefire
Copy link
Collaborator Author

I'm using an env variable BCM_ARGS (for "Boost CMake") to add e.g. BUILD_TESTING=ON, see https://github.com/boostorg/boost-ci/pull/159/files#diff-acad16320afbb68d0981f51534d40a2722e63386916826c48154af0206f429f0

Similarly I add the CMAKE_INSTALL_PREFIX in the install test file: https://github.com/boostorg/boost-ci/pull/159/files#diff-3d84f614fee94218472ec19493340cd330984ad316d6e46f345109fb99b7ae9e

So the cmake_build.sh is similar to the build.sh but it uses CMake instead of B2. It encapsulates the common stuff and provides knobs to change what is required

@Flamefire
Copy link
Collaborator Author

I would like to come to a solution which will be used in all Boost CI tests, i.e. by all maintainers to make updating easier and to benefit the most. These are the things I want to tackle:

  • Works-by-default solution for shared/dynamic CMake builds which needs setting of environment variables (PATH/LD_LIBRARY_PATH) or a CMake option (CMAKE_RUNTIME_OUTPUT_DIRECTORY)
  • Faster CI builds by using parallel CMake builds (i.e. the -j option similar to b2)
  • Faster CI builds by using ccache in CMake builds likely even reusing the same cache as for B2 (compiler and env etc. is likely the same)
  • Optional: Unify the CMake test and CMake install test procedure. I.e. make sure we install (and later test) what we tested before. This is motivated by the generalized workflow for installing/maintaining source-built-software: configure-make-test-install
    We can do it by "just" reusing the code and replace the BUILD_TESTING=ON by CMAKE_INSTALL_PREFIX=... in the 2nd test or even reuse the build tree to reduce CI time. Either by setting both variables and only installing after the test or by reconfiguring the already populated and built build tree with BUILD_TESTING=OFF and CMAKE_INSTALL_PREFIX=...

So major question: Do we want to use reusable, centralized scripts which we can update with features/fixes as done here or have the code in the CI yaml-files as before which duplicates some code and needs every maintainers interaction to fix/enhance things but is more flexible in case maintainers want other/more CMake options (could also be done with env vars, similar to build.sh) or only some parts of the tests or ... E.g. the fix for the shared-lib CMake build would then look like Flamefire/locale@2e722c0 Note how we e.g. need the install path in the test step and we can use the ${{matrix.foo}} syntax instead of needing to persist them in the install step (or passing them in in all steps)

Querying @pdimov @jeking3 as you have introduced CMake CI builds in some Boost libs.

@Flamefire Flamefire force-pushed the shared_build_test branch from 2c9a822 to 7e8379a Compare May 21, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants