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

[bug] test_requires causing cycle/loop in the dependency graph #15412

Closed
DoDoENT opened this issue Jan 8, 2024 · 18 comments · Fixed by #15413
Closed

[bug] test_requires causing cycle/loop in the dependency graph #15412

DoDoENT opened this issue Jan 8, 2024 · 18 comments · Fixed by #15413
Assignees

Comments

@DoDoENT
Copy link
Contributor

DoDoENT commented Jan 8, 2024

Environment details

  • Operating System+version: macos 14.2.1
  • Compiler+version: apple-clang 15.0.0
  • Conan version: 2.0.16
  • Python version: 3.11.7

Steps to reproduce

It appears that test_requires dependencies somehow get leaked and thus causing the cycles/loops when they shouldn't be.

Here is my setup:

abseil/20230802.1.0@company/stable test_requires gtest/[~1]@company/stable
re2/20231101.0.0@company/stable requires abseil/[^20230802]@company/stable and test_requires gtest/[~1]@company/stable

gtest/1.11.0@company/stable has no dependencies
gtest/1.14.0@company/stable requires both abseil/[^20230802]@company/stable and re2/[^20231101]@company/stable.

The build order is the following:

  • first, we built gtest/1.11.0@company/stable
  • then, we built abseil/202300802.1.0@company/stable which used gtest/1.11.0@company/stable for its testing purposes
  • then, we built re2/20231101.0.0@company/stable which used gtest/1.11.0@company/stable for its testing purposes and has a real dependency on abseil/20230802.1.0@company/stable
  • then, we updated gtest to v1.14 and added dependencies to abseil and re2, and built the new gtest package.
  • then, an attempt to use gtest/1.14.0@company/stable as test_requires from some other package (e.g. opencv), fails with the following error:
|| ERROR: There is a cycle/loop in the graph:
||     Initial ancestor: gtest/1.14.0@company/stable
||     Require: gtest/1.14.0@company/stable
||     Dependency: gtest/1.14.0@company/stable

I don't see how this would be a cycle because:

  • opencv test_requries gtest, which requires abseil and re2 (which also requires abseil)
  • the fact that both abseil and re2 test_require gtest should not be leaked to opencv, as it's not relevant (it may appear in the lock file, but should not enter dependency resolution)

Currently, I can still build gtest/1.14 without abseil and re2 dependencies as they are still optional, and this is my workaround.

However, Google has announced that soon gtest will depend on abseil (which itself test-depends on gtest), so this issue needs to be resolved.

Logs

No response

@memsharded
Copy link
Member

memsharded commented Jan 8, 2024

Hi @DoDoENT

Thanks for your report.

So far it seems this is not a bug. The graph, defined as such, is causing an infinite cycle/loop in the dependency graph, not even related to test_requires definition, propagation, or traits.

The issue seems to be in the abseil/20230802.1.0@company/stable test_requires gtest/[~1]@company/stable. This need to be bounded to something lower than 1.14, otherwise the infinite loop happens, and the graph becomes chicken and egg, without being able to build any binary because all depend on non existing binaries. In other words, it is physically impossible to build(and test) abseil/20230802.1.0 that depends on gtest versions higher than the gtest version that requires abseil/20230802.1.0 itself to be built, and then the requirement gtest/[~1]@company/stable would be badly form and needs to be gtest/[>=1.0 <1.14]@company/stable to be realizable

The following test shows that limiting the test-require in abseil works and can correctly construct the dependency graph:

    def test_test_require_loop(self):
        # https://github.com/conan-io/conan/issues/15412
        self._cache_recipe("gtest/1.11", GenConanfile())
        self._cache_recipe("abseil/1.0", GenConanfile().with_test_requires("gtest/[>=1 <1.14]"))
        self._cache_recipe("gtest/1.14", GenConanfile().with_requires("abseil/1.0"))
        deps_graph = self.build_graph(GenConanfile("opencv", "1.0").with_test_requires("gtest/1.14"))

        self.assertEqual(4, len(deps_graph.nodes))
        opencv = deps_graph.root
        gtest14 = opencv.dependencies[0].dst
        abseil = gtest14.dependencies[0].dst
        gtest11 = abseil.dependencies[0].dst

        self._check_node(opencv, "opencv/1.0@", deps=[gtest14], dependents=[])
        self._check_node(gtest14, "gtest/1.14#123", deps=[abseil], dependents=[opencv])
        self._check_node(abseil, "abseil/1.0#123", deps=[gtest11], dependents=[gtest14])
        self._check_node(gtest11, "gtest/1.11#123", deps=[], dependents=[abseil])

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 8, 2024

