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

trying to avoid touching remote when binary not found in -r=remote #9355

Merged
merged 8 commits into from Aug 24, 2021

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Jul 30, 2021

Changelog: Fix: Avoid checking other remotes when -r=remote is defined and revisions are activated and binary is not found in the defined remote.
Docs: Omit

Attempt to fix #9333, not sure if this is a bug or expected behavior, lets see if this change breaks something.

Close #9333

#REVISIONS: 1

Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

I don't see today why I introduced that "or" and doesn't look good to search other remotes if you specified "-r". Said that we are highly probably breaking something, but we have to try.
I'm adding the #REVISIONS: 1 to see if some test give us feedback.

@lasote
Copy link
Contributor

lasote commented Aug 2, 2021

As I expected there are some tests failing apparently not using -r. I'm going to check why.

@@ -50,7 +50,7 @@ def setUp(self):

def _get_header(self, requester, header_name):
hits = sum([header_name in headers for _, headers in requester.requests])
assert hits == 2 if self.revs_enabled else 1
assert hits <= 2 if self.revs_enabled else 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the removed: # FIXME: Here we hit the same remote we did before

@memsharded
Copy link
Member Author

I don't see today why I introduced that "or" and doesn't look good to search other remotes if you specified "-r". Said that we are highly probably breaking something, but we have to try.

Seems your fixes pass the new test and not break other existing tests, so it looks good. I understand this might be breaking some users abusing this behavior, but this totally reads like a bug, so I agree we need to try to fix it.

@lasote
Copy link
Contributor

lasote commented Aug 5, 2021

Yes, I iterated a bit pushing commits until I finally find something that doesn't look very breaking.

@memsharded memsharded requested a review from czoido August 23, 2021 22:25
Copy link
Member Author

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This will conflict with @czoido work on updates for Conan 2.0, asking for a review, just in case.

Copy link
Contributor

@czoido czoido left a comment

Choose a reason for hiding this comment

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

This makes sense, I'll update my develop2 PR according to this when merged

@memsharded memsharded merged commit 6d7642d into conan-io:develop Aug 24, 2021
@memsharded memsharded deleted the fix/remote_revisions_touched branch August 24, 2021 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] What is the expected behavior of conan install --remote REMOTE
3 participants