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

CMake Conan 2.0 compatibility #13739

Merged
merged 41 commits into from
Jan 14, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
8014395
CMake Conan 2.0 compatibility
System-Arch Oct 25, 2022
3279e24
Fix lint errors
System-Arch Oct 25, 2022
312df47
Avoid lint error with Conan 2.0 argument name
System-Arch Oct 25, 2022
3498d3f
Avoid lint error with Conan 2.0 argument name
System-Arch Oct 25, 2022
bb9383d
Removed unused imports
System-Arch Oct 25, 2022
31ab1a3
Handle Conan version disparities
System-Arch Oct 25, 2022
05f13da
Use basic_layout with Autotools
System-Arch Oct 25, 2022
5769d2c
Call AutotoolsDeps generate
System-Arch Oct 25, 2022
b00e0bd
Call CMakeDeps generate
System-Arch Oct 25, 2022
520b6e1
Update imports per suggestioned changes
System-Arch Oct 25, 2022
4be0900
Removed extra space
System-Arch Oct 25, 2022
8a86742
Remove extraneous items
System-Arch Oct 26, 2022
cb56f47
Determine require_version in a version-agnostic manner
System-Arch Nov 2, 2022
fe4cb86
Use Conan 1.53
System-Arch Nov 2, 2022
8154df4
Handle @ in ref
System-Arch Nov 3, 2022
887635c
Fix lint errors
System-Arch Nov 3, 2022
a4b5d3a
Bigger hammer
System-Arch Nov 3, 2022
cede0fe
Remove PATH addition in package_id
System-Arch Nov 3, 2022
b6accd3
Use validate_build instead of validate
System-Arch Nov 3, 2022
8c33609
Moved Mac x86 check to validate() method.
System-Arch Nov 14, 2022
f0fea9e
Bump openssl version
System-Arch Nov 14, 2022
2e008ee
Merge branch 'master' into System-Arch-cmake-1
System-Arch Nov 15, 2022
ed5ef30
Add blank line to trigger CI build
System-Arch Nov 15, 2022
0485ca3
Eliminate use of validate_build()
System-Arch Nov 18, 2022
4713991
Restore use of validate_build()
System-Arch Nov 18, 2022
4354d5e
Merge branch 'master' into System-Arch-cmake-1
System-Arch Nov 28, 2022
e582a53
Removed blank line to trigger CI
System-Arch Nov 29, 2022
be5ffcf
Add blank line to trigger CI
System-Arch Dec 1, 2022
568f04c
Added boostrap options and removed unneeded env. vars
System-Arch Dec 3, 2022
5836bfc
Deal with Conan 1.x vs 2.0 inconsistencies
System-Arch Dec 3, 2022
19f2c05
Fixed lint issue
System-Arch Dec 3, 2022
dd2bdee
Placate linter
System-Arch Dec 3, 2022
58ff30a
Apply suggestions from code review
System-Arch Dec 6, 2022
c9345af
Use save & load for bootstrap args; Use tool_requires in test packages
System-Arch Dec 7, 2022
bb59eb4
Eliminate use of validate_build; Add more settings to test recipes
System-Arch Dec 7, 2022
6b5e6e2
Set PATH in package_info() for v1.x; Lower req. ver. to 1.50
System-Arch Dec 7, 2022
b100df8
Apply suggestions from code review
System-Arch Dec 8, 2022
7fa560a
Added AutotoolsDeps generator; Eliminated can_run() from tests
System-Arch Dec 9, 2022
3bf716f
Correct msvc version
System-Arch Dec 12, 2022
8fa91ae
Use f-strings
System-Arch Dec 12, 2022
182c7d4
Replace VirtualRunEnv with VirtualBuildEnv
System-Arch Jan 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 52 additions & 53 deletions recipes/cmake/3.x.x/conanfile.py
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
import os
from conan import ConanFile
from conan.tools.files import chdir, copy, rmdir, get
from conan.tools.cmake import CMake, CMakeToolchain, cmake_layout
from conan.tools.gnu import Autotools, AutotoolsToolchain
Copy link
Contributor

Choose a reason for hiding this comment

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

No AutotoolsDeps? Is it robust?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate, not an autotools users so I am not sure what you might be referring too?

