[bug] order of calls to self.requires changes the resulting dependency graph?

[puetzk]

Describe the bug

Conan version 2.22.1

How to reproduce it

lib_a/conanfile.py

from conan import ConanFile

class ConanLibA(ConanFile):
    name = "lib_a"
    package_type = "header-library"

conan export lib_a --version 1.0
conan export lib_a --version 1.1
conan export lib_a --version 1.2

lib_b/conanfile.py

from conan import ConanFile

class ConanLibB(ConanFile):
    name="lib_b"
    version="1.0"
    package_type = "static-library"

    def requirements(self):
        self.requires("lib_a/[>=1]")

conan export lib_b

lib_c/conanfile.py

from conan import ConanFile

class ConanLibC(ConanFile):
    name="lib_c"
    version="1.0"
    package_type = "shared-library"

    def requirements(self):
        self.requires("lib_b/1.0", visible=False)
        self.requires("lib_a/1.1", visible=True)

Happy path (this is the expected outcome)

conan graph info lib_c --format html > graph.html --no-remote

Requirements
lib_a/1.1#6a0c16aa1e2f729b87c13ce846c6fd24:da39a3ee5e6b4b0d3255bfef95601890afd80709 - Missing
lib_b/1.0#3ed7aadc114623df36e0697db429e9a8:fcdabf9ba48019c6920aec36e9d7e15d8446b678 - Missing

Buggy path? (I found this outcome unexpected and unwanted...)

swap the order of those two lines in requirements, so lib_a comes first:

     def requirements(self):
+        self.requires("lib_a/1.1", visible=True)
         self.requires("lib_b/1.0", visible=False)
-        self.requires("lib_a/1.1", visible=True)

Now the override/forced resolution doesn't happen, and I get a dependency graph with both lib_a/1.1 and lib_a/1.2

Requirements
lib_a/1.1#6a0c16aa1e2f729b87c13ce846c6fd24 - Cache
lib_a/1.2#6a0c16aa1e2f729b87c13ce846c6fd24 - Cache
lib_b/1.0#3ed7aadc114623df36e0697db429e9a8 - Cache
Resolved version ranges
lib_a/[>=1]: lib_a/1.2

Seemingly now the usage of lib_a/1.1 doesn't get propagated to its upstream lib_b, so the version range in lib_b just resolved to the latest (lib_a/1.2). Now both versions of lib_a are visible in the the self.dependencies of lib_c if I add

    def generate(self):
        for require, dependency in self.dependencies.items():
            self.output.info(f"{require} -> {dependency}")

conan install lib_c --no-remote --build missing

conanfile.py (lib_c/1.0): lib_a/1.1, Traits: build=False, headers=True, libs=False, run=False, visible=True -> lib_a/1.1
conanfile.py (lib_c/1.0): lib_b/1.0, Traits: build=False, headers=True, libs=True, run=False, visible=False -> lib_b/1.0
conanfile.py (lib_c/1.0): lib_a/1.2, Traits: build=False, headers=False, libs=False, run=False, visible=False -> lib_a/1.2

This seems wrong in several ways:

1. The upstream/downstream relationships certainly matter, but I did not expect the order sibling edges (lib_c -> lib_a and lib_c -> lib_b) are added to change the resulting dependency graph. Nothing in https://docs.conan.io/2/reference/conanfile/methods/requirements.html suggests that order requirements are listed within a single recipe matters... just upstream/downstream relationships.
2. As I understand it, visible=False is supposed to affect whether a require is propagated downstream. So I didn't expect that to change anything about what is (or isn't) present in the current recipe's self.dependencies, or what gets forced upstream.
3. The latter outcome (having lib_c depend directly on lib_a/1.1, and transitively on lib_a/1.2 via lib_b) would lead to undefined behavior in linking lib_c - the object files in lib_b (a static-library) will have embedded symbols from lib_a/1.2 (a header-library), while sources directly compiled in lib_c will embed lib_a/1.1, so linking these both into lib_c's shared module is (probably) an ODR violation.

[puetzk]