I understand your point when building the abseil, but I still don't see why is this detail leaked to 3rd party packages, such as opencv.

Isn't the purpose of test_requires to only be resolved at test time, and not when building the package on-demand by dependency (i.e. in case if opencv may require building abseil by-dependency via gtest)? In conan v1 we had self.develop for that matter (i.e. it allowed me to add test-only dependencies only while developing the package, but not when building it by dependency).

@memsharded
Copy link
Member

I understand your point when building the abseil, but I still don't see why is this detail leaked to 3rd party packages, such as opencv.

The dependency graph is always computed first, even before computing which binaries do exist. It is not possible to decide: I already have a binary for this package, then I no longer expand the dependency graph, because the binaries themselves depends on the versions of the dependencies. Then it is necessary to compute the whole dependency graph of versions before computing and deciding which binaries do exists.

test_requires imply:

  • It is a "host" requires
  • It is not propagated to the consumers visible=False.

Still the whole graph, including tool_requires and test_requires is always fully expanded first.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 8, 2024

test_requires imply:

  • It is a "host" requires
  • It is not propagated to the consumers visible=False.
    Still the whole graph, including tool_requires and test_requires is always fully expanded first.

I understand that, and this brings me back to the question I think I had already on some other issue or even during tribe discussion: why are visible=False dependencies expanded?; or even better: why are test=True dependencies expanded?

I understand that visible=False dependencies need to be discovered during graph expansion in case binaries may be missing so they can be rebuilt (because graph expansion happens before package ID calculation). But, as far as I understand, for this very reason requirements traits distinguish between test dependencies (i.e. test=True) and non-test dependencies (i.e. test=False):

I think we should make a clear distinction between requirements specified as build=False, visible=False, test=False (i.e. build-only dependencies in the host context that should not be propagated downstream, but are required when building the package by dependency) and requirements specified as build=False, visible=False, test=True (i.e. build-only dependencies in the host context that should not be propagated downstream, are not required when building the package by dependency, but are required when developing the package (e.g. for running unit tests)).

@memsharded
Copy link
Member

memsharded commented Jan 8, 2024

I understand that, and this brings me back to the question I think I had already on some other issue or even during tribe discussion: why are visible=False dependencies expanded?; or even better: why are test=True dependencies expanded?

Because several reasons:

  • You don't know if you will need them or not, until you compute the package_id of the consumer
  • They can perfectly interact, collide, conflict with other "host" requires of the package
  • They can even affect indirectly the consumer package_id if users decide so in their package_id() methods

Delaying these to a later stage can be from impossible in some cases like the package_id ones, to quite confusing or problematic in others (like collisions with other "host" requires, because the graph has already been solved)

Furthermore the lack of locking in lockfiles when the graph was not fully expanded was a major pain in Conan 1.X. Also the missing information for buildInfo, package migrations from server to server in CI pipelines, etc. This resulted in the decision that the dependency graph is always fully computed in Conan 2.0. The cost of downloading one conanfile.py if the binary can be later skipped is negligible, and there are a lot of advantages.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 8, 2024

You don't know if you will need them or not, until you compute the package_id of the consumer

I'm 100% sure that test=True dependencies will never be needed when package is built by-dependency (i.e. in local cache). However, I agree with you that we can't know if we will need visible=False dependencies until the package_id of the consumer is computed.

They can perfectly interact, collide, conflict with other "host" requires of the package

For visible=False I agree. But for test=True I think it depends: they can interact, collide and conflict with other requirements if they are direct dependencies (i.e. my_package requires abseil/1.0 and test-requires gtest/1.14 which requires abseil/1.1 and this causes the conflict), but they should not for non-direct dependencies (i.e. my_package requires abseil/1.0 and opencv/4.9, while opencv/4.9 test-requires gtest/1.14 which requires abseil/1.1 - there should be no conflict here as neither gtest/1.14 nor abseil/1.1 should not be in the dependency graph of my_package - not even in case opencv/4.9 binaries are missing and need to be rebuilt by-dependency).

