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

Poc/toolchain simpler #7076

Merged
merged 207 commits into from Jun 5, 2020
Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented May 24, 2020

Changelog: Feature: New toolchain() recipe method, as a new paradigm for integrating build systems, and simplifying developer flows.
Docs: conan-io/docs#1729

Close #7112

Simplifying, supersede #5919

This feature intruduce the toolchain new concept:

  • A toolchain() recipe method, that can use a CMakeToolchain object to define build system logic for the current package, this is not related to dependencies at all
  • The toolchain() is called at conan install time. Building with the bare build system and the toolchain should always be identical to building with Conan (build() method).
  • The toolchain (cmake one in this case) generates conan_toolchain.cmake and conan_project_include.cmake files with all the logic that can be loaded with -DCMAKE_TOOLCHAIN_FILE=conan_toolchain.cmake
  • The new CMake() build helper that is used together with the toolchain, is way simpler, does way less thing, calling cmake with the toolchain and the generator (necessary for VS, for example).
  • The CMakeToolchain only works with cmake_find_package generator

Pending/missing/todo:

  • More toolchains, for Autotools, V
  • Environment based toolchains
  • More flexible toolchains, toolchains based on user templates from other packages
  • Pass install_folder information to toolchain() method, for user custom file creation.

#tags: slow

conans/client/build/cmake.py Outdated Show resolved Hide resolved
set(CMAKE_C_FLAGS_INIT "${CONAN_C_FLAGS}" CACHE STRING "" FORCE)
set(CMAKE_SHARED_LINKER_FLAGS_INIT "${CONAN_SHARED_LINKER_FLAGS}" CACHE STRING "" FORCE)
set(CMAKE_EXE_LINKER_FLAGS_INIT "${CONAN_EXE_LINKER_FLAGS}" CACHE STRING "" FORCE)
""")
Copy link

@maikelvdh maikelvdh Jun 2, 2020

Choose a reason for hiding this comment

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

Would it be an option to inject additionally a current CMake toolchain to the generated one by using include(my_toolchain.cmake)? Or how would one handle the interoperability for example with some well-known toolchains like the one shipped as part of the Android NDK.

Copy link
Member Author

@memsharded memsharded Jun 2, 2020

Choose a reason for hiding this comment

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

Yes, definitely. This is a very barebones initial proposal, but this would be a necessary feature.

In reality, it is possible that if you are providing your own toolchain, like the own from Android, the conan one is not very used. Some of the things that this toolchain does for cmake:

  • Skip Rpath in OSX
  • Define the MSVC runtime (MT, MD) based on Conan settings
  • Define the libcxx based on Conan settings
  • Define the cppstd based on Conan settings.

It is likely that some of these are irrelevant or unused. But yes, having a way to inject another toolchain is easy and useful.

@memsharded memsharded marked this pull request as ready for review Jun 3, 2020
@memsharded memsharded requested review from SSE4 and jgsogo Jun 3, 2020
conans/client/toolchain/cmake.py Outdated Show resolved Hide resolved
conans/client/toolchain/cmake.py Outdated Show resolved Hide resolved
conans/client/build/cmake.py Outdated Show resolved Hide resolved
conans/client/build/cmake_toolchain_build_helper.py Outdated Show resolved Hide resolved
conans/client/toolchain/cmake.py Show resolved Hide resolved
conans/client/toolchain/cmake.py Show resolved Hide resolved
CMAKE_C_FLAGS_DEBUG CMAKE_CXX_FLAGS_DEBUG)
if(DEFINED ${flag})
string(REPLACE "/MD" "/MT" ${flag} "${${flag}}")
endif()
Copy link
Member

@jgsogo jgsogo Jun 4, 2020

Choose a reason for hiding this comment

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

Even though CMake won't add these flags by itself with CMP0091=NEW, can they be added by the user to the CONAN_<>_FLAGS? to environment variables? Is this something we want to cover or let the user realize about it?

conans/test/functional/toolchain/test_cmake.py Outdated Show resolved Hide resolved
# TODO: I would want to have here the path to the compiler too
build_type = self._build_type if not is_multi_configuration(self._generator) else None
try:
# This is only defined in the cache, not in the local flow
Copy link
Contributor

@TheQwertiest TheQwertiest Jun 5, 2020

Choose a reason for hiding this comment

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

Is this not what ConanFile.in_local_cache is for?

Copy link
Member Author

@memsharded memsharded Jun 5, 2020

Choose a reason for hiding this comment

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

The rationale when I coded that was that I didn't want to rely on that, I was aiming to have exactly the same experience both in the local cache and in the user local flow. So as long as the "package_folder" is defined, I would like this code to work. We need to think about this local flow with toolchains, this is a minor implementation detail.

Copy link
Contributor

@TheQwertiest TheQwertiest Jun 5, 2020

Choose a reason for hiding this comment

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

On another note, why use package_folder instead of install_folder?
Nvm, should've read docs more carefully.

jgsogo
jgsogo approved these changes Jun 5, 2020
@memsharded memsharded merged commit c923c69 into conan-io:develop Jun 5, 2020
2 checks passed
@memsharded memsharded deleted the poc/toolchain_simpler branch Jun 9, 2020
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.

6 participants