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

Add cmake files for clang/mac/darwin/arm64 #1234

Merged
merged 4 commits into from Feb 28, 2024

Conversation

barendgehrels
Copy link
Collaborator

I moved to a macbook and got several problems with Boost.Geometry unit tests, mainly:

  • some floating point results are different on mac
  • it can compile and run b2, but it does not detect unit tests

Therefore, and because it is probably useful anyway, I added CMakeLists files for the tests I'm using most often.

The compilation can be more compatible with the other platforms by using -ffp-contract=fast
This fixes most (but not all) of the floating point differences.

Please give your opinion about the CMake parts.

#if defined(TEST_WITH_SVG)
// Define before including any buffer headerfile
#define BOOST_GEOMETRY_BUFFER_USE_HELPER_POINTS
#endif
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unrelated small change, it's not used anymore)

@barendgehrels
Copy link
Collaborator Author

The tests in area, buffer and all set operations are all compatible with the b2/jam variants (in the sense of compiler directives, naming, and test coverage)

@pdimov
Copy link
Member

pdimov commented Jan 28, 2024

You shouldn't be adding the Boost root to the include path. When using CMake, we don't rely on the symbolic links created by b2 headers, but use the headers directly from libs/*/include. You need to target_link_libraries to all Boost::lib targets which you use in the tests (even when header-only) so that CMake adds the correct include paths.

You also need to make your test executables dependencies of the global tests target. Boost's testing procedure when using CMake, as documented here: https://github.com/boostorg/cmake#testing-boost-with-cmake, is

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

If your test executables aren't dependencies of tests, they won't be built, and the ctest invocation will fail.

This is accomplished with

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

and then

        add_dependencies(tests ${unit_test_name})

Your test targets need to be prefixed with your project name, which is boost_geometry. In CMake, target names are global. If your library adds a test called something and another library adds a test called something, there will be a conflict.

Consider adding CMake tests to your Github Actions (or other) CI. For an example, you can look at one of my libraries, e.g. Function: https://github.com/boostorg/function/blob/develop/.github/workflows/ci.yml

@barendgehrels
Copy link
Collaborator Author

You shouldn't be adding the Boost root to the include path. When using CMake, we don't rely on the symbolic links created by b2 headers, but use the headers directly from libs/*/include. You need to target_link_libraries to all Boost::lib targets which you use in the tests (even when header-only) so that CMake adds the correct include paths.

You also need to make your test executables dependencies of the global tests target. Boost's testing procedure when using CMake, as documented here: https://github.com/boostorg/cmake#testing-boost-with-cmake, is

Thanks for your comments and links!

@barendgehrels
Copy link
Collaborator Author

Hi @pdimov I updated it. I'm following the approach you did in outcome.

  • boost_geometry_ prefixes the unit test targets
  • dependencies to tests added
  • dependencies to the boost libraries added

Because we had already too many dependencies (many were unused (extensions), optional (adaptations), I split those lists and cleaned it up. I hope that's OK for the super project as well.

Some dependencies should never have been there, such as predef and endian.

As a background: I didn't add these CMakeLists for the general purpose. Mainly because b2 didn't work anymore on my Mac, and CMake is more convenient anyway. So it's mainly for my own purpose and I'm quite dependent on it. But anyway, maybe it serves broader purpose, now that it is more generic.

(It's not complete though)

@pdimov
Copy link
Member

pdimov commented Jan 30, 2024

This looks largely correct to me.

Optional dependencies are a hassle and I don't think you'll gain that much from removing them because many will still be there as transitive dependencies. But you could do it this way if you prefer.

You are linking the tests to dependencies of Boost::geometry explicitly, which shouldn't be necessary because they'll be there as transitive dependencies.

I assume you are using the unit test framework in header-only mode? The target to link to is Boost::included_unit_test_framework in that case.

@barendgehrels
Copy link
Collaborator Author

Thanks again @pdimov
Indeed, I now removed explicit dependencies in the unit tests themselves and added Boost::included_unit_test_framework, that is indeed what we use and it works elegantly like this in cmake.

This is in latest commit, and I added some folders which are also related to overlay algorithms.

@barendgehrels
Copy link
Collaborator Author

@vissarion I now removed the alternatives, because you had them removed as well in your PR.
Is this fine now?

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I am OK with changes. Also managed to build and run without errors on Ubuntu 22.04 with gcc version 12.2.0

@barendgehrels
Copy link
Collaborator Author

Thanks for approving, this is very convenient (at least for me).
Later I'll add the still missing tests (there are quite some), but the current coverage is what I'm mostly working on.

@barendgehrels barendgehrels merged commit 682994c into boostorg:develop Feb 28, 2024
22 of 23 checks passed
@vissarion vissarion added this to the 1.85 milestone Apr 1, 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 this pull request may close these issues.

None yet

3 participants