They can even affect indirectly the consumer package_id if users decide so in their package_id() methods

Again, completely makes sense for visible=False, but IMO test=True dependencies should never affect package_id by default (of course, if someone manually implements the package_id function incorrectly, conan cannot help there). Those dependencies are only needed for test/development purposes of the package and should not be leaked downstream.

This is not the case at the moment, which makes it difficult to understand the purpose of the test requirement trait - it appears that it has no effect... Even documentation does not clearly describe the difference between test and visible, although it clearly describes the visible trait, but fails to correctly describe the interactions between different traits.

If you take all combinations of visible and test, and further combine that with the build trait, you can see that there are some dark corners that should at least be better documented, if not having more reasonable behavior:

Here is my current understanding of the intentions of traits:

  • build=False, visible=False, test=False - a dependency in the host context, needed for building the package, be it by-dependency, or while developing. The dependency is not propagated downstream but participates in conflict resolutions and downstream packages can override it. The dependency is "consumed" by the package, i.e. becomes part of it. For example, a DLL or a "fat" static library.
  • build=False, visible=False, test=True - a dependency in the host context, needed for building the package while developing it, but not while building it by-dependency. The dependency is not propagated downstream, should not participate in conflict resolutions, and downstream packages cannot override it. The dependency is "not consumed" by the package, i.e. does not become part of it.
    • this is obviously not happening at the moment, so I'm missing something
  • build=False, visible=True, test=False - a "normal" dependency in the host context, propagated downstream, participates in conflict resolutions and downstream packages can override it. The dependency is "not consumed" by the package, i.e. does not become part of it.
  • build=False, visible=True, test=True - I'm not completely sure what should happen here and whether should this combination even be allowed.
  • build=True, visible=False, test=False - a "normal" tool-require dependency. Dependency is in the build context, not propagated downstream, but participating in conflict resolutions and downstream can override it.
  • build=True, visible=False, test=True - a "test-only" tool-require dependency. Dependency is in the build context, not propagated downstream, not participating in conflict resolutions and downstream packages cannot override it.
    • this is obviously not happening at the moment, so I'm missing something
  • build=True, visible=True, test=False - indicates a "host->build" crossing dependency. This dependency is in the build context, but propagated downstream, participates in conflict resolutions and downstream can override it. This scenario was discussed in this issue where we concluded that it "mostly works, except for cmake".
  • build=True, visible=True, test=True - again, I'm not sure what should happen here and whether should this combination be allowed.

@memsharded
Copy link
Member

I'm 100% sure that test=True dependencies will never be needed when package is built by-dependency (i.e. in local cache).

When you are expanding the graph, you don't even know what package binary to check for in the cache, because you don't have the package_id yet. There are users that want to parameterize their package_id on their test_requires, even if uncommon, it is still possible.

All the traits combinations that you are describing are still irrelevant to this problem. Conan 2.0 is designed to always expand the full dependency graph, the traits are just indicators about how some information is propagated down the graph, and it can affect how dependencies can conflict or how package_ids are computed, but it cannot and it will not inhibit the expansion of the dependency graph. The traits were never intended to avoid the expansion of certain dependencies in the graph, rather the opposite, to be able to model what binaries will be needed for a complete graph.

This issue of having only partial graphs, not fully expanded to include tool and test requires was one of the most common and problematic issues in Conan 1.X, and Conan 2.0 will not compute partial graphs (at least at the moment, there are plans to investigate further the repackage concept, that will behave as a non-expanded graph, but only for the purpose of final releases between teams, not for normal evaluation of dependency graphs), and it will always expand all dependencies.

Users that want to control the graph expansion, at their own responsibility, can introduce conditional requirements. Users can introduce or use options or confs to conditionally require test_requires if they decide so, and then it will be the user responsibility to manage them accordingly for possible flows that require such test_requires later.

This is not the case at the moment, which makes it difficult to understand the purpose of the test requirement trait - it appears that it has no effect...

