Bug/19082 package list items swallows information if no binary revision is given

[ianda]

Changelog: Fix: conan pkglist find-remote shows prevs.
Docs: Omit
fixes: #19082

• Refer to the issue that supports this Pull Request.
• If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
• I've read the Contributing guide.
• I've followed the PEP8 style guides for Python code. (Too many changes to fix, would bloat the PR)
• I've opened another PR in the Conan docs repo to the develop branch, documenting this one. (Docs not affected)

This PR is sponsored by Bosch.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ memsharded
❌ ianda
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[memsharded]

Hi @ianda

Thanks very much for your contribution.

I had already started checking your issue #19082, and trying to reproduce.
I was trying to write some unit test first that captured the behavior, but I was struggling to understand the report, the expected inputs and outputs.

My test so far:

 def test_graph_binary_info(self):
        c = TestClient(default_server_user=True, light=True)
        c.save({"zlib/conanfile.py": GenConanfile("zlib", "1.0")})
        c.run("create zlib")
        c.run("upload zlib* -c -r=default")

        c.run("list *:* --format=json", redirect_stdout="mylist.json")
        print(c.load("mylist.json"))
        """ Input is:
        {
            "Local Cache": {
                "zlib/1.0": {
                    "revisions": {
                        "c570d63921c5f2070567da4bf64ff261": {
                            "timestamp": 1760361837.8450165
                        }
                    }
                }
            }
        }"""

        c.run("pkglist find-remote mylist.json --format=json --remote default")
        result = json.loads(c.stdout)
        assert len(result["default"]["zlib/1.0"]["revisions"]) == 1

It would be great to start with a test first (without the fixes), just to clarify on the expected vs. seen behavior. If not, it would be nice to clarify it in the ticket, so I can try to contribute the test myself. Many thanks!

[ianda]

Hi @memsharded,

I tried to highlight the issue better. Does it explain the problem well now? If not, feel free to ask again, here, in the issue or contact me on slack or via mail directly.

Regarding your test: in the mylist.json should be a package_id (I think the list command would create another output than what is in the string of your snippet). However, there the "info" sections need to be removed, otherwise it was working all along.

[memsharded]

Thank you, now I understand better.

I have been able to reproduce with the following test:

    def test_graph_binary_info(self):
        c = TestClient(default_server_user=True, light=True)
        c.save({"zlib/conanfile.py": GenConanfile("zlib", "1.0")})
        c.run("create zlib")
        c.run("upload zlib* -c -r=default")

        c.run("list *:* --format=json", redirect_stdout="mylist.json")
        # mylist.json contains "packages" field
        result = json.loads(c.load("mylist.json"))
        rev = result["Local Cache"]["zlib/1.0"]["revisions"]["c570d63921c5f2070567da4bf64ff261"]
        assert len(rev["packages"]) == 1
        assert "da39a3ee5e6b4b0d3255bfef95601890afd80709" in rev["packages"]

        c.run("pkglist find-remote mylist.json --format=json --remote default")
        result = json.loads(c.stdout)
        rev = result["default"]["zlib/1.0"]["revisions"]["c570d63921c5f2070567da4bf64ff261"]
        assert len(rev["packages"]) == 1
        assert "da39a3ee5e6b4b0d3255bfef95601890afd80709" in rev["packages"]

It would be necessary to sign the CLA, could you please have a look and sign it? Sometimes it appears unsigned if using a different email than the Github one. If not possible to sign the CLA, please let us know, close the PR, and we will take it from there to fix it.
Thanks!

[ianda]

Very good, I also fixed conan remove to handle the new way of iterating the PackageList. Shall I add your test snippet?

Regarding CLA; my company should have signed it, maybe the bot is not able to detect my e-mail address, or it was updated since singing. I'll have a look tomorrow.

[memsharded]

Very good, I also fixed conan remove to handle the new way of iterating the PackageList. Shall I add your test snippet?

Yes, a test snippet that captures the fix would be necessary. It should be a test that fails without the code changes and passes with the changes applied. Thanks!

Regarding CLA; my company should have signed it, maybe the bot is not able to detect my e-mail address, or it was updated since singing. I'll have a look tomorrow.

I don't think that the CLA can be signed by a company, here in Github this CLA can only be signed by the individual contributors.

[ianda]

Hi @memsharded,

Yes, a test snippet that captures the fix would be necessary. It should be a test that fails without the code changes and passes with the changes applied. Thanks!

I implemented the test based on your proposal. However, I check for several variants of input files, conan list {"*#*", "*:*", "*#*:*#*"}. It was working with the first and last pattern all along, only *:* failed. I verified that with the develop2 branch.

[memsharded]

Hi @ianda

I have added a test_graph_pkg_list_counter_example new test, that fails given the current changes.

I think maybe the issue is that there was an underlying hypothesis in the conan pkglist find-remote, is that the things that are to be found in the remotes have to be fully determined, including the recipe-revision and/or the package-revision.
The problem is not well defined if those revisions are not defined, like what the find-remote should return in that case, the full list of revisions?

The docs seem to already point in this direction:

For every exact occurrence in a remote matching the recipe, version, recipe-revision, etc, an entry in the resulting “package lists” will be added for that specific remote.

The intent was to find existing artifacts in the cache (which are exactly identified by its revisions) in different remotes. Not to be able to check if a generic package_id, existed in the remotes.

Which means there must be an exact coincidence of all inputs, including revisions, but as it was not intended to return more matches than the inputs, which the current changes are doing.

That doesn't mean that we are not willing to consider some improvements in this line, but we should be careful about the risks.

As there is some risk of breaking, I'd suggest to split the tests into the different use cases, and different potential fixes. One of them is the definition or not of recipe_revisions when not having package-ids at all, other is the definition of package-ids without package revisions, etc.
I'll try to do the refactor/split of the tests later today or tomorrow, to make this more explicitly.

[memsharded]

I have split the tests, some of them new, like conan list * case.
Some are failing, not covered by the fixes so far, and the counter-example test of the multiple package-revisions is also failing.

I have now enough insights to discuss with the team, to see what would be the expected behaviors, inputs and outputs for this feature, I think it is necessary some thinking and UX/UI design before proceeding with the code, please wait a bit, I'll update here when we have some feedback. Many thanks!

[ianda]

You've raised an excellent point regarding the underlying assumptions of conan pkglist find-remote. This touches upon the broader question of find-remote's precise purpose, especially given its functional proximity to conan list. While their behaviors and input paradigms differ slightly, my current reliance on find-remote is primarily due to conan list's current inability to accept a --list attribute with a package list as input. Should conan list gain this capability, find-remote could potentially be superseded, I think even for all use cases.

[ianda]

Hi @memsharded!

I just wanted to kindly check in to see if you’ve had a chance to discuss the matter with your team. Is there something I can support with?

[memsharded]

Hi @ianda

Yes! yesterday we talked, I had pending an update.
We are trying to move this to allow the find_remotes functionality also for partial information, without addition of extra information over the provided one. I am preparing an update on the PR, I will push it later when ready and let you know. Thanks!

[memsharded]

Done, I have updated the PR:

• Not modifying the conan list or conan remove functionality, that is a bit risky, could break other users and use cases
• Everything happens in the find_remote code, that handles all the cases of partial information
• Tests verify the relevant information comes in the response too

[memsharded]

Hi @ianda

The PR is green, but can't be merged yet, it seems you haven't signed the CLA above (sometimes it fails to identify signing if using different email for the commit than the Github one).
Please check the CLA, many thanks!

[memsharded]

Hi @ianda

We are about to close 2.22, if we would like this get into 2.22, it would be necessary to sign the CLA, please check it when possible. Thanks!