Fix export_sources for non-existent recipes in a local index

[cynix]

Previously, a local recipe index client would raise a different exception than a REST client when exporting sources for a non- existent recipe, which causes a hard failure rather than allowing the caller to move on to the next remote. This patch makes their behaviours consistent and raise a RecipeNotFoundException instead.

Changelog: Bugfix: Fix export_sources for non-existent recipes in a local recipes index.
Docs: Omit

Fixes #16777

• 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.

[CLAassistant]

All committers have signed the CLA.

[memsharded]

Thanks for your contribution @cynix

I have done a couple of changes to the test, checked that it still reproduces the issue without the fix and passes with the fix:

• Making the test faster, no need to actually compile anything
• Highly reduced the conanfile.py and the associated files
• used a conan install --requires instead of a conan create

I still need to properly fully understand the fix and investigate if there could be other side or related issues, but I have annotated the PR for next 2.7 to try to move it forward for next release if everything is fine.

As a side note: the layout of the sources located several levels above the recipe conanfile.py looks like an anti-pattern. The local-recipes-index is intended for third-party code, and as such, it doesn't make much sense that it has some C++ code in the repo (it is typically downloaded in source()), and it there is some code, like patches, they should live in the current recipe folder not above.

[cynix]

Hi @memsharded, thanks for reviewing and cleaning up the test. I also thought it's weird for export_sources to access files outside the recipe dir, but the test was mostly a copy-paste of an existing test in the same file and I didn't want to change it too much 😅 apologies for the laziness.

[memsharded]

apologies for the laziness.

No need to apologize 🙂 , and no laziness at all. You did a great job providing a good unit test that covers the case, fails without the fix and works with the fix, that is fantastic. It is our job as maintainers to think about other aspects like the cost of running the full suite, etc.

[cynix]

Is this not necessary in get_recipe() above?

[memsharded]

Hi @cynix

I am trying to understand the root cause and the necessary changes to fix it.

So far, I have only been able to make it fail for the get_recipe_sources() case, when Conan iterates the remotes looking for the sources of a given (already existing) recipe. The above get_recipe() seems to be sufficiently protected by previous checks, but the sources one wasn't. As you can see, the test keeps working (we do quite test-focused development, if there is something else that could be failing in the get_recipe() it would be necessary another test that makes it fail).

According to the above, I have removed the extra calls (to avoid extra cost, those will do extra queries to the DB) and just converted the exception to make it work.

Have you been able to test the changes against your full code? Does it work? If not, maybe you can try to provide a new test that makes it fail again?

[cynix]

You're right, I tested this in my full setup and it does address the issue just fine, as far as I can tell.

But if you have some time, I'd appreciate if you could help me understand why get_recipe was already "protected" but get_recipe_sources was not, and whether this is a symptom of a bigger issue. It seems the calling code should have an idea of which remote a recipe comes from (after all, only one remote can possibly have what we consider to be the "latest" revision, right?). Once we've determined which remote a recipe comes from, should we then only ask that remote for sources? Does it make sense to still ask all remotes? When I was originally looking into this issue, an alternative I considered was to change the caller to pass in a single remote (e.g. node.remote) instead of the full list of enabled remotes, but I wasn't sure if that's intended or not.

[memsharded]

Sure. In the DepsGraphBuilder and ConanProxy class, the logic is more or less:

• Do first a ref = self._remote_manager.get_latest_recipe_reference(reference, remote) to query about the existing reference in the remote.
• If there is such a reference, then do a remote_manager.get_recipe() to get it

While the remote_manager.get_recipe_sources() is called in a different place (only when something needs to build); protected with:

def _try_get_sources(ref, remote_manager, recipe_layout, remote):
    try:
        remote_manager.get_recipe_sources(ref, recipe_layout, remote)
    except NotFoundException:
        return

And this _try_get_sources is iterated for every remote, without checking first if the ref exists in the remote. This is the operation that was not raising a NotFoundException and breaking that case.

[memsharded]

To further clarify, the first resolution of the ref in the graph resolution is often giving a reference without recipe-revision, and returning the full reference with recipe revision in ref = self._remote_manager.get_latest_recipe_reference(reference, remote). The get_recipe_sources() already works with a fully defined reference with recipe-revision, so no need to query for the latest one, just ask the server "give me the sources of this specific recipe revision, because I need them", and it can directly manage the NotFoundException, as it is more efficient than doing 2 calls, one to check the existence, then another to retrieve the sources.

[memsharded]

Please let me know if this clarifies a bit, and we can proceed to merge it. Thanks!

[cynix]

I see, so once you have a full reference, you're happy to get the sources from any remote that contains that exact ref, because in theory they're all fungible? That makes sense, but since resolving the full ref means you now know which remote it came from, isn't it still more efficient to just go directly to that remote for the sources rather than iterating through the full list again? Is there ever a case where a remote may provide the full ref but is unable to provide the sources?

But based on the assumption that the sources are all fungible, it seems your latest change would indeed work as intended, and looks nicer than what I did before, so I'm of course happy for it to be merged :)

[memsharded]

Exactly, the recipe-revision guarantees that you are getting the same sources irrespective of the remote, so the first one containing it will be good.

That makes sense, but since resolving the full ref means you now know which remote it came from, isn't it still more efficient to just go directly to that remote for the sources rather than iterating through the full list again?

Yes, but Conan doesn't keep that memory/state. It did in Conan 1, but we removed it as messy (issues when remotes are removed, disabled, etc). So this is a simple enough, and retrieving the sources is almost always for building, which is very expensive, so a couple of extra api calls is neglegible.

Is there ever a case where a remote may provide the full ref but is unable to provide the sources?

No, that would be a corrupted package recipe.

[memsharded]

thanks for the contribution and the feedback!