It is not used by Conan propagation or linkage, but it is used to print it in the command output and it also allows users to clearly differentiate dependencies that are needed for testing and those that not, for example if some test_requires transitively depend on zlib, users creating a manifest of the "production" dependencies can know they don't depend on zlib.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 8, 2024

It is not used by Conan propagation or linkage, but it is used to print it in the command output and it also allows users to clearly differentiate dependencies that are needed for testing and those that not, for example if some test_requires transitively depend on zlib, users creating a manifest of the "production" dependencies can know they don't depend on zlib.

So, is there absolutely no difference between visible=False, test=True and visible=False, test=False dependencies (besides different output)?

If so, then please document that, because the current documentation implies that test trait should be used for a test requirement that is not propagated downstream (meaning, it cannot cause conflicts nor cycles, yet it does).

Also, this sentence:

for example if some test_requires transitively depend on zlib, users creating a manifest of the "production" dependencies can know they don't depend on zlib

contradicts the fact that zlib will still appear in the final package's conan.lock and will participate in conflict resolutions and package ID calculation.

@memsharded
Copy link
Member

If so, then please document that, because the current documentation implies that test trait should be used for a test requirement that is not propagated downstream (meaning, it cannot cause conflicts nor cycles, yet it does).

Sure, this can be better documented. The current documentation reads:

test
This requirement is a test library or framework, like Catch2 or gtest. It is mostly a library that needs to be included and linked, but that will not be propagated downstream.

It means that the includes and libraries ("included and linked") will not be propagated downstream. It doesn't mean that there cannot be cycles, but we can certainly clarify that in the docs.

To make it clear, even with visible=False you can easily create cycles:

app/1.0-(visible=False)->liba/1.0->(visible=False)->libb/1.0-(visible=False)->liba/1.0

No matter how much visible=False traits you define, this dependency graph cannot be built, it is an impossible infinite loop, chicken and egg. The meaning that "it cannot cause cycles" is not related to any trait, not test, not visible. Cycles in dependency graphs do not depend on visibility or traits.

contradicts the fact that zlib will still appear in the final package's conan.lock and will participate in conflict resolutions and package ID calculation.

A reference can appear in the conan.lock and still not be used in package_id calculation. But still users want to have their test-requires locked to a certain version, even if they don't affect the package_id, those are orthogonal problems, and it is not contradicting. Also, when a lockfile is used, there will be no conflicts, because a conflicted graph cannot produce a lockfile, and a lockfile guarantees that the same versions and revisions are used. So this doesn't appear contradicting either.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 8, 2024

The problematic part in the documentation that confuses both me and most of the people I work with (and talked about conan) is when you say "It's not propagated downstream". What we understand here is that "downstream will not see it". So, imagine then the confusion when this dependency causes conflicts or cycles downstream, where it's not supposed to be visible, because "it's not propagated".

