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

--target option causes issues when upgrading or re-installing #1489

Closed
theacodes opened this Issue Jan 21, 2014 · 19 comments

Comments

Projects
None yet
5 participants
@theacodes
Member

theacodes commented Jan 21, 2014

If you install using the "--target" option:

pip install --target lib requests

And then repeat the command a second time, it doesn't say "requirement satisfied" it installs it again as a subdirectory of the existing installation (lib/requests/requests and lib/requests-2.2.0.dist-info/requests-2.2.0.dist-info

Repeat the command a third time and it appears the nesting stops but it chokes and still doesn't report "requirement satisfied":

Downloading/unpacking requests
  Downloading requests-2.2.0-py2.py3-none-any.whl (623kB): 623kB downloaded
Installing collected packages: requests
Successfully installed requests
Cleaning up...
Exception:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/pip/basecommand.py", line 122, in main
    status = self.run(options, args)
  File "/usr/local/lib/python2.7/dist-packages/pip/commands/install.py", line 307, in run
    os.path.join(options.target_dir, item)
  File "/usr/lib/python2.7/shutil.py", line 291, in move
    raise Error, "Destination path '%s' already exists" % real_dst
Error: Destination path '/tmp/piptest/lib/requests/requests' already exists

Storing debug log for failure in /tmp/tmpcBDoql
@dmartinzar

This comment has been minimized.

Show comment
Hide comment
@dmartinzar

dmartinzar Jun 9, 2014

Has anyone had a chance to look at this since January? I just ran across this issue with the newest pip release.

dmartinzar commented Jun 9, 2014

Has anyone had a chance to look at this since January? I just ran across this issue with the newest pip release.

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Jul 10, 2014

@dstufft, there seems to be a cluster of issues relating to this one. Has anyone looked at it?

webmaven commented Jul 10, 2014

@dstufft, there seems to be a cluster of issues relating to this one. Has anyone looked at it?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 13, 2014

Member

Hey guys, this issue has been open for some time. Is there anything I can do to help this along?

Member

theacodes commented Aug 13, 2014

Hey guys, this issue has been open for some time. Is there anything I can do to help this along?

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Aug 30, 2014

@brosner, @dholth, @dstufft, @jezdez, @pfmoore, @pnasrat, @r1chardj0n3s

This is a longstanding issue that is really annoying.

webmaven commented Aug 30, 2014

@brosner, @dholth, @dstufft, @jezdez, @pfmoore, @pnasrat, @r1chardj0n3s

This is a longstanding issue that is really annoying.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 30, 2014

Member

@jonparrott Apart from the obvious "patches welcome", not much...

The nesting behaviour is really odd. On the other hand, the fact that pip doesn't recognise that requests is installed is understandable (after all, it isn't installed in the sense that import requests will fail) - I can see that the behaviour is not what you'd want, but it's at least understandable (unlike the nesting!)

Personally, I only ever use --target to dump a copy of a distribution somewhere (for vendoring, for example) and I never want to "manage" it - I just blow away the directory and re-download if needed. Even the dist-info directories are unwanted clutter for that usage.

To be honest, I could argue that --target is just broken, and should be removed. Maybe we should replace it with a command, "pip unpack" that just unpacked the distribution into the target and made no pretense that it was "installed"? That might better match the reality.

Member

pfmoore commented Aug 30, 2014

@jonparrott Apart from the obvious "patches welcome", not much...

The nesting behaviour is really odd. On the other hand, the fact that pip doesn't recognise that requests is installed is understandable (after all, it isn't installed in the sense that import requests will fail) - I can see that the behaviour is not what you'd want, but it's at least understandable (unlike the nesting!)

Personally, I only ever use --target to dump a copy of a distribution somewhere (for vendoring, for example) and I never want to "manage" it - I just blow away the directory and re-download if needed. Even the dist-info directories are unwanted clutter for that usage.

To be honest, I could argue that --target is just broken, and should be removed. Maybe we should replace it with a command, "pip unpack" that just unpacked the distribution into the target and made no pretense that it was "installed"? That might better match the reality.

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Aug 30, 2014

@pfmoore thanks for the response!

