-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 - apply defines to Boost::headers too #11217
boost - apply defines to Boost::headers too #11217
Conversation
All green in build 1 (
|
Hooks produced the following warnings for commit 7e68973boost/1.77.0
boost/1.73.0
boost/1.71.0
boost/1.76.0
boost/1.75.0
boost/1.72.0
boost/1.79.0
boost/1.78.0
boost/1.69.0
boost/1.74.0
boost/1.70.0
|
I detected other pull requests that are modifying boost/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change makes sense. I don't like auto-linking, to be honest. it introduces more headache for us, providing less value.
Specify library name and version: boost/any version
I've mentioned a few past PRs in the hope that some boost-experts can weigh in on this PR.
I've done what I think is right, but I'm just a humble long time boost user.
Linking to Boost::headers only can still trigger autolink, even if the package had magic_autolink=False, because the no-auto-link define (BOOST_ALL_NO_LIB) was not set for the Boost::headers target.
This matters on the MSVC platform.
In my case, it was even more complicated. For MSVC:
My boost package had magic_autolink=False and layout=tagged (the default is versioned).
I had a library LibA that
#include <boost/atomic.hpp>
in its public header, and linked PUBLIC Boost::headers PRIVATE Boost::atomicI had an exe in the same cmake project that linked to LibA.
The exe code was only set to link to Boost::headers (due to the PUBLIC link in LibA).
The exe code saw the boost/atomic header, and as Boost::headers target did NOT have the autolink or layout defines, it asked the linker to link with libboost_atomic_versionnum.lib which did not exist in the package/lib folder (I had built the "tagged" library name, not the "versioned" library name).
With this change, the exe code sees the BOOST_ALL_NO_LIB and does not autolink.
And interestingly, I was able to drop Boost::atomic from the link library list entirely ... the atomic features I was using did not require linking to the library at all !
I have added a bunch of notes into the recipe, perhaps they should be shortened or removed.
The boost recipe is very elaborate, so I felt it needed some explanation.
For reference, the definitions were added in the current state in #2097
I think this is a better solution to issue #5156, you should not have to decorate your CMakeList.txt with Boost::disable_autolinking if you have already specified magic_autolink=False. Not should you have to link to (eg) Boost::atomic just to prevent it from autolinking to the wrong {layout} library name.
There are other definitions I felt also needed to be added to Boost::headers,
like BOOST_LIB_DIAGNOSTIC, and BOOST_AUTO_LINK_{layout} as I already discussed above.
I also think BOOST_ALL_DYN_LINK should be added to the headers component, as I discuss in the recipe, you can have one library using only headers, and another linking to it that also links to the actual libraries. You don't want half your boost compiled assuming static linkage, and the other half assuming dynamic linkage. Nor do you want to have to link that first library to boost-libs just so you can get those definition flags.
I did not add the BOOST_STACKTRACE_* definitions to the headers, but I do wonder if they should be added anyway. Surely any definitions should always be set when using any part of boost, as it is a complicated beast and who knows when you will include a stacktrace header and will need those definitions?