Skip to content

Ensure wheel download dir is present at the beginning of wheel command #7524

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
Merged
merged 4 commits into from
Jan 1, 2020
Merged

Ensure wheel download dir is present at the beginning of wheel command #7524

merged 4 commits into from
Jan 1, 2020

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Dec 29, 2019

Similar to what is done in the download command.

Following #7517 (comment)

@chrahunt chrahunt added skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code labels Dec 29, 2019
@sbidoul
wheel: ensure wheel dir is present earlier
d8374b8 
Similar to what is done for download_dir
in the download command
@sbidoul
In Resolver, assume wheel_download_dir exists
9da9f60 
This is ensured at the beginning of the wheel
command, which is the only command that
sets wheel_download_dir.

This is also similar to what is done for download_dir
in the download command.
@sbidoul
Normalize wheel dir at the beginning of wheel command
f558197 
Similar to what is done in the download command.
@sbidoul sbidoul force-pushed the wheel-download-dir-sbi branch from bbcff50 to f558197 Compare December 30, 2019 08:54
@sbidoul
Copy link
Member Author

sbidoul commented Dec 30, 2019 •
edited
Loading

The macos error seems to have been a transient.

This PR also fixes a problem introduced in #7517 where options.wheel_dir used in the copy operation was not normalized (and user expanded) anymore. Before #7517 it was normalized because we used preparer.wheel_download_dir which was normalized by the preparer constructor.

xavfernandez
xavfernandez approved these changes Dec 30, 2019
View reviewed changes
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Since the path normalization now happens in the wheel command,

pip/src/pip/_internal/operations/prepare.py

Lines 375 to 376 in b24a6d0

if wheel_download_dir:
wheel_download_dir = normalize_path(wheel_download_dir)

could also be cleaned.

@sbidoul
Copy link
Member Author

sbidoul commented Dec 30, 2019

@xavfernandez yes. I left it to keep some sort of symmetry with download_dir, for which there is an expanduser just before. Both can probably be removed indeed.

@chrahunt
Copy link
Member

Can we remove the unnecessary normalize_path(wheel_download_dir) pointed out by @xavfernandez, and then move the expanduser for download dir in a separate PR?

@sbidoul
Remove redundant path normalization in preparer
a6d2123 
wheel_download_dir is normalized at the
beginning of the wheel command
@sbidoul
Copy link
Member Author

sbidoul commented Dec 31, 2019

Can we remove the unnecessary normalize_path(wheel_download_dir)

Sure, done.

@xavfernandez xavfernandez merged commit 6896dfc into pypa:master Jan 1, 2020
@xavfernandez
Copy link
Member

Thanks 👍

@sbidoul sbidoul deleted the wheel-download-dir-sbi branch January 1, 2020 15:15
@sbidoul sbidoul mentioned this pull request Jan 1, 2020
Merged
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 31, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbidoul @chrahunt @xavfernandez