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

feat: support conan 2 #249

Merged
merged 10 commits into from
Feb 25, 2024
Merged

feat: support conan 2 #249

merged 10 commits into from
Feb 25, 2024

Conversation

FeignClaims
Copy link
Contributor

Support conan 2 by replacing the old conan script with conan_provider.cmake.

Todo:

  • customizing conan profiles and invocation of conan install.

Current design questions:

  • The new conan_provider.cmake script for conan 2 requires CMake 2.24+. Is it acceptable to set cmake_minimum_required(VERSION 3.24) for the whole project_options?
  • The cmake script for conan 2 is incompatible with conan 1. Should I keep the conan 1 function or replace it?

Close #212

@aminya
Copy link
Owner

aminya commented Feb 23, 2024

Can you keep the old version? Maybe we can specify the version in the Enable_conan flag (with the default being 1).
The version can be also detected from a CLI call to Conan.

@FeignClaims
Copy link
Contributor Author

Can you keep the old version? Maybe we can specify the version in the Enable_conan flag (with the default being 1). The version can be also detected from a CLI call to Conan.

Ok.

@FeignClaims FeignClaims changed the title Support Conan 2 feat: support conan 2 Feb 23, 2024
@FeignClaims FeignClaims force-pushed the feature/conan2 branch 2 times, most recently from 211e755 to 9e88181 Compare February 23, 2024 10:45
@FeignClaims
Copy link
Contributor Author

FeignClaims commented Feb 23, 2024

Ready to merge.

I finally choose to add a new function run_conan2() to support conan 2.

  • As for the cmake 3.24+ requirement, run_conan2() will detect it when it is actually invoked.
  • As for users who enable conan 1 but actually want conan 2 (or vise versa), run_conan2() and run_conan() will detect the version of conan and send a FATAL_ERROR message if the version dosen't match.

The cmake script of conan 2 uses a different logic to make itself almost transparent to cmake (by cmake -B build -S . -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan_provider.cmake, it can require no modification of CMakeLists.txt).

In order to integration, run_conan2 must be invoked before project(...), but conan 1 is required to run after project(...). So there's no way (AFAIK) to generalize them to one function.


As for tests, I failed to figure out how to keep both conan 1 and conan 2 tests, so I install conan 2 manually and use conan 2 in all tests. Help wanted.

@FeignClaims FeignClaims marked this pull request as ready for review February 23, 2024 12:16
@FeignClaims
Copy link
Contributor Author

So there's no way (AFAIK) to generalize them to one function.

It occurs to me there's a way to generalize them: a function run_conan() which should be invoked before project(...). Then,

  • If conan 1 is used, run_conan() registers a invocation. When project_options(...) is invoked, it checks the registration and actually calls _run_conan1() if it should.
  • If conan 2 is used, run_conan() invokes _run_conan2().

This is somehow wired since conan 1 actually runs when project_options(...) is invoked.

Should I switch to use this generalized function or just separate them into different functions like now?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Owner

@aminya aminya left a comment

Choose a reason for hiding this comment

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

I like the idea of run_conan. Should we deprecate the Enable_conan option?

@FeignClaims
Copy link
Contributor Author

FeignClaims commented Feb 25, 2024

Finished run_conan() and ready to merge.

Should we deprecate the Enable_conan option?

I removed it in documentation but kept t it in code for backward compability. A DEPRECATED_CALL option was added to distinguish the different behaviour of the interface, That is,

  • run_conan() before project(...): supports both conan 1 and conan 2. The conan 1 follows the new conan 2 interface (the HOST_PROFILEBUILD_PROFILEINSTALL_ARGS in documentation).
  • ENABLE_CONAN in project_options(...) after project(...): removed from documentation, only supports conan 1, messages a FATAL_ERROR if users enable it using conan 2.

Copy link
Owner

@aminya aminya left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! 💯

@aminya aminya merged commit da5f308 into aminya:main Feb 25, 2024
@FeignClaims FeignClaims deleted the feature/conan2 branch February 25, 2024 10:30
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.

Support Conan 2
2 participants