Documentation needs to clearly define what "propagating downstream" actually means (i.e. only in C++ terms, the actual libraries don't get linked into the downstream dependency; but the conan package itself does get propagated and can cause conflicts and dependency cycles).

And then, we need to find a suggested solution for cases like mine - where the package truly does not want to propagate its dependencies downstream, meaning downstream will never see my dependencies - neither in the lock file, nor in the dependency graph, nor in build system integration, nor anywhere.

@memsharded
Copy link
Member

memsharded commented Jan 8, 2024

Documentation needs to clearly define what "propagating downstream" actually means (i.e. only in C++ terms, the actual libraries don't get linked into the downstream dependency; but the conan package itself does get propagated and can cause conflicts and dependency cycles).

I don't think it will create conflicts. test_requires can easily use different versions of their dependencies without conflicts. I think this is only an issue for these cycles in dependencies.

So, if we go back to the root issue reported in this thread, the case is the "cycle/infinite-loop" of dependencies, which is a very different case than conflicts. I understand that propagation and dependency graph computation is not completely obvious in all cases, and maybe there is error to improve the error message, but if we state the root problem of this issue I think we can state that the dependencies:

abseil/202300802.1.0@company/stable -> gtest/1.14.0@company/stable -> abseil/202300802.1.0@company/stable -> 

Is an infinite loop, that cannot be realized, and as such is an error, no matter what any package manager or tool tries to do, no matter what traits or visibility we try to define to Conan. That is an impossible infinite loop. It is not even about the propagation downstream, the graph cannot be computed, because it would be infinite.

And that the only solution to break that infinite loop is not depend freely on gtest/[~1] which is an invalid range for this case, but make abseil to depend on the previous gtest/1.11 version (or something like <1.14).

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 9, 2024

but if we state the root problem of this issue I think we can state that the dependencies: ... Is an infinite loop, that cannot be realized, and as such is an error, no matter what any package manager or tool tries to do,

I agree. However, I would like this to be contained to abseil <-> gtest, but not also leaked to every single dependency that test_requires gtest.

And that the only solution to break that infinite loop is not depend freely on gtest/[~1] which is an invalid range for this case, but make abseil to depend on the previous gtest/1.11 version (or something like <1.14).

I actually tried that yesterday, but it quickly escalated. At first, I contained the dependency on gtest in abseil and re2, but then discovered that my gtest also test_requires our internal logging library which then additionally requires several more libraries, and all of them test_require gtest. That means that I would need to keep a list of dozen libs that must never use gtest newer than 1.11 and prevent juniors from accidentally upgrading it. Even worse, this is not future-proof as it basically disables upgrading gtest for these libraries forever.

For now, my solution is to simply build gtest without abseil/re2 dependencies, as they are still optional, and after they become mandatory, I will try to find some other solution outside of conan.

@memsharded
Copy link
Member

gtest also test_requires our internal logging library which then additionally requires several more libraries, and all of them test_require gtest

How is this possible? gtest is the google open source library? why would you be adding extra testing to gtest itself? Like you are augmenting the test suite of gtest itself with extra tests?

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 10, 2024

Like you are augmenting the test suite of gtest itself with extra tests?

Yes. We are augmenting gtest with additional features (some are described in issue #15143). On top of gtest excellent capabilities for running pure C++ test, we added features that allow running same c++ tests on browsers, android devices (via our junit to test wrapper+generator), and iOS devices (via our adaptation of gtest to xcode adapter that additionally adds features that are specific to needs in our company).

So, we need to test all those additional features, so our gtest has its own tests.

(I really hope that we will be able to open source our augmentations one day, as they could be really beneficial to others as well).

@memsharded
Copy link
Member

memsharded commented Jan 10, 2024

I actually tried that yesterday, but it quickly escalated. At first, I contained the dependency on gtest in abseil and re2, but then discovered that my gtest also test_requires our internal logging library which then additionally requires several more libraries, and all of them test_require gtest. That means that I would need to keep a list of dozen libs that must never use gtest newer than 1.11 and prevent juniors from accidentally upgrading it. Even worse, this is not future-proof as it basically disables upgrading gtest for these libraries forever.

I am afraid that without knowing the details, it is difficult to understand the scope. It is not the dependency to gtest that cannot be upgraded, my test opencv use gtest/1.14 without issues. It is just the dependency of abseil to a gtest that is newer than itself that has to be limited. Any other package should be able to safely test-require gtest/[>=1 <2], shouldn't it? I don't see how they wouldn't be able.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Jan 11, 2024

I'll try to get back to this and maybe even create an open-source reproducer when I find some free time. At the moment I've opted into building gtest without abseil/re2 dependencies as this unblocked me for the time being...

@Ruwei-Liu
Copy link

Having a similar issue here:
test_require packages are propagating into my project's output and I really need this resolved.

My test depends on cgal, which depends on GMP/MPFR, and they are all LGPL licensed and conan center has only static libraries available for them. My project does not need CGAL, but it depends on GMP/MPFR which are supposed to be linked dynamically.

However, when I try to build my project, the dependency resolve to the static conan GMP/MPFR packages, thus my project will be statically linked to GMP/MPFR.

Now I have to remove CGAL from my conan file as a walkaround. Conan is a great tool and would very much appreciate that this could be resolved so I can get back to use Conan.

@memsharded
Copy link
Member

Hi @Ruwei-Liu

I think that what you are reporting is a different issue, could you please open it as an independent new issue? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants