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

Refactor yanked warning out of package finder #7796

Closed

Conversation

uranusjr
Copy link
Member

Noticed this when reading @pfmoore’s effort to wire pip internals to ResolveLib primitives.

The previous implementation assumes that when a best candidate is found, it will be downloaded/installed. This will no longer be the case for the new resolver, since it may decide to discard the candidate after inspection and fetch a new one.

By moving it to InstallRequirement.populate_link(), we can be sure that the warning is only set when the yanked link is actually populated for installation.

@uranusjr uranusjr force-pushed the yanked-warning-out-of-evaluator branch 3 times, most recently from a81c54c to e878aac Compare February 27, 2020 06:16
@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Feb 27, 2020
The previous implementation assumes that when a best candidate is found,
it will be downloaded/installed. This will no longer be the case for the
new resolver, since it may decide to discard the candidate after
inspection and fetch a new one.

By moving it to populate_link(), we can be sure that the warning is only
set when the yanked link is actually populated for installation.
@uranusjr uranusjr force-pushed the yanked-warning-out-of-evaluator branch from e878aac to 333600a Compare February 27, 2020 06:19
@uranusjr
Copy link
Member Author

Ugh, this still seems to be the wrong place. We need to call prepare_linked_requirement() for dist metadata, but to make an InstallRequirement linked we need to call populate_link() first. So the warning needs to be triggered at the call site of populate_link(), not inside it.

@pradyunsg
Copy link
Member

pradyunsg commented Feb 27, 2020

suggestion: add a check_yanked_link method to InstallRequirement, that looks are self.link.is_yanked and call it from within the current call site of populate_link (i.e. the resolver).

@uranusjr
Copy link
Member Author

Replacing with #7801. The link.is_yanked strategy does not work because populate_link() actively replaces the link with cached location (losing the yanking context).

@uranusjr uranusjr closed this Feb 28, 2020
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Apr 2, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020
@uranusjr uranusjr deleted the yanked-warning-out-of-evaluator branch September 28, 2020 14:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants