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 support to use cmake package configuration file from boost #154

Merged
merged 35 commits into from
Mar 13, 2020

Conversation

Bjoe
Copy link

@Bjoe Bjoe commented Feb 13, 2020

Bjoe added 28 commits January 22, 2020 10:10
It is needed, even when you set CONFIG in find_package! See BoostConfig
line 51 in hunter.
The original FindBoost from cmake add for example in iostream regex as
required dependency but this is an optional dependency ....
@Bjoe
Copy link
Author

Bjoe commented Feb 13, 2020

This PR introduced the possibility to use the cmake config module provided from the boost project.
It is optional and can be enabled via hunter_config. Add as CMAKE_ARGS USE_CONFIG_FROM_BOOST=ON

That has the advantage that the "user" does not need to know which dependency for example boost::log needs. See the difference in the diff https://github.com/cpp-pm/hunter/pull/154/files#diff-20118a896da0e1fbefa637e8075cb6fa

It will also solve a potential problem that the BoostConfig from hunter is using FindBoost see #139

@Bjoe
Copy link
Author

Bjoe commented Feb 13, 2020

Btw. this PR needs the PR from #140

@daminetreg
Copy link

That has the advantage that the "user" does not need to know which dependency for example boost::log needs. See the difference in the diff https://github.com/cpp-pm/hunter/pull/154/files#diff-20118a896da0e1fbefa637e8075cb6fa

Note that this isn't fully true, you just need a cmake version that knows about the Boost version and then it automatically adds the knowledge of what you need to link transitively. It doesn't know when cmake is older than boost.

But clearly it's better to trust boost whenever everything works fine. The problem is mostly for me the static vs shared strategy that we should get changed.

@Bjoe
Copy link
Author

Bjoe commented Feb 13, 2020

@daminetreg Yep I know, but hunter introduced its own FindBoost.cmake and it is not well maintained ...

@daminetreg Did you know why in hunter is a FindBoost.cmake?

@Bjoe
Copy link
Author

Bjoe commented Feb 13, 2020

@daminetreg Maybe you can update this FindBoost.cmake in hunter in your PR #140 ?

@daminetreg
Copy link

@daminetreg Yep I know, but hunter introduced its own FindBoost.cmake and it is not well maintained ...

Oh 😨 Thanks for noticing, I didn't remember that.

@daminetreg Did you know why in hunter is a FindBoost.cmake?

I think he might have updated based on the version of Boost @ruslo integrated but I'm unsure. I'll try to find the common ancestor in cmake repository in the times he added the "fixed" FindBoost to see what changes he operated.

@daminetreg Maybe you can update this FindBoost.cmake in hunter in your PR #140 ?

If I can I'll make a separate PR for this, because it's better to have what we have now, than to extend the scope infinitely of this PR.

@rbsheth
Copy link
Member

rbsheth commented Feb 15, 2020

@Bjoe #140 is merged, do you need to update this one?

@Bjoe
Copy link
Author

Bjoe commented Feb 16, 2020

@Bjoe #140 is merged, do you need to update this one?

@rbsheth Yep. I will do this .... give me some time

@BenWhetton
Copy link

Does anybody know hunter maintains a fork of FindBoost.cmake? It seems like something that should be understood before bolting on more complexity to work around it?

@Bjoe
Copy link
Author

Bjoe commented Feb 19, 2020

@BenWhetton FindBoost.cmake in hunter was introduced four years ago ... I'm not sure what you mean with bolting on more complexity to work around it?

Again my idea is to use the BoostConfig from boost so that BoostConfig and FindBoost from hunter is not any more used (maybe remove it) ... in my opinion it is lesser complexity and lesser maintained stuff in hunter ...

@Bjoe
Copy link
Author

Bjoe commented Feb 19, 2020

Btw. since I merge latest master and introduced a USE_CONFIG_FROM_BOOST 1f1e729 ... it unfortunately not working very well.
If you want to have a successful test with BoostConfig from boost, then use this commit 14eb85c ...

