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

dep provider: fixes #476

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Mar 13, 2023

  • Add logic to map CMake compiler names for gcc and clang
  • Propagate the CMake generator of the current project to tools.cmake.cmaketoolchain:generator - this will cause the CMakeDeps generator to generate files in the same folder when calling conan install for both Debug and Release
  • Conditionally call conan install twice based on CMAKE_CONFIGURATION_TYPES - this is only defined in multi-config generators - unfortunately CMAKE_BUILD_TYPE may still be defined at configure time even on a multi-config generator (even if it is a no-op)
  • On non-windows platforms, test both the default generator and a multi-config generator.

The test passes on Linux and Mac now. On Linux it passes with both gcc and clang.

@jcar87 jcar87 requested a review from memsharded March 13, 2023 11:14
@CLAassistant
Copy link

CLAassistant commented Mar 13, 2023

CLA assistant check
All committers have signed the CLA.

@hwhsu1231
Copy link
Contributor

  • Propagate the CMake generator of the current project to tools.cmake.cmaketoolchain:generator - this will cause the CMakeDeps generator to generate files in the same folder when calling conan install for both Debug and Release

@jcar87 - Any idea of solving the CAE problem mentioned in #475 (comment)?

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking good!

@@ -91,6 +99,10 @@ function(detect_host_profile output_file)
else()
set(_FN ${output_file})
endif()

string(APPEND PROFILE "[conf]\n")
string(APPEND PROFILE "tools.cmake.cmaketoolchain:generator=${CMAKE_GENERATOR}\n")
Copy link
Member

Choose a reason for hiding this comment

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

Is CMAKE_GENERATOR always defined as a variable? Even when not provided in CLI and defaulted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, should be defined by CMake itself, even when not provided by the user

@@ -128,7 +140,7 @@ function(conan_provide_dependency package_name)
if(NOT CONAN_INSTALL_SUCCESS)
message(STATUS "CMake-conan: first find_package() found, running 'conan install' to install deps")
detect_host_profile(${CMAKE_BINARY_DIR}/conan_host_profile)
if(CMAKE_BUILD_TYPE)
if(NOT CMAKE_CONFIGURATION_TYPES)
Copy link
Member

Choose a reason for hiding this comment

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

I was relying on the fact that users in Windows can still force installing just 1 configuration with -DCMAKE_BUILD_TYPE=Debug, even if that does nothing, Conan will know to install only the Debug.

If we are not allowing this, we would need another kind of opt-out I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, we would need some opt-out indeed! I would try to avoid giving CMAKE_BUILD_TYPE a meaning in the context of a multi-config generator (where giving CMAKE_BUILD_TYPE a value at configure time is unfortunately allowed, but will have no effect).

@jcar87 jcar87 merged commit 82b04f2 into conan-io:experiment/cmake_project_include Mar 13, 2023
@jcar87 jcar87 deleted the lcc/experimental/dep-provider-generator-logic branch March 13, 2023 12:15
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