Copy link
Contributor

@SpaceIm SpaceIm Dec 7, 2022

Choose a reason for hiding this comment

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

OpenSSL is an optional dependency. AutotoolsDeps injects dependencies informations in compile and link flags of autotools, so that it can work out of the box. Usually if upstram configure.ac relies on pkgconf to find & inject these flags, AutotoolsDeps is not needed (but PkgConfigDeps is), otherwise it may be fragile and AutotoolsDeps is here to improve robustness.
Legacy AutotoolsBuildEnvironment was like an union of AutotoolsToolchain, AutotoolsDeps & Autotools conan v2 build helpers.

from conan.tools.layout import basic_layout
from conan.tools.build import build_jobs, cross_building, check_min_cppstd
from conan.tools.scm import Version
from conan.tools.files import rmdir, get
from conans import tools, AutoToolsBuildEnvironment, CMake
from conan.errors import ConanInvalidConfiguration, ConanException
from conan.errors import ConanInvalidConfiguration
from conan.errors import ConanException
import os

required_conan_version = ">=1.49.0"
required_conan_version = ">=1.53.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need 1.53.0? 1.50.0 seems to be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to stay current with what CCI is using although, regrettably, it is several releases out-of-date despite guidance to always test with the latest release

Copy link
Contributor

Choose a reason for hiding this comment

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

It's orthogonal. required_conan_version should be the minimum conan version being able to execute this conanfile, not conan version running in c3i.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The c3i version of Conan is the only one that (as far as I know) the recipes get tested with, so unless one does off-line regression testing, there is no guarantee that an earlier version is suitable--especially across the large number of configurations that are tested here.

Copy link
Contributor

@SpaceIm SpaceIm Dec 6, 2022

Choose a reason for hiding this comment

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

c3i doesn't test 1.54.0 or 1.55.0 either, so it's not how we reason. It's like cmake_minimum_required(). Check minimum conan version needed by each conan function (in conan doc), it will give you min conan version, that's all (like all other CCI recipes).

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said it's orthogonal, please change required_conan_version to >=1.50.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @prince-chrismc, HI @memsharded,
Can you provide clarity around the CCI required_conan_version strategy? Isn't the goal to move everything to 2.0 and to leverage the new features and thus qualify with the latest versions of Conan 1.x and Conan 2.0? If not, I guess I am confused as to how the CI system is actually validating recipes unless it is looking at the required_conan_version value and using the specified version of Conan for qualification testing, which, based on my understanding of the infrastructure, it is not that sophisticated, but it would be a nice feature.

Copy link
Contributor

@jcar87 jcar87 Dec 7, 2022

Choose a reason for hiding this comment

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

As far as we are concerned from our side, we appreciate that this is a community-lead effort, and we do not expect contributors to spend significant effort in working out the exact minimum Conan version with which a recipe is compatible, although we do please ask that it is not any lower than what is actually needed, so that users will get a clear error that they can resolve for themselves if they are using a version of Conan that is too old for the recipe. Indeed, our CI does not check this so this is a best-effort situation.

If this recipe were to be merged with a version higher than strictly necessary I would not expect significant disruption at this point - given that theres a great number of recipes already requiring 1.53, and we do expect users of Conan Center to be on recent versions - chances are most users already are, making this a moot point by now.

On the other hand, if a reviewer kindly spots that this as an improvement, there's no harm in addressing it.

CC @prince-chrismc

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn’t agree more, it's best effort to accurately get the true "minimum" and a good default is the one being tested in CCI. Most often we are updating recipes to that latest version so generally those coincide.

I like to drop links with "hey this looks like the most recent feature being used" such that I can politely get them lower it when I know it's too high. If a reviewer shows an accurate version, that would improve the quality of the recipe making it more widely available which is not a bad things.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be transparent, the behind the scenes we've had lots of conversations about the role of the linters. Just my quick summary

  • Hooks: validate recipe folder and package contents + those match the recipe's implementation themselves.
  • Pylinter: Ensures the quality of recipes by enforcing best practices and proven conventions

I personally would love to see the pylint improved with a table of "these imports were added in this version" and double check that and report an error. However unless it's in the linter and enforced (and my example is not enough to guarantee that) its best effort to get the exactly right one.

we kindly ask contributors do their best and reviewers to helpfully share details they know so together we get the best recipe.


class CMakeConan(ConanFile):
name = "cmake"
package_type = "application"
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
description = "Conan installer for CMake"
topics = ("cmake", "build", "installer")
url = "https://github.com/conan-io/conan-center-index"
homepage = "https://github.com/Kitware/CMake"
license = "BSD-3-Clause"
generators = "cmake"
settings = "os", "arch", "compiler", "build_type"

options = {
Expand All @@ -26,9 +30,6 @@ class CMakeConan(ConanFile):
"bootstrap": False,
}

_source_subfolder = "source_subfolder"
_cmake = None

def config_options(self):
if self.settings.os == "Windows":
self.options.with_openssl = False
Expand All @@ -37,16 +38,16 @@ def requirements(self):
if self.options.with_openssl:
self.requires("openssl/1.1.1q")
System-Arch marked this conversation as resolved.
Show resolved Hide resolved

def validate(self):
def validate_build(self):
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
if self.settings.os == "Macos" and self.settings.arch == "x86":
raise ConanInvalidConfiguration("CMake does not support x86 for macOS")

if self.settings.os == "Windows" and self.options.bootstrap:
raise ConanInvalidConfiguration("CMake does not support bootstrapping on Windows")

minimal_cpp_standard = "11"
if self.settings.compiler.cppstd:
tools.check_min_cppstd(self, minimal_cpp_standard)
if self.settings.get_safe("compiler.cppstd"):
check_min_cppstd(self, minimal_cpp_standard)