I'm working on a proper solution to have both approaches in hunter ...

@Bjoe
Copy link
Author

Bjoe commented Feb 19, 2020

Sorry, this problem that I mentioned above was only a local issue. 1f1e729 is absolutely working correctly.

See also Travis/AppVeyor test builds:

Please test/give it a try and give your opinion

This is the hunter "version config" to test:

HunterGate(
    URL "https://github.com/Bjoe/hunter/archive/1f1e7295e026b5b6bdf6122a98d36d521fff1dbb.zip"
    SHA1 "69417c1d41f39be07465dc57e115ef97b2c648f2"
    LOCAL
)

Add in the [YOUR_PROJECT_SRC_DIR]/cmake/Hunter/config.cmake:

hunter_config(Boost VERSION 1.72.0-p0 CMAKE_ARGS USE_CONFIG_FROM_BOOST=ON)

As additional information:
I occupy all the tests in hunter to verify if my solution is working. This should not be merged because it should also test the old behaviour. Should I add some "new tests" or "take only some tests"?

@rbsheth
Copy link
Member

rbsheth commented Feb 21, 2020

Should I add some "new tests" or "take only some tests"?

What do you mean by this? If you want to test both the old and new behavior, you can add some new tests for this flag.

@BenWhetton
Copy link

@BenWhetton FindBoost.cmake in hunter was introduced four years ago ... I'm not sure what you mean with bolting on more complexity to work around it?

Again my idea is to use the BoostConfig from boost so that BoostConfig and FindBoost from hunter is not any more used (maybe remove it) ... in my opinion it is lesser complexity and lesser maintained stuff in hunter ...

I believe this solution will not work for previous versions of Boost where there is no CMake support so it is not a full solution to #141.

@Bjoe
Copy link
Author

Bjoe commented Feb 25, 2020

@BenWhetton Yep absolutely right, that's why I added a Boost version check ... for example

if(NOT USE_CONFIG_FROM_BOOST OR HUNTER_Boost_VERSION VERSION_LESS 1.72.0)
and
if(NOT HUNTER_Boost_VERSION VERSION_LESS 1.72.0)

As I said ... maybe we can remove it .... but I think it is not yet possible to remove it because it has to be backward compatible ....

Sometimes I think, maybe it is better to create a new "Boost" package with boost config support? It is maybe lesser to "maintain"/ better to understand? But I don't know if the "community"/user will understands this?

@Bjoe
Copy link
Author

Bjoe commented Feb 25, 2020

@rbsheth Ok, I added some new tests: Boost-useBoostConfig, Boost-filesystem-useBoostConfig, Boost-iostream-useBoostConfig, Boost-chrono-useBoostConfig and Boost-log-useBoostConfig

Here is the links to the Travis/AppVeyor with status "All passed":

I think I should add a comment in the Boost.rst .... I will do this next days...

@Bjoe
Copy link
Author

Bjoe commented Mar 4, 2020

Ok, I added a hint about USE_CONFIG_FROM_BOOST in the boost doc (Boost.rst)

So I'm done ...
Let's review/discuss/test :-)

Discussable is maybe that approach in hunter.cmake line 428 - 448 to set the package configuration options for the boost package configuration file.

@Bjoe Bjoe changed the title Add support to use cmake config modul from boost Add support to use cmake package configuration file from boost Mar 5, 2020
@rbsheth
Copy link
Member

rbsheth commented Mar 6, 2020

@Bjoe Do you think this is ready for merge?

@Bjoe
Copy link
Author

Bjoe commented Mar 10, 2020

All test`s are green:

With enabled USE_CONFIG_FROM_BOOST

Without enabled USE_CONFIG_FROM_BOOST

So from my side it looks fine.

It would be great if someone else can test this also ... or you wait until I test that in my company where we build a project for iOS, Android, linux, Windows and macOS.....

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.

4 participants