-
Notifications
You must be signed in to change notification settings - Fork 36
Don't add all libraries when BOOST_INCLUDE_LIBRARIES is set #86
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
Conversation
I don't think that we should impose the "one library per line" requirement on all test/CML files. We allow more freedom there. |
I enhanced the scanner to allow multiple Boost libs per line. That also fixes #17 |
Looks like we need to do a bit of special casing. |
MS Copilot seems "pleased". Here’s a review of PR #86: "Don't add all libraries when BOOST_INCLUDE_LIBRARIES is set". SummaryThis PR significantly improves how Boost's CMake superproject handles the
Key Changes
Impact
Suggestions & Comments
Minor suggestions:
RecommendationApprove – This is a solid improvement to the Boost CMake superproject’s partial build handling, with clear benefits for users and maintainers. |
Done. I also added a CI job that includes all libraries one-by-one and configures them only. This should ensure that we catch all such issues. |
2fbd2cc
to
df90185
Compare
but the CI job succeeds. |
There's also
but the Asio CMake is now broken (installation doesn't work) so this asio_core target may disappear. Note that the suggested fix for the Asio issue will be broken by this patch. |
|
|
There are a few more. |
df90185
to
b3c1215
Compare
Looks good to me. Works for cmake-3.8 in boostorg/json#1106 |
I'm not convinced that the new scanner will not cause us issues, as it will pick up a lot more Boost::... things than the current one. What's the new logic? |
The MSM CI looks rusty. Maybe I need to update it. |
Previously all libraries were added even though only a subset was selected. This makes the configure process much longer then required or may fail for libraries that aren't intended to be build. Instead gather a 2nd list of dependencies determined by the test/CMakeLists.txt when BUILD_TESTING is ON and add only those (with disabled install and tests)
Don't check the test/example folders for (implicit) dependencies as BUILD_TESTING for those is disabled.
b3c1215
to
7ab2d8d
Compare
I think that should still be pretty safe as libraries picked up by the scanner get excluded from ALL and with
Base case: Pick up anything that looks like a Boost library, i.e. Determine 2 sets of dependencies:
The former are handled as before and the latter limits the libraries added when So we could rather not add enough libraries if the logic for the 2nd list was wrong, which is why I enhanced the CI. That found e.g. tests in multiple subfolders or example-folders |
But we're modifying the scanner for the non-testing case as well, don't we? |
We'll need to "fix it here" for every such occurrence. It's much better if people can "fix it there" without having to rely on the central infrastructure each time. Note that asio_core was introduced two months ago. |
And nobody noticed
But we need to fix it here either way as the parser would/could have trouble too: https://github.com/boostorg/asio/blob/be4af2d923f6145cd384e9bd1d572a5f204f5461/CMakeLists.txt#L38 |
I'm pretty sure the parser didn't have trouble with it.
People did notice post-release because this broke Asio CMake installation. |
Ah true, the check was basically Then maybe linter-like macros/comments like this?
|
Yes, this should work. |
This supports comments in CMLs like: ``` # Boost-Include: Boost::add_this # Boost-Exclude Boost::remove_this ``` Whitespace is ignored and the colon is optional
44cf6e1
to
650a43b
Compare
CMake supports `list(REMOVE_ITEM` without any items only since 3.20
Ok, I implemented this. It now supports comments in CMLs like:
Whitespace is ignored and the colon is optional |
Do you consider this ready to merge? |
Yes. We should document that include/exclude part somewhere but I haven't seen where it fits best. The main README seems to be for users not Boost authors |
The documentation for authors is https://github.com/boostorg/cmake/blob/develop/developer.md but I still haven't finished it. |
I'm going away on vacation next week. I probably won't be paying such close attention to boost and cmake, but I wanted to thank you for the engagement and work on this. I think that having cmake working is a bit of an enabler for encouraging more boost engagement, so it was this curiosity that was driving this exploration of boost and cmake. Thanks all. (I've decided not to take a laptop on vacation, which feels wrong, but I'll see how I go...) |
I added a commit to explain the pragmas. With that this can be merged. |
Instead of just rewriting, you should explain that while the exact form of |
When this is merged that old form is no longer relevant so I think it only confuses to put/keep that information especially as there is no change to existing code required. So I don't see what that extra piece of information provides. If you still want a reference to the Boost version would you be ok with:
That would be short enough to answer a potential questions like "Why did I had to do that extra work before?" |
Yes please. |
OK, let's have a go and see what happens. |
This broke several libraries we are working on because the dependent libraries are linked in This happens even though we add test dependencies to BOOST_INCLUDE_LIBRARIES in the root CML: Edit: It looks like dependencies in subdirectories are also getting picked up, we just needed to use Boost namespaced targets. |
Without Boost namespace, the superproject cannot resolve dependencies from subdirectory CMakeLists: boostorg/cmake#86
Without Boost namespace, the superproject cannot resolve dependencies from subdirectory CMakeLists: boostorg/cmake#86
Without Boost namespace, the superproject cannot resolve dependencies from subdirectory CMakeLists: boostorg/cmake#86
Without Boost namespace, the superproject cannot resolve dependencies from subdirectory CMakeLists: boostorg/cmake#86
I tested a few examples and found that some libraries did use Using the Boost namespaced targets should be a given as I can't see an advantage of not doing so.
Side note to be clear: "cannot resolve dependencies [from any CMakeLists]" If you find any other issues with the new scanner please report. |
Previously all libraries were added even though only a subset was selected. This makes the configure process much longer then required or may fail for libraries that aren't intended to be build.
Instead gather a 2nd list of dependencies determined by the test/CMakeLists.txt when BUILD_TESTING is ON and add only those (with disabled install and tests)
For that I factored out the collection of dependencies, which also allows to use "clean" variable names, for reuse when collecting both lists
Use case is a user who wanted to build (and test) Boost.JSON but Boost.Parser requires e.g. a higher CMake version which fails the configure process even though Boost.Parser is not required for Boost.JSON:
cmake ~/git/boost -DBUILD_TESTING=Y -DBOOST_INCLUDE_LIBRARIES=json -DBoost_DEBUG=Y
Now:
Fixes #17