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

Fixing issues with install's --target option. #2007

Merged
merged 3 commits into from
Sep 11, 2014
Merged

Conversation

theacodes
Copy link
Member

  1. Check for the existence of a directory before copying from temporary directory to final target. If it exists, warn the user.
  2. If the user specifies the --upgrade option and the directory exists, delete it and continue with installation.
  3. Adding tests for above cases.

This resolves #1489, #1710 completely and parts of #1833.

 1. Check for the existence of a directory before copying from temporary directory to final target. If it exists, warn the user.
 2. If the user specifies the --upgrade option and the directory exists, delete it and continue with installation.
 3. Adding tests for above cases.

This resolves pypa#1489, pypa#1710 completely and parts of pypa#1833.
@pfmoore
Copy link
Member

pfmoore commented Aug 31, 2014

Looks nice, many thanks for this.

@pfmoore
Copy link
Member

pfmoore commented Aug 31, 2014

Some issues:

  1. pip install --target a distlib==0.1.8 followed by pip install --target a -U distlib==0.1.9 leaves a distlib-0.1.8-py3.3.egg-info directory lying around.
  2. pip install --target a distlib==0.1.8 followed by pip install --target a distlib==0.1.9 correctly gives a "Target directory already exists" error, but copies the 0.1.9 egg-info directory over.
  3. Installing a single-file project like six twice gives the "Specify --upgrade" message, but if you do specify --upgrade, it then says "six.py already exists and is not a directory" and asks you to remove it manually.

If these can be fixed, that would be ideal. But they will be tricky to do so, and I'm not sure they are worth the bother.

@dstufft, @qwcode, @jezdez any opinions on this? I'm tempted to say that the behaviour with this patch is better than the current behaviour, so let's go with it. I'm also tempted to raise a follow-up PR to omit the metadata directories for a --target install, which would avoid issues (1) and (2) completely. Do you know of any reason that might not work? Do people use --target installs with the expectation of being able to introspect what's there with tools like pkg_resources?

@webmaven
Copy link

Isn't introspecting with pkg_resources a prerequisite for namespace packages working correctly?

@pfmoore
Copy link
Member

pfmoore commented Aug 31, 2014

@webmaven I don't use namespace packages so I can't say. But it certainly could be. If you can check (delete the egg-info directories and see if things still work) that'd be great.

@webmaven
Copy link

.egg-info or .dist-info directories?

UPDATE: OK, looks like neither are required, as long as the nspkg.pth files are still there.

@pfmoore
Copy link
Member

pfmoore commented Aug 31, 2014

@webmaven Thanks. I'm not sure where the nspkg.pth files come from, though, so I'll put this one down as "a bit risky" and not go there for now ;-)

@webmaven
Copy link

Those appear when you install a namespace package. In this case, installing repoze.lru puts repoze.lru-0.6-py2.7-nspkg.pth in the directory along with the repoze package.

@theacodes
Copy link
Member Author

Typical App Engine apps do not need the egg/dist-info files so maybe we can leave them out by default but with an option to include them? I just didn't want to add an option that only applied to --target. What are your thoughts on that @pfmoore? Alternatively, I could write some logic to clean old egg/dist-info files out.

For your third point, I think I can safely cover that case. I'm still somewhat new to python packaging internals and I forgot that packages could be just one file.

@pfmoore
Copy link
Member

pfmoore commented Aug 31, 2014

@jonparrott I'm inclined to leave the dist-info/egg-info directories alone. Seems like too much risk for too little benefit.

Behavior is now to overwrite files or folders if --upgrade is specified, however, links will not be overwritten.
@theacodes
Copy link
Member Author

@pfmoore that's fine with me. Will having the older info files around hurt pkg resources? If so, I would prefer to exclude them all together (which is pretty easy).

I've pushed a new commit to handle the case of single-module packages such as six. The reason I was skipping non-directories is because I didn't want to overwrite links. I've added a more appropriate check there and updated the tests to match.

Is there anything else that needs to be done on this?

@pfmoore
Copy link
Member

pfmoore commented Aug 31, 2014

Nope, I don't think so. I'd like to see if any of the other devs have feedback - I pinged them above - so I'll leave it a few days before merging. Many thanks for the contribution!

@theacodes
Copy link
Member Author

No problem, always happy to help, and thank you for your guidance. App Engine developers will be super happy when this lands.

I'll keep an eye out if the other devs have any other feedback for me here.

pfmoore added a commit that referenced this pull request Sep 11, 2014
Fixing issues with install's --target option.
@pfmoore pfmoore merged commit f6b0665 into pypa:develop Sep 11, 2014
@pfmoore
Copy link
Member

pfmoore commented Sep 11, 2014

No feedback, so I assume others are happy with this. Merged it.

@theacodes
Copy link
Member Author

Thanks, @pfmoore!

@theacodes
Copy link
Member Author

@pfmoore Curious as to when you guys are planning to release this patch as we'd like to inform the app engine community. If the answer is "when its done" that's fine. Thanks.

@dstufft
Copy link
Member

dstufft commented Oct 3, 2014

It'll be released as part of pip 6.0, there's not a hard time line for that, but near future is likely.

@theacodes
Copy link
Member Author

Thanks, @dstufft.

@webmaven
Copy link

webmaven commented Oct 3, 2014

@dstufft what is 'near future' in this context? A loose range would be fine as an answer.

@dstufft
Copy link
Member

dstufft commented Oct 3, 2014

Likely more than a month, less than 6 months. That depends on time and such though.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants