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] Regression from v2.7.1: "Required package 'nlohmann_json' not in component 'requires'" #17164

Closed
DoDoENT opened this issue Oct 15, 2024 · 19 comments
Assignees
Milestone

Comments

@DoDoENT
Copy link
Contributor

DoDoENT commented Oct 15, 2024

Describe the bug

I have a package that test-depends on nhlohmann_json:

    def requirements(self):
        self.requires('something/[~1]@company/main', transitive_headers=True)

    def build_requirements(self):
        self.test_requires('nlohmann_json/3.10.4')

    def package_info(self):
        self.cpp_info.libs = ["MyLib"]
        self.cpp_info.requires = [
            'something::a_component',
        ]

When creating the package with conan v2.8.0, I get the error Required package 'nlohmann_json' not in component 'requires' after package gets built and test_package is executed to test the packaging.

As you can see, nlohmann_json is just a test dependency, so it doesn't need to be listed in self.cpp_info.requires.

Downgrading to v2.7.1 helps fixing the issue (v2.7.1 correctly builds and tests the package).

How to reproduce it

No response

@memsharded
Copy link
Member

Hi @DoDoENT

Thanks for your report.

I am trying to reproduce, I have done a unit test replicating your scenario above in #17165, but it seems to be working fine. Could you please have a look and check what could be missing in the test? Thanks.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 15, 2024

I really don't know. I've been trying to create the minimal reproducer for last couple of ours, and so far it has 6 packages with interconnections that resemble my real project but it doesn't reproduce the issue.

However, the real project with 300+ nodes in the dependency graph, reproduces it. Can I somehow provide you with graph data from my real project that may have any meaning to you for debugging?

@memsharded
Copy link
Member

I have done a full diff between 2.8 and 2.7.1 and I cannot find anything that would be modifying something regarding the components or this specific check. The only possible related changes could be:

  • Using [replace_requires]
  • A fix that was done when using sef.requires("dep/version", package_id_mode="something"), that caused wrong package_id in Conan <2.7 and then with Conan 2.8 it would require a new package_id and a new build from source, and that could be unveiling some previous issue that is not firing in 2.7 because the package is not being built from source.

The first step would be to compare the result of conan install in both cases, the terminal output. Which dependencies are being build and what not. If you can post the full output of the command with 2.7 and 2.8, that could have some hints.

Then, I think the best would be to instrument a bit the Conan code, run in 2.7.1 and 2.8.0 and compare.

First, lets try to report the direct dependencies in build_info.py, in def check_component_requires(self, conanfile):

diff --git a/conans/model/build_info.py b/conans/model/build_info.py
index 4505075e6..ec7e99d46 100644
--- a/conans/model/build_info.py
+++ b/conans/model/build_info.py
@@ -653,7 +653,8 @@ class CppInfo:
         # So consumers can keep their ``self.cpp_info.requires = ["pkg_name::comp"]``
         direct_dependencies = [r.ref.name for r, d in conanfile.dependencies.items() if r.direct
                                and not r.build and not r.is_test and r.visible and not r.override]
