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

Make candidate download even lazier #9522

Merged
merged 3 commits into from Jan 29, 2021
Merged

Conversation

uranusjr
Copy link
Member

Fix #9516.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Pretty sure that this needs a 21.0.1.

/cc @pfmoore

@pfmoore
Copy link
Member

pfmoore commented Jan 27, 2021

Um, why? I can see that it's an improvement, but (a) is it a fix for a regression, and (b) why is it urgent enough to need an emergency release?

My criteria for a bugfix release are:

  • We introduced a bug in the release, and it's serious enough that people can't wait for the next release for a fix. Basically, it breaks people's builds.
  • We haven't made any significant non-bugfix changes to master, or the fixes are important enough to commit to the complexity/risk of cherry-picking the changes onto a branch.

Once this change is no longer in draft status, I'll review it and make a call on a 21.0.1. At the moment, this, and a possible packaging release are the two candidates, and neither is in a state to land yet. And the longer the delay, the more chance that releasing from master won't be possible (at which point I become a lot harder to convince - I simply don't have the free time to play with merge conflicts).

(It's worth noting that the packaging release doesn't meet my criteria above - so I'm already bending my own rules. Don't push me too far 🙂)

@uranusjr uranusjr marked this pull request as ready for review January 28, 2021 02:29
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

I've desk-checked this and it looks OK to me.

@pfmoore
Copy link
Member

pfmoore commented Jan 29, 2021

It looks like there will be a packaging release in the next few days, so 21.0.1 is likely to happen. So the question "does this need a 21.0.1" is no longer relevant - but someone needs to ensure this is merged before the release. I'm likely to do the release this weekend (assuming packaging gets released) and I'm not going to delay 21.0.1 for another week for this, nor am going to do a 21.0.2...

@pradyunsg
Copy link
Member

We introduced a bug in the release, and it's serious enough that people can't wait for the next release for a fix. Basically, it breaks people's builds.

I think (unnecessary) double downloads if you pass --upgrade is a bug that justifies a bugfix. 🙈

@pradyunsg pradyunsg merged commit 55d2969 into pypa:master Jan 29, 2021
@pradyunsg
Copy link
Member

Well, that's easy. :)

@pfmoore
Copy link
Member

pfmoore commented Jan 29, 2021

I think (unnecessary) double downloads if you pass --upgrade is a bug that justifies a bugfix.

Oh, it does fix that one? I hadn't worked out why this addressed that problem. Definitely a bug worth fixing, my point was that I don't think it was necessarily urgent enough to warrant a 21.0.1 (it's "only" an extra download after all). But that's irrelevant now, so that's good.

Comment on lines +31 to +32
This iterator is used the package is not already installed. Candidates
from index come later in their normal ordering.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo. I believe when is missing –" ... is used when the ..."

Copy link
Member Author

Choose a reason for hiding this comment

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

A pull request would be welcomed :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip 21 performs one unneeded fetch when -U is supplied
4 participants