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.

Sign up for GitHub

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

Fix export_sources for non-existent recipes in a local index #16776

Merged
merged 5 commits into from
Aug 20, 2024
Merged

Fix export_sources for non-existent recipes in a local index #16776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9473e21
Fix export_sources for non-existent recipes in a local index.
cynix Aug 3, 2024
da67372
Fix formatting
cynix Aug 3, 2024
3f079ea
refactored test
memsharded Aug 5, 2024
07da098
Merge branch 'develop2' into fix_export_sources
memsharded Aug 19, 2024
9c8faec
use try-except
memsharded Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions conans/client/rest_client_local_recipe_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
from conan.internal.cache.home_paths import HomePaths
from conans.client.cmd.export import cmd_export
from conans.client.loader import ConanFileLoader
from conans.errors import ConanException, PackageNotFoundException, RecipeNotFoundException
from conans.errors import ConanException, PackageNotFoundException, RecipeNotFoundException, \
ConanReferenceDoesNotExistInDB, NotFoundException
from conans.model.conf import ConfDefinition
from conans.model.recipe_ref import RecipeReference
from conans.util.files import load, save, rmdir, copytree_compat
Expand Down Expand Up @@ -76,7 +77,12 @@ def get_recipe(self, ref, dest_folder):
return self._copy_files(export_folder, dest_folder)

def get_recipe_sources(self, ref, dest_folder):
export_sources = self._app.cache.recipe_layout(ref).export_sources()
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not necessary in get_recipe() above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@cynix cynix Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the contribution and the feedback!

export_sources = self._app.cache.recipe_layout(ref).export_sources()
except ConanReferenceDoesNotExistInDB as e:
# This can happen when there a local-recipes-index is being queried for sources it
# doesn't contain
raise NotFoundException(str(e))
return self._copy_files(export_sources, dest_folder)

def get_package(self, pref, dest_folder, metadata, only_metadata):
Expand Down
32 changes: 32 additions & 0 deletions test/functional/test_local_recipes_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,35 @@ def package_info(self):
# Finally lets remove the remote, check that the clone is cleared
c.run('remote remove local')
assert "Removing temporary files for 'local' local-recipes-index remote" in c.out

def test_not_found(self):
"""testing that the correct exception is raised when a recipe is not found
"""
repo1_folder = temp_folder()
repo2_folder = temp_folder()
config_yml = textwrap.dedent("""\
versions:
"0.1":
folder: all
""")
conanfile = textwrap.dedent("""\
import os
from conan import ConanFile
from conan.tools.files import copy

class PkgRecipe(ConanFile):
name = "pkg"

def export_sources(self):
copy(self, "*", src=self.recipe_folder, dst=self.export_sources_folder)
""")

save_files(repo2_folder, {"recipes/pkg/config.yml": config_yml,
"recipes/pkg/all/conanfile.py": conanfile,
"recipes/pkg/all/pkg.h": gen_function_h(name="pkg")})

c = TestClient()
c.run(f"remote add local1 '{repo1_folder}'")
c.run(f"remote add local2 '{repo2_folder}'")
c.run("install --requires=pkg/0.1 --build=missing")
assert "Install finished successfully" in c.out