Repository: conan-io/conan
Pull request #16849 (2024-08-20 21:43:10)

Fix/transitive build requires minimal changes
Changelog: Fix https://github.com/conan-io/conan/issues/16333
This Pull Request is #16539 with more minimal changes. Requires in build context are now propagated depending on their visibility and `run` trait, but `headers` and `libs` are always set to `False`.

The proposed changes fix the example in the linked issue and also our local Simulink workflow. Please let me know about additional checks / use cases to be performed or where unit tests could be added.

Note that the expected value of the `run` trait had to change with some tests with packages in build context as this is now respecting what was initially set - for tool requirements, this is `run=True`.

- [x] Refer to the issue that supports this Pull Request.
- [x] If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
- [x] I've read the [Contributing guide](https://github.com/conan-io/conan/blob/develop2/.github/CONTRIBUTING.md).
- [X] I've followed the PEP8 style guides for Python code.
- [X] I've opened another PR in the Conan docs repo to the ``develop`` branch, documenting this one: https://github.com/conan-io/docs/pull/3816


maximilianmuehlbauer: > Thanks for following up on the review and contributing this PR. I have been reviewing it, also added an extra test while running some checks. I think this is much lower risk than the previous one, and I think it can be moved forward for next release.

Thanks, that sounds good!

> We will probably need to document this a bit more and mark it as experimental just in case, but seems this is a more correct behavior.

I've started documenting it here: https://github.com/conan-io/docs/pull/3816

One more thing might be to disallow or print warnings when `headers=True` or `libs=True` is set for `build=True` in the first place, what do you think?
memsharded: > One more thing might be to disallow or print warnings when headers=True or libs=True is set for build=True in the first place, what do you think?

I think at the moment I am fine with not propagating them. An error/hard disallow will probably be too much, one of the problems of having such a large userbase is that it is likely that there is someone out there relying on being able to do that, and we will break them. And warnings and other information have relatively low value, practically no ones reads them...

So probably it is not worth the effort, until some users (repeatedly) report issues or confusions being hit by this.

Patch:
diff --git a/conans/model/requires.py b/conans/model/requires.py
index 23987d0dc..b1974a7dc 100644
--- a/conans/model/requires.py
+++ b/conans/model/requires.py
@@ -270,15 +270,18 @@ class Requirement:
 
         if require.build:  # public!
             # TODO: To discuss if this way of conflicting build_requires is actually useful or not
+            # Build-requires will propagate its main trait for running exes/shared to downstream
+            # consumers so run=require.run, irrespective of the 'self.run' trait
             downstream_require = Requirement(require.ref, headers=False, libs=False, build=True,
-                                             run=False, visible=self.visible, direct=False)
+                                             run=require.run, visible=self.visible, direct=False)
             return downstream_require
 
         if self.build:  # Build-requires
             # If the above is shared or the requirement is explicit run=True
+            # visible=self.visible will further propagate it downstream
             if dep_pkg_type is PackageType.SHARED or require.run:
                 downstream_require = Requirement(require.ref, headers=False, libs=False, build=True,
-                                                 run=True, visible=False, direct=False)
+                                                 run=True, visible=self.visible, direct=False)
                 return downstream_require
             return
 


Test patch:
diff --git a/test/functional/revisions_test.py b/test/functional/revisions_test.py
index e763a8937..bf052ca71 100644
--- a/test/functional/revisions_test.py
+++ b/test/functional/revisions_test.py
@@ -658,6 +658,7 @@ def test_touching_other_server():
     assert "ERROR: Missing binary: pkg/0.1@conan/channel" in c.out
 
 
