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

Further clean and fix the CMLs #135

Closed
wants to merge 5 commits into from
Closed

Conversation

Flamefire
Copy link

Don't add C++ requiredment in test CML (added in root CML)

option does nothing when the variable is already set.

Also remove some redundancies (especially checks for BOOST_COBALT_IS_ROOT and BOOST_SUPERPROJECT_VERSION inside the "BOOST_COBALT_IS_ROOT"-branch)

Happy to help if any questions.

`option` does nothing when the variable is already set.

Also remove some redundancies (especially checks for BOOST_COBALT_IS_ROOT and BOOST_SUPERPROJECT_VERSION inside the "BOOST_COBALT_IS_ROOT"-branch)
@klemens-morgenstern
Copy link
Collaborator

I think this is outdated now, what part is still relevant?

@Flamefire
Copy link
Author

I resolved the conflicts. Please check if that looks ok

@klemens-morgenstern
Copy link
Collaborator

Why are you removing target_compile_features(boost_cobalt PUBLIC cxx_std_20) ?

@Flamefire
Copy link
Author

Flamefire commented Nov 9, 2023

I'm only removing the duplicated line which is (erroneously) in the test-CML. The actual correct one is still at

target_compile_features(boost_cobalt PUBLIC cxx_std_20)

Note on "erroneously": This is a requirement of the library so it needs to be set for the library by the library not by a user even though the "user" here is the tests of the library.

@klemens-morgenstern
Copy link
Collaborator

I removed the duplicate in test, is it ok now?

@Flamefire
Copy link
Author

I removed the duplicate in test, is it ok now?

Synced and updated to see what is left:

  • Unconditional dependency on Boost.Container (reformatting the other line allows the CMake scanner to work)
  • Superflous include_directories: This should be covered by target_include_directories(boost_cobalt PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/include") unless you are missing a dependency on boost_cobalt

@Flamefire Flamefire deleted the patch-1 branch May 31, 2024 14:23
@Flamefire
Copy link
Author

The change at https://github.com/boostorg/cobalt/pull/135/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL76-R78 was important because otherwise the dependency won't be detected by the dependency scanner in the super project build which requires it on its own line, see https://github.com/boostorg/cmake/blob/440dd10f57f86a91c9d0ae23f096f343fdd004cb/include/BoostRoot.cmake#L210

It seems to work though, I guess some dependency pulls it in transitively

klemens-morgenstern added a commit that referenced this pull request Jul 2, 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

2 participants