conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.14k stars 970 forks source link

[feature] add function "getHostCmakeVersion" #12143

Closed AndreyMlashkin closed 2 months ago

AndreyMlashkin commented 2 years ago

In many conan-center recipes, there is such line;

    def build_requirements(self):
        self.tool_requires("cmake/3.24.1")

The only reason for adding it is that CI has an older cmake version than a recipe requirement. Many recipies consumers don't need this unnesessery dependency. It would be much better to have a conditional add here, i.e.:

    def build_requirements(self):
        if getHostCmakeVersion() < "x.x.x" # where x.x.x is a recipe min requirement
              self.tool_requires("cmake/3.24.1")

Alternative solution: we can add a new field 'min_cmake_requirement' to conandata.yml and add corresponding cmake requirement in a background.

czoido commented 2 years ago

Hi @AndreyMlashkin, Thanks for the suggestion but I think, in this case most of the recipes that include that tool_requires are test_package's, so is not that if you install a Conan package you are getting an extra tool_requires. May it's a bit too much adding a built-in tool in Conan to cover that specific case and probably in the future those tool_requires will disappear from recipes. Thanks for the feedback.

AndreyMlashkin commented 2 years ago

@czoido, I can't agree: in conan-center-index there are 34 recipes that are not test_packages are requiring cmake directly:

sentry-native/all/conanfile.py:            self.build_requires("cmake/3.22.0")
enjincppsdk/all/conanfile.py:        self.build_requires("cmake/3.16.9")
diligent-core/all/conanfile.py:        self.build_requires("cmake/3.22.0")
ccache/all/conanfile.py:            self.build_requires("cmake/3.22.0")
iceoryx/all/conanfile.py:            self.tool_requires("cmake/3.16.2")
qarchive/all/conanfile.py:        self.build_requires("cmake/3.23.1")
co/all/conanfile.py:            self.build_requires("cmake/3.20.1")
libmysqlclient/all/conanfile.py:            self.build_requires("cmake/3.22.5")
cryptopp/all/conanfile.py:            self.tool_requires("cmake/3.21.0")
hexl/all/conanfile.py:        self.build_requires("cmake/3.22.0")
octo-encryption-cpp/all/conanfile.py:        self.build_requires("cmake/3.24.0")
octo-wildcardmatching-cpp/all/conanfile.py:        self.build_requires("cmake/3.24.0")
accellera-uvm-systemc/all/conanfile.py:        self.tool_requires("cmake/3.24.0")
qt/6.x.x/conanfile.py:        self.build_requires("cmake/3.23.2")
folly/all/conanfile.py:        self.build_requires("cmake/3.16.9")
mailio/all/conanfile.py:        self.build_requires("cmake/3.23.2")
daggy/2.1/conanfile.py:        self.build_requires("cmake/3.21.3")
qcoro/all/conanfile.py:        self.build_requires("cmake/3.23.2")
nmos-cpp/all/conanfile.py:        self.build_requires("cmake/3.23.2")
tensorflow-lite/all/conanfile.py:        self.tool_requires("cmake/3.24.0")
libsamplerate/all/conanfile.py:            self.tool_requires("cmake/3.24.0")
scc/all/conanfile.py:        self.tool_requires("cmake/3.24.0")
sdl/all/conanfile.py:            self.build_requires("cmake/3.22.0")
octo-keygen-cpp/all/conanfile.py:        self.tool_requires("cmake/3.24.0")
octo-logger-cpp/all/conanfile.py:        self.build_requires("cmake/3.24.0")
glog/all/conanfile.py:            self.tool_requires("cmake/3.22.3")
memsharded commented 2 years ago

This is related to this: https://github.com/conan-io/conan/pull/10166

I don't like the idiom of checking the local version in the recipes to pull or not a Conan tool_requires, this is why I proposed that PR, but it is too experimental, we will probably re-consider again in the future 2.X

AndreyMlashkin commented 2 years ago

The cleanest way would be to update cmake on all CI runners :D But as I understand, there is some blocker, right? @jgsogo

jgsogo commented 2 years ago

@AndreyMlashkin In CCI we are driven by tribe minimum requirements (https://github.com/conan-io/tribe/blob/main/design/004-tools-cmake.md) unless a specific version is required for all the recipes (this is why windows and macOS servers use other CMake versions). I think it is a good policy to keep the CI as stable as possible and, also, it should be good for broader compatibility.

I would support the proposal to add some kind of check to decide if certain tool_requires should be added or not. It was already proposed a long time ago, maybe those binary deferred could serve the same purpose.

memsharded commented 2 years ago

In any case I don't oppose to a CMake.get_version() or similar helper, I see how it can be useful, and have some recipes a bit cleaner.

memsharded commented 2 months ago

Conan 2 added the [platform_tool_requires] to avoid having to add those expression in recipes, checking the existing CMake version in recipes is not a recommended practice.

Closing as solved.