The case I minimized this out of is likely one where
https://docs.conan.io/2/devops/vendoring.html might be a better tool than visible=False (a COM InProcServer dll which has static libraries embedded and hidden inside its binaries) - but it also has a few shared libraries that (currently) need to propagate run=True, and there's nothing like vendor="embed", it seems to be all or nothing right now. I should be able to get things to a shape where "all" is correct by (copy the runtime DLLS into the package, taking advantage of [ISOLATION_AWARE_ENABLED](https://learn.microsoft.com/en-us/windows/win32/sbscs/isolating-components) and the fact CoLoadLibrary uses LOAD_WITH_ALTERED_SEARCH_PATH to make those shared-library references invisible too, and this is likely a good improvement to the component boundary.

But I found the current behavior surprising - I wasn't not expecting visible to affect the resolution of upstream requirements, or the order of siblings to matter, and it seemingly allowed me to create a conflicting version/ODR violation that wasn't diagnosed... so it feels like there's something weird going on.

[puetzk]

Just found another curious interaction - if I change the traits in lib_b to include transitive_headers=True

     def requirements(self):
-        self.requires("lib_a/[>=1]")
+        self.requires("lib_a/[>=1]", transitive_headers=True)

Then the then I get back the happy path (where lib_c's choice of lib_a/1.1 is forced upstream).

I tried this expecting it to maybe provoke CMakeDeps into noticing the conflicting requirement - but I definitely didn't expect that to change how which refs got picked in the graph. Feels like a clue, though I don't know how to interpret it.

transitive_libs=True did not do anything (though with lib_a as a header-library, it wouldn't actually have anything to propagate.

[memsharded]

Thanks for the extensive details to reproduce @puetzk, that helped a lot, I have been able to reproduce.

It seems a challenging issue derived from the depth-first dependency resolution algorithm, together with the visible=False trait.
Indeed, when you make the headers transitive, that forces back the propagation. The issue occurs when the transitive dependency is fully hidden because it is a header-only library, that is required as an implementation detail (because it is header and not transitive_headers=True), together with the downstream visible=False trait that kind of disconnects that branch from the rest of the graph. But that disconnection is not invariant with respect to the dependency resolution order.

I am investigating possible mitigations, found one idea, but might be very risky of breaking behavior, I keep working on this.

[memsharded]

It would also be interesting to understand the usage of self.requires("lib_b/1.0", visible=False). This visible=False in the example above is typically unnecessary, being lib_c a shared-library and lib_b a static library, lib_b linkage requirements is not propagated downstream, so why declaring it visible=False`, and the requirement to lib_a/1.1is not required also asvisible=False``?

[puetzk]

that's really just my attempt to provide a minimal reproduction with as little use of visible=False as I could manage (while still provoking the misbehavior). And I made the chain be header-library -> static-library -> shared_library so every requirement was "embedded" - which made the resolution affect package_id and made it into an ODR violation when linking lib_c (which is what convinced me it was a bug I should report, not just my misunderstanding the meaning of visible).

But yes, I can see how the result looks weird. Originally pretty much all the requirements, including lib_a were also visible=False. And lib_b was actually also a header-only library, though it didn't have the appropriate package_type set (noticing that is what clued me onto the interaction with transitive_headers).

The real scenario actually started out as is one of our many recipes that had to be split in conan 1.x, with an "internal" conanfile.txt (usually produced by conan_cmake_run) that had the requirements and generators used to build, and a wholly separate recipe (listing only the publicly-visible transitive requirements, e.g. the things that would have run=True) that is used with export-pkg to package and upload the resulting binaries for consumers. I have ended up with a lot of recipes using this anti-pattern, mostly on behalf of COM inprocserver libraries or tool applications that export no (or very minimal) symbols, but have lots of implementation details their consumers should not have to interact with. But I've lately been trying to figure out ways make better use of conan 2's improved support for visible=False, vendor=True, skipped binaries, etc and try to get things back in a single recipe that can conan create, --build missing, etc.

https://docs.conan.io/2/devops/vendoring.html is close to what they want, but it's all-or-nothing, and sometimes there is a shared library whose run=True needs to come through in the virtualenv generator, or should be visible because Win32 LoadLibrary is a shared namespace in which loading "foo.dll" will get you the already-loaded one in the process. You can't easily isolate that (at least not without going to things like manifests and https://learn.microsoft.com/en-us/windows/win32/sbscs/isolating-components). So right now I've been playing with the idea of marking almost every dependency (except those shared libraries) as visible=False, rather than the whole recipe as vendor=True, as a way to merge the producer and consumer recipes back together.

[memsharded]

I am experimenting in #19286 with a new consistent trait (name might change) concept, that would allow to customize this without necessarily breaking users that might rely on the current behavior already.