--target is currently rather important for one environment in particular, Google App Engine (eg. https://github.com/GoogleCloudPlatform/appengine-python-flask-skeleton and https://github.com/GoogleCloudPlatform/appengine-python-bottle-skeleton

An 'unpack' command, that still retrieved dependencies, updated in place, etc. could be a workable replacement, but you might want to talk to the folks in that community for feedback.

The 'blow away and re-download' approach is what @jonparrott's tool Gaelic does as a workaround, BTW.

webmaven commented Aug 30, 2014

@pfmoore thanks for the response!

--target is currently rather important for one environment in particular, Google App Engine (eg. https://github.com/GoogleCloudPlatform/appengine-python-flask-skeleton and https://github.com/GoogleCloudPlatform/appengine-python-bottle-skeleton

An 'unpack' command, that still retrieved dependencies, updated in place, etc. could be a workable replacement, but you might want to talk to the folks in that community for feedback.

The 'blow away and re-download' approach is what @jonparrott's tool Gaelic does as a workaround, BTW.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 30, 2014

Member

@pfmoore The vendoring use case is exactly what the app engine community is trying to do, as mentioned by @webmaven. The problem stems that even though --target works great for the initial vendoring, running it again just breaks everything. I've even written a tool to do exactly what you described- nuke the target directory and re-"install" everything.

Personally I think a command like "unpack" would perfectly suit the app engine community. I'm unfamiliar with pip internals and the particulars behind package installation, but I'm willing to dive in and take a first stab at implementing it. What are your thoughts, @webmaven?

Of course, if Google would add proper virtualenv support to app engine we'd be fine where we are. :(

Member

theacodes commented Aug 30, 2014

@pfmoore The vendoring use case is exactly what the app engine community is trying to do, as mentioned by @webmaven. The problem stems that even though --target works great for the initial vendoring, running it again just breaks everything. I've even written a tool to do exactly what you described- nuke the target directory and re-"install" everything.

Personally I think a command like "unpack" would perfectly suit the app engine community. I'm unfamiliar with pip internals and the particulars behind package installation, but I'm willing to dive in and take a first stab at implementing it. What are your thoughts, @webmaven?

Of course, if Google would add proper virtualenv support to app engine we'd be fine where we are. :(

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 30, 2014

Member

@webmaven as you say, it's probably the AppEngine community that need to discuss alternative approaches. If we can fix upgrades with --target we will, but I'm not promising anything (it feels like a hard problem).

Member

pfmoore commented Aug 30, 2014

@webmaven as you say, it's probably the AppEngine community that need to discuss alternative approaches. If we can fix upgrades with --target we will, but I'm not promising anything (it feels like a hard problem).

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Aug 30, 2014

OK, here is the bare minimum of functionality that I think a notional pip unpack would need. For all operations, unpack needs to get dependencies by default in the exact same way install does:

pip unpack <packagename> --target lib/

pip unpack -r requirements.txt --target lib/

A current pain point not yet addressed by pip: deriving a list of installed packages and their versions from a target folder. Because it isn't installing anything, you need an alternate method of creating expanded requirements files that include the unpacked dependencies. Either of the following would work: pip unpack -r requirements.txt --freeze > requirements.txt or pip freeze --target lib/ > requirements.txt Frankly, in the absence of an unpack command, the latter spelling would still be a good feature for pip, for the same reasons.

@jonparrott, can you think of anything else that is required to replace pip install --target for GAE's use case?

webmaven commented Aug 30, 2014

OK, here is the bare minimum of functionality that I think a notional pip unpack would need. For all operations, unpack needs to get dependencies by default in the exact same way install does:

pip unpack <packagename> --target lib/

pip unpack -r requirements.txt --target lib/

A current pain point not yet addressed by pip: deriving a list of installed packages and their versions from a target folder. Because it isn't installing anything, you need an alternate method of creating expanded requirements files that include the unpacked dependencies. Either of the following would work: pip unpack -r requirements.txt --freeze > requirements.txt or pip freeze --target lib/ > requirements.txt Frankly, in the absence of an unpack command, the latter spelling would still be a good feature for pip, for the same reasons.

@jonparrott, can you think of anything else that is required to replace pip install --target for GAE's use case?

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 30, 2014

Member

All you need to scan a directory for what's installed is:

import pkg_resources
env = pkg_resources.Environment([dirname])
for project in env:
    for dist in env[project]:
        print("{}=={}".format(dist.project_name, dist.version))

That's effectively what you were wanting from pip freeze --target dirname. It doesn't deal quite right with editable installations, but I suspect you'd have problems installing those with --target anyway.

Member

pfmoore commented Aug 30, 2014

All you need to scan a directory for what's installed is:

import pkg_resources
env = pkg_resources.Environment([dirname])
for project in env:
    for dist in env[project]:
        print("{}=={}".format(dist.project_name, dist.version))

That's effectively what you were wanting from pip freeze --target dirname. It doesn't deal quite right with editable installations, but I suspect you'd have problems installing those with --target anyway.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 30, 2014

Member

Oh, and the pip unpack invocations you show work now, using pip install rather than pip unpack. Modulo the bug originally reported for this issue, of course ;-)

So it seems to me that you can probably use the "blow away the directory" approach along with the script to generate a requirements file as a workaround.

Member

pfmoore commented Aug 30, 2014

Oh, and the pip unpack invocations you show work now, using pip install rather than pip unpack. Modulo the bug originally reported for this issue, of course ;-)