+@pytest.mark.artifactory_ready
 def test_reupload_older_revision():
     """ upload maintains the server history
         https://github.com/conan-io/conan/issues/7331
@@ -698,3 +699,41 @@ def test_reupload_older_revision():
     c.run(f"remove pkg/0.1#{rrev1} -r=default -c")
     c.run("upload * -r=default -c --force")
     check_order(inverse=True)
+
+
+@pytest.mark.artifactory_ready
+def test_reupload_older_revision_new_binaries():
+    """ upload maintains the server history
+        https://github.com/conan-io/conan/pull/16621
+    """
+    c = TestClient(default_server_user=True)
+    c.save({"conanfile.py": GenConanfile("pkg", "0.1").with_settings("os")})
+    c.run("create . -s os=Linux")
+    rrev1 = c.exported_recipe_revision()
+    c.run("upload * -r=default -c")
+    c.save({"conanfile.py": GenConanfile("pkg", "0.1").with_settings("os")
+                                                      .with_class_attribute("potato = 42")})
+    c.run("create . -s os=Linux")
+    rrev2 = c.exported_recipe_revision()
+    c.run("upload * -r=default -c")
+
+    def check_order(inverse=False):
+        c.run("list pkg/0.1#* -r=default")
+        out = str(c.out)
+        assert rrev1 in out
+        assert rrev2 in out
+        if inverse:
+            assert out.find(rrev1) > out.find(rrev2)
+        else:
+            assert out.find(rrev1) < out.find(rrev2)
+
+    check_order()
+
+    # If we create the same older revision, and upload, still the same order
+    # c.run("remove * -c")  # Make sure no other revision
+    c.save({"conanfile.py": GenConanfile("pkg", "0.1").with_settings("os")})
+    c.run("create . -s os=Windows")
+    rrev3 = c.exported_recipe_revision()
+    assert rrev3 == rrev1
+    c.run(f"upload pkg*#{rrev3} -r=default -c --force")
+    check_order()
diff --git a/test/integration/graph/core/test_build_requires.py b/test/integration/graph/core/test_build_requires.py
index acf8c40e2..7e396044b 100644
--- a/test/integration/graph/core/test_build_requires.py
+++ b/test/integration/graph/core/test_build_requires.py
@@ -639,7 +639,89 @@ class PublicBuildRequiresTest(GraphManagerTest):
         # node, include, link, build, run
         _check_transitive(lib, [(cmake, False, False, True, True)])
         _check_transitive(app, [(lib, True, True, False, False),
-                                (cmake, False, False, True, False)])
+                                (cmake, False, False, True, True)])
+
+    def test_deep_dependency_tree(self):
+        # app -> liba -> libb-(br public) -> sfun -> libsfun -> libx -> liby -> libz
+        #                    -(normal req) -> libsfun -> libx -> liby -> libz
+        self.recipe_conanfile("libz/0.1", GenConanfile())
+        self.recipe_conanfile("liby/0.1", GenConanfile().with_requirement("libz/0.1", run=True))
+        self.recipe_conanfile("libx/0.1", GenConanfile().with_requirement("liby/0.1", run=True))
+        self.recipe_conanfile("libsfun/0.1", GenConanfile().with_requirement("libx/0.1", run=True))
+        self.recipe_conanfile("sfun/0.1", GenConanfile().with_requirement("libsfun/0.1", run=True))
+        self.recipe_conanfile("libb/0.1", GenConanfile()
+                              .with_tool_requirement("sfun/0.1", visible=True)
+                              .with_requirement("libsfun/0.1", run=True))
+        self.recipe_conanfile("liba/0.1", GenConanfile().with_requirement("libb/0.1", run=True))
+        deps_graph = self.build_graph(GenConanfile("app", "0.1").with_requirement("liba/0.1", run=True))
+
+        # Build requires always apply to the consumer
+        self.assertEqual(8 + 4, len(deps_graph.nodes))
+        app = deps_graph.root
+        liba = app.dependencies[0].dst
+        libb = liba.dependencies[0].dst
+        libsfun = libb.dependencies[0].dst
+        libx = libsfun.dependencies[0].dst
+        liby = libx.dependencies[0].dst
+        libz = liby.dependencies[0].dst
+        sfun = libb.dependencies[1].dst
+        libsfun_build = sfun.dependencies[0].dst
+        libx_build = libsfun_build.dependencies[0].dst
+        liby_build = libx_build.dependencies[0].dst
+        libz_build = liby_build.dependencies[0].dst
+
+        # TODO non-build-requires
+
+        self._check_node(app, "app/0.1@", deps=[liba], dependents=[])
+        self._check_node(liba, "liba/0.1#123", deps=[libb], dependents=[app])
+        self._check_node(libb, "libb/0.1#123", deps=[sfun, libsfun], dependents=[liba])
+        self._check_node(sfun, "sfun/0.1#123", deps=[libsfun_build], dependents=[libb])
+        self._check_node(libsfun_build, "libsfun/0.1#123", deps=[libx_build], dependents=[sfun])
+        self._check_node(libx_build, "libx/0.1#123", deps=[liby_build], dependents=[libsfun_build])
+        self._check_node(liby_build, "liby/0.1#123", deps=[libz_build], dependents=[libx_build])
+        self._check_node(libz_build, "libz/0.1#123", deps=[], dependents=[liby_build])
+
+        # node, include, link, build, run
+        _check_transitive(liby_build, [(libz_build, True, True, False, True)])
+        _check_transitive(libx_build, [(liby_build, True, True, False, True),
+                                       (libz_build, True, True, False, True)])
+        _check_transitive(libsfun_build, [(libx_build, True, True, False, True),
+                                          (liby_build, True, True, False, True),
+                                          (libz_build, True, True, False, True)])
+        _check_transitive(sfun, [(libsfun_build, True, True, False, True),
+                                 (libx_build, True, True, False, True),
+                                 (liby_build, True, True, False, True),
+                                 (libz_build, True, True, False, True)])
+        _check_transitive(libb, [(libsfun, True, True, False, True),
+                                 (libx, True, True, False, True),
+                                 (liby, True, True, False, True),
+                                 (libz, True, True, False, True),
+                                 (sfun, False, False, True, True),
+                                 (libsfun_build, False, False, True, True),
+                                 (libx_build, False, False, True, True),
+                                 (liby_build, False, False, True, True),
+                                 (libz_build, False, False, True, True)])
+        _check_transitive(liba, [(libb, True, True, False, True),
+                                 (libsfun, True, True, False, True),
+                                 (libx, True, True, False, True),
+                                 (liby, True, True, False, True),
+                                 (libz, True, True, False, True),
+                                 (sfun, False, False, True, True),
+                                 (libsfun_build, False, False, True, True),
+                                 (libx_build, False, False, True, True),
+                                 (liby_build, False, False, True, True),
+                                 (libz_build, False, False, True, True)])
+        _check_transitive(app, [(liba, True, True, False, True),
+                                (libb, True, True, False, True),
+                                (libsfun, True, True, False, True),
+                                (libx, True, True, False, True),
+                                (liby, True, True, False, True),
+                                (libz, True, True, False, True),
+                                (sfun, False, False, True, True),
+                                (libsfun_build, False, False, True, True),
+                                (libx_build, False, False, True, True),
+                                (liby_build, False, False, True, True),
+                                (libz_build, False, False, True, True)])
 
     def test_conflict_diamond(self):
         # app -> libb -(br public)-> cmake/0.1
@@ -668,6 +750,40 @@ class PublicBuildRequiresTest(GraphManagerTest):
         self._check_node(libb, "libb/0.1#123", deps=[cmake1], dependents=[app])
         self._check_node(cmake1, "cmake/0.1#123", deps=[], dependents=[libb])
 
+    def test_conflict_diamond_two_levels(self):
+        # app -> libd -> libb -(br public)-> cmake/0.1
+        #   \--> libe -> libc -(br public)-> cmake/0.2
+        self.recipe_conanfile("cmake/0.1", GenConanfile())
+        self.recipe_conanfile("cmake/0.2", GenConanfile())
+        self.recipe_conanfile("libb/0.1",
+                              GenConanfile().with_tool_requirement("cmake/0.1", visible=True))
+        self.recipe_conanfile("libc/0.1",
+                              GenConanfile().with_tool_requirement("cmake/0.2", visible=True))
+        self.recipe_conanfile("libd/0.1", GenConanfile().with_requires("libb/0.1"))
+        self.recipe_conanfile("libe/0.1", GenConanfile().with_requires("libc/0.1"))
+
+        deps_graph = self.build_graph(GenConanfile("app", "0.1").with_requires("libd/0.1",
+                                                                               "libe/0.1"),
+                                      install=False)
+
+        assert type(deps_graph.error) == GraphConflictError
+
+        # Build requires always apply to the consumer
+        self.assertEqual(6, len(deps_graph.nodes))
+        app = deps_graph.root
+        libd = app.dependencies[0].dst
+        libe = app.dependencies[1].dst
+        libb = libd.dependencies[0].dst
+        libc = libe.dependencies[0].dst
+        cmake1 = libb.dependencies[0].dst
+
+        self._check_node(app, "app/0.1@", deps=[libd, libe], dependents=[])
+        self._check_node(libd, "libd/0.1#123", deps=[libb], dependents=[app])
+        self._check_node(libe, "libe/0.1#123", deps=[libc], dependents=[app])
+        self._check_node(libb, "libb/0.1#123", deps=[cmake1], dependents=[libd])
+        self._check_node(libc, "libc/0.1#123", deps=[], dependents=[libe])
+        self._check_node(cmake1, "cmake/0.1#123", deps=[], dependents=[libb])
+
     def test_tool_requires(self):
         # app -> libb -(br public)-> protobuf/0.1
         #           \--------------> protobuf/0.2
@@ -695,7 +811,7 @@ class PublicBuildRequiresTest(GraphManagerTest):
         # node, headers, lib, build, run
         _check_transitive(app, [(libb, True, True, False, False),
                                 (protobuf_host, True, True, False, False),
-                                (protobuf_build, False, False, True, False)])
+                                (protobuf_build, False, False, True, True)])
 
     def test_tool_requires_override(self):
         # app -> libb -(br public)-> protobuf/0.1
@@ -726,7 +842,7 @@ class PublicBuildRequiresTest(GraphManagerTest):
         # node, headers, lib, build, run
         _check_transitive(app, [(libb, True, True, False, False),
                                 (protobuf_host, True, True, False, False),
-                                (protobuf_build, False, False, True, False)])
+                                (protobuf_build, False, False, True, True)])
         _check_transitive(libb, [(protobuf_host, True, True, False, False),
                                  (protobuf_build, False, False, True, True)])