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

Boost CMake testing procedure doesn't work for StaticString #53

Closed
pdimov opened this issue Dec 17, 2023 · 3 comments · Fixed by #54
Closed

Boost CMake testing procedure doesn't work for StaticString #53

pdimov opened this issue Dec 17, 2023 · 3 comments · Fixed by #54

Comments

@pdimov
Copy link
Member

pdimov commented Dec 17, 2023

The documented testing procedure

mkdir __build && cd __build
cmake -DBUILD_TESTING=ON -DBOOST_INCLUDE_LIBRARIES=static_string ..
cmake --build . --target tests
ctest --output-on-failure --no-tests=error

fails for Boost.StaticString, because the test executables aren't built by the target tests. BoostTest handles this automatically, but for tests declared manually, one needs to first declare the tests target if not present

if(NOT TARGET tests)
    add_custom_target(tests)
endif()

and then for each test executable, use add_dependencies(test my_test_executable).

Or, since there is already a target boost_static_string_all_tests, it should be enough to make it a dependency of tests: add_dependencies(tests boost_static_string_all_tests).

(For this to keep working after the eventual fix, it might be a good idea to add it to CI.)

It's also good practice to make the test executables EXCLUDE_FOR_ALL, so that building the tests target builds them, but building the default target doesn't, but this step is not in principle required.

alandefreitas added a commit to alandefreitas/static_string that referenced this issue Dec 22, 2023
@pdimov
Copy link
Member Author

pdimov commented Feb 16, 2024

Is this going to be fixed? It breaks the tests of Mysql.

@sdkrystian
Copy link
Member

@pdimov Looking into it

@alandefreitas
Copy link
Member

Probably this one fixes it: #54

alandefreitas added a commit that referenced this issue Feb 20, 2024
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 a pull request may close this issue.

3 participants