-
+        conanfile.output.info(f"Checking component requires")
+        conanfile.output.info(f"DIRECT DEPENDENCIES: {direct_dependencies}")
         for e in external:
             if e not in direct_dependencies:
                 raise ConanException(

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 15, 2024

conan-output.zip

I've added the log from your diff to both v2.7.1 and v2.8.0 installations I have on my machines and ran conan create . for my real project with both v2.8.0 and v2.7.1.

The terminal outputs are attached.

In the diff I see that v2.8.0 thinks that nlohmann_json is a direct dependency of document_verification_recognizer, even though it's not - it's a test_requires dependency. v2.7.1 correctly discovers that.

@memsharded
Copy link
Member

Ok, yes, can you please share the dependency graphs in json with conan graph info for both versions?

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 15, 2024

conan-graph-info.zip

Sure. Diff on those two files is empty, meaning that both v2.7.1 and v2.8.0 generated the same graph 🤔

@memsharded
Copy link
Member

That is quite unexpected. Maybe the test_package/conanfile.py is injecting some extra requirements and logic that could be affecting and making it different at test-package time? Can you please share it?

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 15, 2024

Sure:

from os.path import join

from conan import ConanFile
from conan.tools.files import copy


class DocumentVerificationRecognizerPackageTest(ConanFile):
    python_requires = 'conanfile_utils/[~1]@microblink/main'
    python_requires_extend = 'conanfile_utils.MicroblinkConanFile'

    def requirements(self):
        self.requires(self.tested_reference_str)
        self.requires('keygen/[~12]@microblink/main')
        self.requires('version_and_paths/[~1]@microblink/main')

    def generate(self):
        # Copy all resource files from all dependencies to the `res`directory
        for dep in self.dependencies.values():
            if len(dep.cpp_info.resdirs) == 1:
                copy(self, "*", src=dep.cpp_info.resdirs[0], dst=join(self.build_folder, 'res'), keep_path=False)
            copy(self, "features_*", dep.package_folder, join(self.build_folder, 'features_cmake'))

        # Run the base class `generate` method
        super().generate()

    def test(self):
        self.mb_run_test('DocumentVerificationRecognizerPackageTest')

Neither keygen nor version_and_paths should have anything to do with nlohmann_json...

@memsharded
Copy link
Member

Can you please simplify that test_package, removing those (one at a time) extra requirements, to check what happens?
As the build of the test_package would break, you can drop the rest of the test_package, drop the python_requirements, drop the generate(), etc.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 15, 2024

Even a barebones test_package with only self.requires(self.tested_reference_str) triggers an error.

That's really weird.

@memsharded
Copy link
Member

So what does a conan install --requires=pkgname/version after a conan create . -tf="" (to disable the test_package) does?
Does it work correctly?

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 16, 2024

🤔 , the error is thrown even when creating a package with -tf="", so it appears it's not related to the actual test_package.

@memsharded
Copy link
Member

I saw in the graph.json information that you reported that the "root" node contains:

"181": {
                        "ref": "nlohmann_json/3.10.4",
                        "run": false,
                        "libs": true,
                        "skip": false,
                        "test": false,
                        "force": false,
                        "direct": true,
                        "build": false,
                        "transitive_headers": true,
                        "transitive_libs": null,
                        "headers": true,
                        "package_id_mode": "minor_mode",
                        "visible": true
                    },

As a dependency.

According to this, the nlohmann_json/3.10.4 dependency is a direct one, as reported in "direct": true, ,but with "test": false,, so it is not detected as a test-requires, which could happen if it is also required as an indirect regular requirement.

This took me to a possible change in 2.8, related to the replace_requires, that could be related. I am investigating it, thanks very much for the details and the continuous investigation and feedback.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 16, 2024

According to this, the nlohmann_json/3.10.4 dependency is a direct one, as reported in "direct": true, ,but with "test": false,, so it is not detected as a test-requires, which could happen if it is also required as an indirect regular requirement.

That's also my hunch, although I couldn't reproduce the issue with smaller example.

The nlohmann_json is a regular dependency of core_utils::benchmark component, which is a dependency of recognizer::lib_recognizer component, which is a dependency of blinkid_generic_recognizer which is a direct dependency, together with nlohmann_json as a test dependency.

Theoretically, if I remove self.test_requires('nlohmann_json/3.10.4') from top-level dependencies, I will still get it transitively via the path above, and I just tested that I don't get the error in this case, even with conan v2.8.0.

However, that change would not be correct long-terms because nlohmann_json is indeed a top-level test dependency, whilst recognizer::lib_recognizer -> core_utils::benchmark is currently just a workaround for an issue we have in internal code, but in the long term this edge in graph should go away, and then that would break the top-level package as nlohmann_json wouldn't be available anymore.

I'm not using the replace_requires feature - I've seen it in release notes of some release but didn't read the documentation about it thoroughly.

@memsharded
Copy link
Member

memsharded commented Oct 16, 2024

Can you please double check that the above conanfile.py except is correct?

I have been able to reproduce it with:

            def requirements(self):
                self.test_requires("liba/1.0")
                self.requires("libb/1.0")

That is, declaring the test_requires() in the requirements() method and before other dependencies instead of declaring it in build_requirements().

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 16, 2024

According to the documentation, requirements and build_requiremets are just called one after another and don't make any difference.

But, as a matter of fact, our test_requires was correctly put into build_requirements function:

    def requirements(self):
        self.requires('blinkid_generic_recognizer/[~10]@microblink/main', transitive_headers=True)
        self.requires('ml_models_document_verification/[~4]@microblink/main')
        self.requires('ml_models_image_capture/[~3]@microblink/main')
        self.requires('ml_image_capture/[~1]@microblink/main')

        if not self.is_cross_compiling:
            # VA request
            self.requires('cpr/[~1]@microblink/main')
            self.requires('model_protection/[~2]@microblink/main')

            # Model Proxy
            self.requires('mlp_model_proxy/[~0]@microblink/main')

    def build_requirements(self):
        if not self.is_cross_compiling:
            self.test_requires('crow/[~1]@microblink/main')
            self.test_requires('nlohmann_json/3.10.4')

        self.test_requires('mls_testing/[~9]@microblink/main')
        self.test_requires('keygen/[~12]@microblink/main')
        self.tool_requires('compress_utility/[~1]@microblink/main')

@memsharded
Copy link
Member

I think I might have a fix, I am running some tests.
If you want to give it a try against your project:

diff --git a/conans/model/requires.py b/conans/model/requires.py
index d9a9dbc76..cb819d74e 100644
--- a/conans/model/requires.py
+++ b/conans/model/requires.py
@@ -254,6 +254,7 @@ class Requirement:
         self.transitive_libs = self.transitive_libs or other.transitive_libs
         if not other.test:
             self.test = False  # it it was previously a test, but also required by non-test
+        self.is_test = self.is_test or other.is_test
         # package_id_mode is not being propagated downstream. So it is enough to check if the
         # current require already defined it or not
         if self.package_id_mode is None:
@@ -344,6 +345,7 @@ class Requirement:

         if self.test:
             downstream_require.test = True
+        downstream_require.is_test = require.is_test

         # If the current one is resolving conflicts, the downstream one will be too
         downstream_require.force = require.force

If this is confirmed, we will release a 2.8.1 soon.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Oct 16, 2024

I confirm this patch works on my project.

Thank you for the fix.

@memsharded
Copy link
Member

Closed by #17174 for next Conan 2.8.1

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

Successfully merging a pull request may close this issue.

2 participants