search

conan-io / conan

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

[question] visible=True for tool requires seems not to work correctly #16058

Closed maitrey closed 6 months ago

maitrey commented 7 months ago

What is your question?

Hi! I have a package A that is dependent on package B. Both must use same version of tool X and I set the visible attribute to True. recipe for B:

import os
import sys
from conan import ConanFile
from conan.tools.files import copy, save
from conan.tools.cmake import CMake, CMakeToolchain, CMakeDeps

class pkgb(ConanFile):
    name = 'pkgb'
    url = 'https://github.com/pkgb.git'
    exports_sources = "src/*", "archi/*", "doc/*", "inc/*", "CMakeLists.txt", "conf/*"
    settings = "os", "compiler", "arch", "build_type"
    python_requires = "common/2.0.0@autosar/release"
    python_requires_extend = "common.AutosarBase"

    def build_requirements(self):
        self.tool_requires("ctool/3.0.0@autosar/release", visible=True)

    def build(self):
        # we can skip build and skip tests
        # this allows us to run conan build for e.g. executing single tests
        skip_build = self.conf.get("user.build:skip_build", default=False)

        cmake = CMake(self)

        if skip_build:
            print(f"Build skipped: {skip_build}")
            cmake.configure(variables={'PYTHON_EXE': sys.executable, 'BUILD_TESTING': 'ON'} )
        else:
            cmake.configure(variables={'PYTHON_EXE': sys.executable})
            cmake.build()

        if not self.conf.get("tools.build:skip_test", default=False):
            print("Tests are set to:", self.conf.get("tools.build:skip_test"))
            ctests = self.conf.get("user.build:ctests", default="")
            verbose = '--verbose' if self.conf.get("user.build:verbose", default=False) else ''
            self.run(f"ctest {verbose} --output-on-failure -R \"{ctests}\"")

    def generate(self):
        tc = CMakeToolchain(self)
        tc.generate()
        deps = CMakeDeps(self)
        deps.generate()

    def package_info(self):
        self.cpp_info.includedirs = ['inc']
        self.cpp_info.libdirs = ['lib']
        self.cpp_info.objects = [os.path.join("lib", "packageb.c.obj")]
# EOF

Example for recipe a:

import os
import sys
from conan import ConanFile
from conan.tools.files import copy, save
from conan.tools.cmake import CMake, CMakeToolchain, CMakeDeps

class pkga(ConanFile):
    name = 'pkga'
    url = 'https://github.com/pkga.git'
    exports_sources = "src/*", "archi/*", "doc/*", "inc/*", "CMakeLists.txt", "conf/*"
    settings = "os", "compiler", "arch", "build_type"
    python_requires = "common/2.0.0@autosar/release"
    python_requires_extend = "common.AutosarBase"

    def requirements(self):
        self.requires("pkgb/1.0.0@autosar/release")

    def build_requirements(self):
        self.tool_requires("ctool/3.0.0@autosar/release", visible=True)

    def build(self):
        # we can skip build and skip tests
        # this allows us to run conan build for e.g. executing single tests
        skip_build = self.conf.get("user.build:skip_build", default=False)

        cmake = CMake(self)

        if skip_build:
            print(f"Build skipped: {skip_build}")
            cmake.configure(variables={'PYTHON_EXE': sys.executable, 'BUILD_TESTING': 'ON'} )
        else:
            cmake.configure(variables={'PYTHON_EXE': sys.executable})
            cmake.build()

        if not self.conf.get("tools.build:skip_test", default=False):
            print("Tests are set to:", self.conf.get("tools.build:skip_test"))
            ctests = self.conf.get("user.build:ctests", default="")
            verbose = '--verbose' if self.conf.get("user.build:verbose", default=False) else ''
            self.run(f"ctest {verbose} --output-on-failure -R \"{ctests}\"")

    def generate(self):
        tc = CMakeToolchain(self)
        tc.generate()
        deps = CMakeDeps(self)
        deps.generate()

    def package_info(self):
        self.cpp_info.includedirs = ['inc']
        self.cpp_info.libdirs = ['lib']
        self.cpp_info.objects = [os.path.join("lib", "packageb.c.obj")]
# EOF

CMakeLists for pkga has: find_package(b config REQUIRED) pkg a fails with : -- Conan: Target declared 'pkgb::pkgb' CMake Error at C:/ProgramFiles/CMake/share/cmake-3.26/Modules/CMakeFindDependencyMacro.cmake:76 (find_package): Could not find a package configuration file provided by "ctool" with any of the following names:

ctoolConfig.cmake
ctool-config.cmake

Add the installation prefix of "ctool" to CMAKE_PREFIX_PATH or set "ctool_DIR" to a directory containing one of the above files. If "ctool" provides a separate development package or SDK, be sure it has been installed. Call Stack (most recent call first): kdp/build/Release/generators/pkgb-config.cmake:24 (find_dependency) _src/CMakeLists.txt:6 (find_package) What I donot understand why is it looking for ctool in find_package (b CONFIG REQUIRED) Am I not using the visible attribute in the correct way?

Could you please help me?

Thanks! Br, Maitrey

Have you read the CONTRIBUTING guide?

memsharded commented 7 months ago

Hi @maitrey

Thanks for your question.

Hi! I have a package A that is dependent on package B. Both must use same version of tool X and I set the visible attribute to True. recipe for B:

I'd like to challenge this idea. The visible=True does way more than just forcing the versions to be aligned. It propagate downstream the tool-require, which might be unintended in many cases.

There might be other approaches to align the version easily, for example using a version range like this in all the recipes:

def build_requirements(self):
        self.tool_requires("ctool/[>=3.0 <4]@autosar/release")

will automatically align the versions to the same and latest one within the range.

I will have a look to your report, but as a starting point, I'd probably discourage making tool-requires visible=True for this reason, it might not be the best solution.

maitrey commented 7 months ago

It works if I set this : https://docs.conan.io/2/reference/tools/cmake/cmakedeps.html#build-context-build-modules for ctool. And this seems to be fitting to my use-case. Isnot it? Also the reason for visible=True is because we really have to use same version of ctool in all our packages.

memsharded commented 7 months ago

Yes, this is the gap I am having a look to, it is not expected that tool_requires are propagated transitively and also used with the visible=True for CMakeDeps.

Even if I am having a look and trying to improve it, I'd still recommend thinking my comment above, using visible=True is probably not the best approach to this problem.

maitrey commented 7 months ago

Any other alternative as this tool is common for all the packages and must be set with visible=True so that any other versions fail to build the dependents.

memsharded commented 6 months ago

Any other alternative as this tool is common for all the packages and must be set with visible=True so that any other versions fail to build the dependents.

Yes, this case is recommended to do with:

[tool_requires]
ctool/version

In your profile file, this is a better alternative in most senses:

iso8859-1 commented 6 months ago

will tool_requires / build_requires factor into the binary-hash? I'm just asking under the scenario that I have a code generator for multiple packages and they all need to have the same version because the generated code might not be compatible if different generators are used. If I'm consuming those packages as binaries and tool_requires / build_requires is not factored into the id, they might not align - or am I seeing something wrong? In this case I would have to use a "normal" / library require?

memsharded commented 6 months ago

will tool_requires / build_requires factor into the binary-hash?

Yes, if you enable it in the appropriate config or in the package:

The factoring into the consumers package_id is the same for recipe self.tool_requires(), by default it is not factored-in, it has to be configured.

maitrey commented 6 months ago

Any other alternative as this tool is common for all the packages and must be set with visible=True so that any other versions fail to build the dependents.

Yes, this case is recommended to do with:

[tool_requires]
ctool/version

In your profile file, this is a better alternative in most senses:

  • easier to update versions without modifying all the recipes
  • easier to swap to another tool-requires for other profiles
  • no need to make it visible
  • simpler recipes

Even if I set this in profiles, and if developer configures any higher version in his recipe then he will still be allowed to use the higher version? Right? I have not yet tested your suggestion but before that asking.

memsharded commented 6 months ago

Even if I set this in profiles, and if developer configures any higher version in his recipe then he will still be allowed to use the higher version? Right? I have not yet tested your suggestion but before that asking.

No, the profile is the most "downstream" user definition, and as such it always have precedence. Whatever you type in your profiles or command line will have precedence over recipe-defined things. It will be very weird to do conan install . -o *:shared=True and still get static libraries, just because recipes decided to define shared=False instead. The final user should always have control.

The strategy for this is recipes won't define the tool-requires at all, but use it from the profiles. Users that want to use a different version of the tool, can override their profile or append an extra profile. Or even use the per-package syntax mypkg/*:ctool/version2 to apply that tool require just to that package.

But in any case, what you are asking about recipes being able to define their own version conflicts with the initial approach you were trying. If you make the tool_requires visible=True, it will not allow recipes to define different versions, because they will conflict.

memsharded commented 6 months ago

This is being marked for closing with the fix in https://github.com/conan-io/conan/pull/16077.

Still, marking the tool_requires(..., visible=True) doesn't seem like a recommended approach for this issue, I wouldn't recommend it, and the profile defined one seems better approach.

maitrey commented 6 months ago

Thanks @memsharded . I will need to test it and provide feedback with profiles. Many thanks for your suggestion.

maitrey commented 6 months ago

One question somehow the last suggestion introduces a hen and egg problem.. If I have to release ctool and I have conan-config with profiles , if there is any change in profiles there will be a problem what to release first ctool or conan-config. Isnot it? Second question: Your suggestion is not to use tools with attribute visible=True but what does this ticket solve then: https://github.com/conan-io/conan/pull/16077

memsharded commented 6 months ago

If I have to release ctool and I have conan-config with profiles , if there is any change in profiles there will be a problem what to release first ctool or conan-config.

It depends if the profiles do require a different package_id for ctool, you will have to re-build it with the new profiles.

Second question: Your suggestion is not to use tools with attribute visible=True but what does this ticket solve then: https://github.com/conan-io/conan/pull/16077

Because one thing is that it is not recommended at all for your problem, and another different thing is that it fails that way, that it shouldn't even if users make it visible=True. Note the fix is actually excluding the transitive generation of files for the consumer, because they are "build" requires, so even if visible=True, they are not propagated for the build-system, it doesn't really make sense.