Create a top level pip download command. #2995
Conversation
MathewJennings
changed the title
MathewJennings
force-pushed
the
add-pip-download-command
branch
from
76d9f1d to
70d89b3
Compare
|
|
||
| finder = self._build_package_finder(options, index_urls, session) | ||
| build_delete = (not (options.no_clean or options.build_dir)) | ||
| wheel_cache = WheelCache(options.cache_dir, options.format_control) |
There was a problem hiding this comment.
seems strange to have a wheel cache for the download command: if you want wheel, you can use the pip wheel command. I dont know what to think about it...
There was a problem hiding this comment.
I think you're right. I probably left that in on accident, because I started download.py by copying install.py. I removed it in a git commit --amend.
|
Looks like a nice first step for the deprecation |
MathewJennings
force-pushed
the
add-pip-download-command
branch
2 times, most recently
from
1b7ee1b to
009ec81
Compare
|
@xavfernandez Do you know what's causing my third shard to fail? Seems external; reminds me of the errors we had with the mock update not too long ago. |
|
Latest coverage is no longer supporting python 3.2, which is causing testing failure when run on that python version :/ |
|
Alright, CI is all passing now thanks to the changes @dstufft made regarding coverage over the weekend. Is this ready to be merged in? Once it is I'd like to edit my other pull request over in #2965 to be built on top of |
MathewJennings
force-pushed
the
add-pip-download-command
branch
from
009ec81 to
54f8a81
Compare
|
@dstufft @xavfernandez Could you take a look at this PR and #2965 when you get a chance? We're still blocking a downstream change on getting these merged in. |
| allow_all_prereleases=options.pre, | ||
| process_dependency_links=options.process_dependency_links, | ||
| session=session, | ||
| ) |
There was a problem hiding this comment.
Looks like this method could moved to RequirementCommand since, it is shared between install, wheel and download commands.
There was a problem hiding this comment.
Good call, I just did this.
|
Like I've already said, I think it is a first simple step towards a new command. |
|
Thanks for your feedback @xavfernandez . I addressed your comments in the updated commit. Can you take a look at those changes? |
MathewJennings
force-pushed
the
add-pip-download-command
branch
3 times, most recently
from
017e5d6 to
f6c38d2
Compare
| @@ -498,6 +498,18 @@ def only_binary(): | |||
| "options to setup.py install. If you are using an option with a " | |||
| "directory path, be sure to use absolute path.") | |||
|
|
|||
| download_options = partial( | |||
There was a problem hiding this comment.
Line 49 of download.py: cmd_opts.add_option(cmdoptions.download_options())
I copied this paradigm from install.py because of how similar pip download is to pip install. If you think this should not be included I will remove it.
There was a problem hiding this comment.
Yes I saw it was declared but I didn't see where it was used by pip (no options.dowload_options used anywhere).
For pip install, it is used in https://github.com/pypa/pip/blob/develop/pip/commands/install.py#L208 and those options are passed down to the final setup.py install call when installing.
There was a problem hiding this comment.
Oh, I see! My mistake, I had not noticed I failed to pass it down further! Do you think it makes sense to have this here? I wasn't sure, which is why download_options ended up being half-implemented I suppose.
Since for pip install the install_options are only used in here https://github.com/pypa/pip/blob/develop/pip/commands/install.py#L307
I'm thinking maybe download_options doesn't need to exist the way install_options does. Shall I remove this?
There was a problem hiding this comment.
If neither of us sees its purpose, I guess you can probably remove it ;)
There was a problem hiding this comment.
Sounds good -- I removed it. Do you think we're almost ready to merge this in? We're blocking on the download features over in #2965 to become available for a feature in pantsbuild/pants, and that PR is now blocking on this one.
MathewJennings
force-pushed
the
add-pip-download-command
branch
from
f6c38d2 to
9959b52
Compare
|
Looks good to me :) You could even add a line to https://github.com/pypa/pip/blob/develop/CHANGES.txt |
|
Thanks @xavfernandez, I really appreciate your help! I'm making that change now and pushing one final amended commit! |
…stall --download does (which now has a deprecated warning, but still functions as expected).
MathewJennings
force-pushed
the
add-pip-download-command
branch
from
9959b52 to
e243b4f
Compare
|
Sounds like this is all set? Anything else needed before a merge @dstufft @xavfernandez ? |
|
Nudge. This is now almost a month old and it seems to have accumulated a merge conflict at this point. Is there anything preventing this from being merged? We're still blocking another pip PR and downstream code on this feature. |
|
You can rebase your branch on develop, I think I will merge it |
|
Since Mat is back to class and presumably busy, I'm going to resolve the merge conflicts and any test issues that have cropped up in #3085 I'll cc you there when Travis goes green. |
|
Closing this one since #3085 got merged |
lock
bot
added
the
auto-locked
pip download -d path/to/download/directorybehaves exactly likepip install --download path/to/download/directory. Added a deprecation warning topip install --download. Addresses #2643