So it seems to me that you can probably use the "blow away the directory" approach along with the script to generate a requirements file as a workaround.

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Aug 30, 2014

It doesn't deal quite right with editable installations, but I suspect you'd have problems installing those with --target anyway.

@jonparrott, you have more experience than I in this area, but it doesn't seem to me, provided that the local install is in a virtualenv, that the --editable flag would be particularly problematic, unless you forgot to change it back before you deployed to GAE.

Oh, and the pip unpack invocations you show work now, using pip install rather than pip unpack. Modulo the bug originally reported for this issue, of course ;-)

Of course.

But then, you were asking what it would take to replace --target dirname on the install command with something like pip unpack, well, the answer is that it needs to support those relevant parts of the install command (such as --target) and actually work for reinstalls (ie. 'modulo the bug'). It isn't very surprising that the resulting commands look so similar.

So it seems to me that you can probably use the "blow away the directory" approach along with the script to generate a requirements file as a workaround.

Blowing away the contents of the directory only works for pip install -r requirements.txt --target lib/ and not for pip install <packagename> --target lib/.

webmaven commented Aug 30, 2014

It doesn't deal quite right with editable installations, but I suspect you'd have problems installing those with --target anyway.

@jonparrott, you have more experience than I in this area, but it doesn't seem to me, provided that the local install is in a virtualenv, that the --editable flag would be particularly problematic, unless you forgot to change it back before you deployed to GAE.

Oh, and the pip unpack invocations you show work now, using pip install rather than pip unpack. Modulo the bug originally reported for this issue, of course ;-)

Of course.

But then, you were asking what it would take to replace --target dirname on the install command with something like pip unpack, well, the answer is that it needs to support those relevant parts of the install command (such as --target) and actually work for reinstalls (ie. 'modulo the bug'). It isn't very surprising that the resulting commands look so similar.

So it seems to me that you can probably use the "blow away the directory" approach along with the script to generate a requirements file as a workaround.

Blowing away the contents of the directory only works for pip install -r requirements.txt --target lib/ and not for pip install <packagename> --target lib/.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 30, 2014

Member

OK. I just looked at the code. If --target is set, all pip does is:

  1. Set ignore-installed to true. (So that stuff already in site-packages doesn't block the install).
  2. Create a temporary directory.
  3. Do the install with --home=the_temp_dir
  4. Move everything from the lib_dir of the temp home into the target dir.
  5. Delete the temp dir.

So the nesting comes from step (4) as if the destination of shutil.move is a directory, the source is moved inside that directory. That's a clear bug, and fixable (although fixing is a bit fiddly). I may have a go at it, although I'm not promising when, as it's not a key issue for me (fixing the code isn't the hard bit, we'd want a test adding to confirm that the fix works). PRs are obviously welcome.

The "install into a temp dir" aspect explains most of the other inconsistencies, too. Multiple installs don't say "this is already there" because it isn't. Upgrade won't work because it's not there to upgrade. Editable would probably be a mess, I have no idea how it would work. "Fragile" is the most polite term I can think of for the behaviour...

Member

pfmoore commented Aug 30, 2014

OK. I just looked at the code. If --target is set, all pip does is:

  1. Set ignore-installed to true. (So that stuff already in site-packages doesn't block the install).
  2. Create a temporary directory.
  3. Do the install with --home=the_temp_dir
  4. Move everything from the lib_dir of the temp home into the target dir.
  5. Delete the temp dir.

So the nesting comes from step (4) as if the destination of shutil.move is a directory, the source is moved inside that directory. That's a clear bug, and fixable (although fixing is a bit fiddly). I may have a go at it, although I'm not promising when, as it's not a key issue for me (fixing the code isn't the hard bit, we'd want a test adding to confirm that the fix works). PRs are obviously welcome.

The "install into a temp dir" aspect explains most of the other inconsistencies, too. Multiple installs don't say "this is already there" because it isn't. Upgrade won't work because it's not there to upgrade. Editable would probably be a mess, I have no idea how it would work. "Fragile" is the most polite term I can think of for the behaviour...

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 30, 2014

Member

Thanks for looking into this Paul and finding the source of one of the problems.

I think for App Engine's purposes, fixing upgrade isn't really necessary - if there's a way for us to overwrite anything in the destination directory instead of failing that would be sufficient.

I can try to see if I can write a test/PR this weekend for the fix if you don't beat me to it.

Member

theacodes commented Aug 30, 2014

Thanks for looking into this Paul and finding the source of one of the problems.

I think for App Engine's purposes, fixing upgrade isn't really necessary - if there's a way for us to overwrite anything in the destination directory instead of failing that would be sufficient.

I can try to see if I can write a test/PR this weekend for the fix if you don't beat me to it.

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Aug 30, 2014

@pfmoore thanks for investigating! Like @jonparrott says, upgrading is desired but not required (desired because it makes using app engine that much more like the rest of the Python ecosystem). If we just get to idempotency for repeated installs, I think we'll both be quite pleased.

What happens if you skip steps 2, 4, and 5, and do the install with --home=target_dir?

webmaven commented Aug 30, 2014

@pfmoore thanks for investigating! Like @jonparrott says, upgrading is desired but not required (desired because it makes using app engine that much more like the rest of the Python ecosystem). If we just get to idempotency for repeated installs, I think we'll both be quite pleased.

What happens if you skip steps 2, 4, and 5, and do the install with --home=target_dir?

theacodes added a commit to theacodes/pip that referenced this issue Aug 31, 2014

Fixing issues with instal's --target option.
 1. Check for the existance 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.

theacodes added a commit to theacodes/pip that referenced this issue Aug 31, 2014

Fixing issues with install's --target option.
 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.
@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 31, 2014

Member

PR submitted. It was easier than I expected to get the behavior that we wanted from an app engine perspective, I'm just not sure if it's what pip wants for the behavior of --target. Either way, I think it's an improvement over the broken behavior that's there currently.

@pfmoore feel free to let me know if I need to adjust anything here.

Member

theacodes commented Aug 31, 2014

PR submitted. It was easier than I expected to get the behavior that we wanted from an app engine perspective, I'm just not sure if it's what pip wants for the behavior of --target. Either way, I think it's an improvement over the broken behavior that's there currently.

@pfmoore feel free to let me know if I need to adjust anything here.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 31, 2014

Member

@jonparrot I've just looked at the PR, and it looks good. I'll probably download it and test it a bit locally before merging it, but I don't see any obvious problems.

As regards the preferred behaviour of --target, I'm happy with what you've done, I don't think there's any particular "preferred behavior" to worry about here.

On a somewhat unrelated note, are the .dist-info directories of any use for your envioronment? I'm considering writing a separate PR to omit them, as all I ever do with them is delete them. But if they are needed by others, that's fine, I won't bother.

Member

pfmoore commented Aug 31, 2014

@jonparrot I've just looked at the PR, and it looks good. I'll probably download it and test it a bit locally before merging it, but I don't see any obvious problems.

As regards the preferred behaviour of --target, I'm happy with what you've done, I don't think there's any particular "preferred behavior" to worry about here.

On a somewhat unrelated note, are the .dist-info directories of any use for your envioronment? I'm considering writing a separate PR to omit them, as all I ever do with them is delete them. But if they are needed by others, that's fine, I won't bother.

@webmaven

This comment has been minimized.

Show comment
Hide comment
@webmaven

webmaven Sep 11, 2014

@pfmoore any problems show up in testing?

webmaven commented Sep 11, 2014

@pfmoore any problems show up in testing?

@pfmoore pfmoore closed this in #2007 Sep 11, 2014

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Sep 11, 2014

Member

@webmaven sorry I got caught up with RL. Thanks for the ping, I've merged this now.

Member

pfmoore commented Sep 11, 2014

@webmaven sorry I got caught up with RL. Thanks for the ping, I've merged this now.

kevinburke added a commit to kevinburke/requests that referenced this issue Apr 21, 2016

Flip conditional in session.send()
Previously we checked that the `request` being sent was an instance of a
PreparedRequest. If a user somehow created a PreparedRequest using a different
Requests library instance, this check makes the request un-sendable.

(This happened recently - unbeknownst to me, my server was running an outdated
version of pip, vulnerable to this issue - pypa/pip#1489, which creates
multiple subdirectories (src/requests, src/requests/requests) when you rerun
pip install --target. So the PreparedRequest was being created in one version
of the library and compared against the other version of the library, and
throwing this exception, even though they were both PreparedRequest instances!)

It would probably be preferable to check the object's behavior (instead of
its type), but a PreparedRequest has a lot of behavior, and it wouldn't be
really feasible or allow us to provide a helpful error message to check all
of it here. Instead flip the conditional to guard against the user sending an
unprepared Request, which should still give us most of the benefits of the
better error message.

Fixes #3102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment