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

Fix 6483 do not download binary for conan info #6487

Conversation

monsdar
Copy link
Contributor

@monsdar monsdar commented Feb 6, 2020

Changelog: Fix: Do not download binary data during conan info when no remote is explicitly given by the user. Fixes #6483
Docs: Not necessary??

  • 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.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@monsdar
Copy link
Contributor Author

monsdar commented Feb 6, 2020

Seems the Git Log got messed up from within my fork... The older commits listed are all changes already merged into Conan or trivial (me updating my fork to the latest version of Conan). In one case I used a different Username (my real name), so it looks like two users have worked on this PR -.-

@@ -98,9 +98,9 @@ def _evaluate_cache_pkg(self, node, package_layout, pref, metadata, remote, remo

def _evaluate_remote_pkg(self, node, pref, remote, remotes):
remote_info = None
if remote:
if remotes.selected:
Copy link
Member

Choose a reason for hiding this comment

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

This is breaking most of the binary-remote logic in Conan.

If no remote is explicitly selected, the location of the binary package is:

  • The one defined from the binary package, if it was previously downloaded
  • The one defined for the recipe, if the recipe is associated with a remote.

As you can see in the CI, a ton of tests are broken by this.

We need to understand better:

  • If this is a regression, it seems it is not, the behavior has always been there
  • I'd say that the binary is not really downloaded, but the server is actually queried for the information of it. That is what the conan info command does. The title might be a bit misleading.
  • Disabling all remotes indeed should completely avoid contacting them, if true, that could be a bug, but not related to the remotes.selected specification of remote.

@jgsogo
Copy link
Contributor

jgsogo commented Feb 7, 2020

@monsdar Thanks for trying to address the performance issue you detected in #6483 It looks like it requires a little bit more investigation, maybe, instead of using the remotes.selected it is about checking the activated/deactivated flag associated to them

@monsdar
Copy link
Contributor Author

monsdar commented Feb 7, 2020

Yeah, saw that it was breaking a lot of things as well. Would've been too easy if it was nothing more than this easy fix.

The question is if conan info really needs to gather info about remote binaries or not.

  • If it does, perhaps there could be a parameter to control whether to query remotes or not
  • If it doesn't perhaps it's possible to internally avoid querying the remotes for binary data

@monsdar
Copy link
Contributor Author

monsdar commented Feb 7, 2020

Let's rather continue the discussion within the Issue

@CLAassistant
Copy link

CLA assistant check
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.

✅ monsdar
❌ Nils Brinkmann


Nils Brinkmann seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@memsharded
Copy link
Member

PR #12807 should close this PR, it has been a long time, and it didn't move forward.
#12807 contains a rationale of the current behavior, which seems correct and expected, and not hitting the servers if the packages are in cache, and only hitting if they are not in cache.

@memsharded
Copy link
Member

Closed by #12807, please check also related #12808 (for Conan 2.0)

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 this pull request may close these issues.

[bug] Slow package resolution from local cache when remotes are defined
4 participants