minimal_version = {
"gcc": "4.8",
Expand All @@ -57,9 +58,9 @@ def validate(self):

compiler = str(self.settings.compiler)
if compiler not in minimal_version:
self.output.warn(
self.output.warning(
"{} recipe lacks information about the {} compiler standard version support".format(self.name, compiler))
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
self.output.warn(
self.output.warning(
"{} requires a compiler that supports at least C++{}".format(self.name, minimal_cpp_standard))
return

Expand All @@ -68,71 +69,69 @@ def validate(self):
raise ConanInvalidConfiguration(
"{} requires a compiler that supports at least C++{}".format(self.name, minimal_cpp_standard))

def layout(self):
if self.options.bootstrap:
basic_layout(self, src_folder="src")
else:
cmake_layout(self, src_folder="src")

def source(self):
get(self, **self.conan_data["sources"][self.version], strip_root=True, destination=self._source_subfolder)
get(self, **self.conan_data["sources"][self.version],
destination=self.source_folder, strip_root=True)

def _configure_cmake(self):
if not self._cmake:
self._cmake = CMake(self)
def generate(self):
if self.options.bootstrap:
tc = AutotoolsToolchain(self)
tc.generate()
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
else:
tc = CMakeToolchain(self)
if not self.settings.compiler.cppstd:
self._cmake.definitions["CMAKE_CXX_STANDARD"] = 11
self._cmake.definitions["CMAKE_BOOTSTRAP"] = False
tc.variables["CMAKE_CXX_STANDARD"] = 11
tc.variables["CMAKE_BOOTSTRAP"] = False
if self.settings.os == "Linux":
self._cmake.definitions["CMAKE_USE_OPENSSL"] = self.options.with_openssl
tc.variables["CMAKE_USE_OPENSSL"] = self.options.with_openssl
if self.options.with_openssl:
self._cmake.definitions["OPENSSL_USE_STATIC_LIBS"] = not self.options["openssl"].shared
if tools.cross_building(self):
self._cmake.definitions["HAVE_POLL_FINE_EXITCODE"] = ''
self._cmake.definitions["HAVE_POLL_FINE_EXITCODE__TRYRUN_OUTPUT"] = ''
self._cmake.configure(source_folder=self._source_subfolder)

return self._cmake
openssl = self.dependencies["openssl"]
tc.variables["OPENSSL_USE_STATIC_LIBS"] = not openssl.options.shared
if cross_building(self):
tc.variables["HAVE_POLL_FINE_EXITCODE"] = ''
tc.variables["HAVE_POLL_FINE_EXITCODE__TRYRUN_OUTPUT"] = ''
tc.generate()
System-Arch marked this conversation as resolved.
Show resolved Hide resolved

def build(self):
if self.options.bootstrap:
with tools.chdir(self._source_subfolder):
self.run(['./bootstrap', '--prefix={}'.format(self.package_folder), '--parallel={}'.format(tools.cpu_count())])
autotools = AutoToolsBuildEnvironment(self)
with chdir(self, self.source_folder):
self.run('./bootstrap --prefix="" --parallel={}'.format(build_jobs(self)))
autotools = Autotools(self)
autotools.make()
else:
tools.replace_in_file(os.path.join(self._source_subfolder, "CMakeLists.txt"),
"project(CMake)",
"project(CMake)\ninclude(\"{}/conanbuildinfo.cmake\")\nconan_basic_setup(NO_OUTPUT_DIRS)".format(
self.install_folder.replace("\\", "/")))
if self.settings.os == "Linux":
tools.replace_in_file(os.path.join(self._source_subfolder, "Utilities", "cmcurl", "CMakeLists.txt"),
"list(APPEND CURL_LIBS ${OPENSSL_LIBRARIES})",
"list(APPEND CURL_LIBS ${OPENSSL_LIBRARIES} ${CMAKE_DL_LIBS} pthread)")

cmake = self._configure_cmake()
cmake = CMake(self)
cmake.configure()
cmake.build()

def package(self):
self.copy("Copyright.txt", dst="licenses", src=self._source_subfolder)
copy(self, "Copyright.txt", self.source_folder, os.path.join(self.package_folder, "licenses"), keep_path=False)
if self.options.bootstrap:
with tools.chdir(self._source_subfolder):
autotools = AutoToolsBuildEnvironment(self)
with chdir(self, self.source_folder):
autotools = Autotools(self)
autotools.install()
else:
cmake = self._configure_cmake()
cmake = CMake(self)
cmake.configure()
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
cmake.install()
rmdir(self, os.path.join(self.package_folder, "doc"))

def package_id(self):
del self.info.settings.compiler
del self.info.options.bootstrap

def package_info(self):
module_version = "{}.{}".format(Version(self.version).major, Version(self.version).minor)

bindir = os.path.join(self.package_folder, "bin")
self.output.info("Appending PATH environment variable: {}".format(bindir))
self.env_info.PATH.append(bindir)
System-Arch marked this conversation as resolved.
Show resolved Hide resolved

self.buildenv_info.prepend_path("CMAKE_ROOT", self.package_folder)
self.env_info.CMAKE_ROOT = self.package_folder
self.runenv_info.define_path("CMAKE_ROOT", self.package_folder)
module_version = "{}.{}".format(Version(self.version).major, Version(self.version).minor)
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
mod_path = os.path.join(self.package_folder, "share", f"cmake-{module_version}", "Modules")
self.buildenv_info.prepend_path("CMAKE_MODULE_PATH", mod_path)
self.env_info.CMAKE_MODULE_PATH = mod_path
self.runenv_info.define_path("CMAKE_MODULE_PATH", mod_path)
if not os.path.exists(mod_path):
raise ConanException("Module path not found: %s" % mod_path)

Expand Down
8 changes: 5 additions & 3 deletions recipes/cmake/3.x.x/test_package/conanfile.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import os
from six import StringIO
from conan import ConanFile
from conan.tools.build import can_run
import re


class TestPackageConan(ConanFile):
Expand All @@ -14,10 +14,12 @@ def requirements(self):
def test(self):
if can_run(self):
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
output = StringIO()
self.run("cmake --version", env="conanrun", output=output)
# Third arg to self.run renamed "stdout" in Conan 2.0 but 1.x linter doesn't like it
self.run("cmake --version", output, env="conanrun")
output_str = str(output.getvalue())
self.output.info("Installed version: {}".format(output_str))
require_version = str(self.deps_cpp_info["cmake"].version)
tokens = re.split('[@#]', self.tested_reference_str)
require_version = tokens[0].split("/", 1)[1]
self.output.info("Expected version: {}".format(require_version))
assert_cmake_version = "cmake version %s" % require_version
assert(assert_cmake_version in output_str)