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

Jump to bottom

New resolver: Avoid polluting dest dir #8843

Merged
merged 3 commits into from
Oct 7, 2020
Merged

New resolver: Avoid polluting dest dir #8843

merged 3 commits into from
Oct 7, 2020

Conversation

McSinyx
Copy link
Contributor

@McSinyx McSinyx commented Sep 4, 2020 •
edited
Loading

Previously, during dependency resolution for pip download -d <dir> or pip wheel -w <dir>, distributions downloaded are always saved to <dir>, even for those are only used in backtracking and are not part of the returned requirement set.

Note that this changes the level from info to debug of the log saying we're skipping copying linked-to-directory requirements.

Fix GH-8827.

@uranusjr
Copy link
Member

uranusjr commented Sep 4, 2020

Would it be possible to do this without adding InstallRequirement.download_dir? That class already has so many stateful attributes I am reluctant to see one more 😞

@McSinyx
Copy link
Contributor Author

McSinyx commented Sep 4, 2020

Yes it is! Thank you for the heads-up, I didn't noticed that earlier 😅

@McSinyx McSinyx force-pushed the no-save-resolve-artifacts branch 2 times, most recently from db976e6 to 09c6785 Compare September 12, 2020 16:05
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 16, 2020
pradyunsg
pradyunsg requested changes Sep 16, 2020
View reviewed changes
src/pip/_internal/operations/prepare.py
Comment on lines -594 to +579
if self._download_should_save:
req.archive(self.download_dir)
req.archive(self.download_dir)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right. Is this covered by the build_dir changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is covered by the change to req.archive, since more than one call to it check for the same condition: ceaddcc

src/pip/_internal/commands/wheel.py Outdated Show resolved Hide resolved
@pradyunsg pradyunsg added the !release blocker Hold a release until this is resolved label Sep 16, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 16, 2020
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 17, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 17, 2020
@McSinyx
Copy link
Contributor Author

McSinyx commented Sep 29, 2020

Gentle ping!

@McSinyx
Copy link
Contributor Author

McSinyx commented Oct 6, 2020 •
edited
Loading

Thanks, I've just resolved the conflicts! BTW did you switch brown truck off? I haven't seen it rolling for quite a while.

@pradyunsg
Copy link
Member

CI linter failed here.

@McSinyx
Remove download_dir exist check
78b294e 
Both pip download and wheel call endure_dir on the directory.
@McSinyx
Merge usage of download_dir and wheel_download_dir
6887b07 
In every cases, at least one of them is None.  By doing this,
it is also possible to simplify wrapper codes around download_dir.
@McSinyx
New resolver: Avoid polluting dest dir
b28e2c4 
Previously, during dependency resolution for `pip download -d <dir>`
or `pip wheel -w <dir>`, distributions downloaded are always saved
to <dir>, even for those are only used in backtracking and are not
part of the returned requirement set.
@McSinyx
Copy link
Contributor Author

McSinyx commented Oct 7, 2020

I've just sync'ed with master, should be OK now.

@pradyunsg pradyunsg merged commit 739f342 into pypa:master Oct 7, 2020
@pradyunsg
Copy link
Member

Thanks @McSinyx! :)

@McSinyx McSinyx deleted the no-save-resolve-artifacts branch October 7, 2020 10:25
@sbidoul sbidoul mentioned this pull request Nov 11, 2020
Closed
@sbidoul sbidoul mentioned this pull request Dec 14, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@F2MZK8A6N70W F2MZK8A6N70W F2MZK8A6N70W approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
!release blocker Hold a release until this is resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pip download does not clean up new resolver's backtracking artifacts
7 participants
@McSinyx @uranusjr @BrownTruck @pradyunsg @jsirois @F2MZK8A6N70W @pypa-bot