AdjacentTempDirectory should fail on unwritable directory #6215

zooba · 2019-01-28T17:51:44Z

Fixes #6169
Based on #6225

zooba · 2019-01-28T17:52:44Z

Ping @cjerdonek - are you able to suggest how best to make the temp directory unwritable? Or another way to test that scenario? I'm stuck (more precisely, all my ideas would be Windows specific ;) )

dstufft · 2019-01-28T17:53:41Z

Can you monkeypatch os.makedir?

cjerdonek · 2019-01-28T18:58:07Z

@zooba, @dstufft's suggestion sounds good. I was waiting myself to see how you would test that when you made your original comment. :)

pradyunsg · 2019-01-28T19:01:24Z

Directly linking to #6169 for a better cross-reference.

YannickJadoul · 2019-01-28T19:12:23Z

Minor remark/question; sorry if this is not relevant: isn't it weird that pip wants to create a folder in the parent folder of site-packages, so outside of site-packages? Is that folder (i.e., something like .../lib/pythonX.Y) supposed to be writable by pip?

zooba · 2019-01-28T19:32:31Z

@YannickJadoul It shouldn't be trying to create it there - what scenario is causing this?

zooba · 2019-01-28T19:44:23Z

Ah, found it (uninstalling scripts). The fallback needs to be better when this approach fails, but the infrastructure is there - more work incoming!

zooba · 2019-01-28T20:02:29Z

My fallback should also handle the previous problem of hitting the maximum path length limit, though it may create more temporary folders (in most cases I assume two: bin/Scripts and any files installed directly into site-packages)

YannickJadoul · 2019-01-28T20:23:49Z

@zooba I don't think it's just the scripts. Those result in maybe an even bigger problem (since a new folder in the parent folder of bin will be created), but what I was mentioning was linked to removing the .dist-info folder.

Example (using the ancient version of setuptools==18.2 as example because this is what is happining in the cibuildwheel repository, and I know it reproduces what I want to show):

yannick@Athenai:~$ source TEMPENV/bin/activate
(TEMPENV) yannick@Athenai:~$ ls TEMPENV/
bin  include  lib  pip-selfcheck.json
(TEMPENV) yannick@Athenai:~$ pip install setuptools==18.2
Collecting setuptools==18.2
  Using cached https://files.pythonhosted.org/packages/c3/f6/b3224fb53fd3ddebdcc2aafb8f2d3a044a66446c9f17f4e5a24ca03b781b/setuptools-18.2-py2.py3-none-any.whl
Installing collected packages: setuptools
  Found existing installation: setuptools 40.7.0
    Uninstalling setuptools-40.7.0:
      Successfully uninstalled setuptools-40.7.0
Successfully installed setuptools-18.2
(TEMPENV) yannick@Athenai:~$ chmod -w TEMPENV/lib/python3.6/
(TEMPENV) yannick@Athenai:~$ pip install --upgrade setuptools
Collecting setuptools
  Using cached https://files.pythonhosted.org/packages/5b/ac/90c7617bfc48ae1265d2c0cc46aeb0a0d482e00577008d21b64efe9a2006/setuptools-40.7.0-py2.py3-none-any.whl
Installing collected packages: setuptools
  Found existing installation: setuptools 18.2
    Uninstalling setuptools-18.2:
^COperation cancelled by user
(TEMPENV) yannick@Athenai:~$ ls TEMPENV/
bin  -in  include  lib  pip-selfcheck.json
(TEMPENV) yannick@Athenai:~$ ls TEMPENV/-in/
easy_install  easy_install-3.6
(TEMPENV) yannick@Athenai:~$ chmod +w TEMPENV/lib/python3.6/
(TEMPENV) yannick@Athenai:~$ pip install --upgrade -vvv setuptools

[... lots and lots of PyPI links ...]

Installing collected packages: setuptools
  Found existing installation: setuptools 18.2
    Uninstalling setuptools-18.2:
      Created temporary directory: /home/yannick/TEMPENV/~in
      Removing file or directory /home/yannick/TEMPENV/bin/easy_install
      Removing file or directory /home/yannick/TEMPENV/bin/easy_install-3.6
      Created temporary directory: /home/yannick/TEMPENV/lib/python3.6/site-packages/~_pycache__
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/__pycache__/
      Created temporary directory: /home/yannick/TEMPENV/lib/python3.6/site-packages/~markerlib
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/_markerlib/
      Created temporary directory: /home/yannick/TEMPENV/lib/python3.6/-ite-packages
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/easy_install.py
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/pkg_resources/
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/setuptools-18.2.dist-info/
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/setuptools/
      Successfully uninstalled setuptools-18.2

  changing mode of /home/yannick/TEMPENV/bin/easy_install to 755
  changing mode of /home/yannick/TEMPENV/bin/easy_install-3.6 to 755
Successfully installed setuptools-40.7.0
Cleaning up...
Removed build tracker '/tmp/pip-req-tracker-e2lpcqcx'

EDIT: That's a lot of output, but notice the line saying Created temporary directory: /home/yannick/TEMPENV/lib/python3.6/-ite-packages

zooba · 2019-01-28T20:55:40Z

@YannickJadoul Yep, that's the one that I fixed - it's doing that file because of the easy_install.py script, not the dist-info.

zooba · 2019-01-28T21:17:43Z

Okay, my fix doesn't account for ordering of the paths, so it's nearly back to where it was before :( Trying again

YannickJadoul · 2019-01-28T21:19:15Z

@zooba Are you sure? I'm pretty sure this package does not contain scripts, and I still get this -ite-packages sibling directory:

Installing collected packages: praat-parselmouth
  Found existing installation: praat-parselmouth 0.2.0
    Uninstalling praat-parselmouth-0.2.0:
      Created temporary directory: /home/yannick/TEMPENV/lib/python3.6/-ite-packages
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/parselmouth.cpython-36m-x86_64-linux-gnu.so
      Removing file or directory /home/yannick/TEMPENV/lib/python3.6/site-packages/praat_parselmouth-0.2.0.dist-info/
      Successfully uninstalled praat-parselmouth-0.2.0

Successfully installed praat-parselmouth-0.3.2

zooba · 2019-01-28T21:33:47Z

The -ite-packages folder is because they have a file in site-packages - a .so in your new case.

The -in folder was from files in bin.

If there's another directory where files may be installed but the directory is not wholly owned by the package, that could also trigger the same thing.

I have a bit more of a fix coming to remove the ordering issues from stash directory creation, but it certainly resolves this issue.

YannickJadoul · 2019-01-28T21:35:18Z

@zooba OK, yes, fair enough; my bad. Thanks!

cjerdonek · 2019-01-28T22:01:24Z

@zooba Are you fixing two issues now?

zooba · 2019-01-28T22:02:55Z

@cjerdonek The second one was introduced by the fix for the first. Shuffling directories around is considerably more complex once the fallback mechanism is introduced.

zooba · 2019-01-28T23:43:17Z

The CI failures are (going to be) due to some tests failing and timing out, but the logging isn't showing me anything useful. I'm trying to reproduce locally.

benoit-pierre · 2019-01-28T23:49:14Z

It's also holding up the queue on AppVeyor... I'll go ahead and cancel the build.

zooba · 2019-01-29T01:26:52Z

The current problem is during one of the rollback tests - it's creating an extra subdirectory for the .dist-info and moving the original .dist-info into it.

Normally this means that the "move" call is recognizing that the source is a file and the destination is a directory and creating the directory, but that isn't the case here (they're both directories). I'll keep looking tomorrow.

cjerdonek · 2019-01-29T01:32:33Z

Is there any way to separate the trickier stashing logic from the larger UninstallPathSet class to make the logic simpler to understand and test?

zooba · 2019-01-29T18:01:46Z

@cjerdonek I hope I can make it easier to follow, but ultimately, isn't it the whole point of the UninstallPathSet? The functionality won't be used elsewhere, and it requires creating real directories or else it won't fail to create them (without some serious monkeypatching…).

But I'll try factoring it into its own class and then see whether it's worth it.

cjerdonek · 2019-01-29T22:00:50Z

It was just a thought. I was mainly just thinking of the _stash() method and the methods it accesses (and _save_dirs attribute). The change you're making happens to be confined (largely) to that anyways.

zooba · 2019-01-29T23:56:02Z

@cjerdonek I like the idea, so I started it (posted my WIP, which probably won't pass, but will let you see where I'm heading). But now I'm going down a venv rabbithole instead 😖 So I'll get back to this as soon as I fix up the stuff I broke in CPython...

cjerdonek

I added a couple minor comments.

Also, re: splitting out, I was only thinking _stash() and _save_dirs, but this looks good, too!

src/pip/_internal/req/req_uninstall.py

pradyunsg

Seems OK to me but I'm not sure I'm familiar enough with this to "approve".

.gitignore

pradyunsg · 2019-02-03T15:25:38Z

@zooba Could you please update this PR?

BrownTruck · 2019-02-03T19:30:03Z

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!

…ctory

zooba · 2019-02-04T19:25:39Z

This was just the rebase - I'll get back to finishing the actual PR as soon as I can.

pradyunsg · 2019-02-05T03:00:52Z

Thanks @zooba! :)

I'm hoping to get this in for 19.0.2 -- which I plan to release in this week. It'd be ideal if we can get this done ~8th or 9th. :)

zooba · 2019-02-06T00:43:44Z

I don't think I have anything left to do here. Any suggestions?

pradyunsg · 2019-02-07T03:46:09Z

@cjerdonek Could you review this?

pradyunsg

LGTM

If no one voices any concerns in the next few hours, I'll go ahead and merge this.

cjerdonek · 2019-02-08T06:35:52Z

@pradyunsg Generally my preference is for a clean commit history (as opposed to fixing mistakes, etc), so I would prefer this to be squashed unless the commits were designed to have a coherent sequence.

pradyunsg · 2019-02-08T06:54:22Z

@pradyunsg Generally my preference is for a clean commit history (as opposed to fixing mistakes, etc), so I would prefer this to be squashed unless the commits were designed to have a coherent sequence.

Will do. :)

pradyunsg · 2019-02-09T04:56:49Z

Not sure why @zooba is not listed in the authors in 7eb79b1. I squash-merged via the Web UI.

A GitHub bug maybe?

xavfernandez · 2019-02-11T11:05:23Z

Not sure why @zooba is not listed in the authors in 7eb79b1. I squash-merged via the Web UI.

It looks like a simple merge happened :)

pradyunsg · 2019-02-11T12:53:01Z

Uh. And here I am, genuinely sure that I'd clicked Squash and Merge.

lock · 2019-05-29T00:04:43Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg changed the title ~~Fixes #6169: AdjacentTempDirectory should fail on unwritable directory~~ AdjacentTempDirectory should fail on unwritable directory Jan 28, 2019

pradyunsg added this to the 19.0 milestone Jan 28, 2019

YannickJadoul mentioned this pull request Jan 28, 2019

MacOS travis-ci build stalls pypa/cibuildwheel#122

Closed

cjerdonek mentioned this pull request Jan 29, 2019

Release 19.0 -- New year, New release #6106

Closed

cjerdonek reviewed Jan 30, 2019

View reviewed changes

src/pip/_internal/req/req_uninstall.py Show resolved Hide resolved

src/pip/_internal/req/req_uninstall.py Outdated Show resolved Hide resolved

cjerdonek added C: uninstall The logic for uninstallation of packages type: bugfix labels Jan 30, 2019

pradyunsg reviewed Feb 3, 2019

View reviewed changes

.gitignore Show resolved Hide resolved

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 3, 2019

zooba force-pushed the issue-6169 branch from 69594dd to 7affb2e Compare February 4, 2019 19:07

Fixes pypa#6169: AdjacentTempDirectory should fail on unwritable dire…

c31f19f

…ctory

zooba force-pushed the issue-6169 branch from 7affb2e to c31f19f Compare February 4, 2019 19:12

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 4, 2019

Remove mismerged line

a56854d

Added more tests and handling for short directory names

53d28ba

zooba added 2 commits February 5, 2019 14:08

Remove unused code

aba2943

Fixes ordering of imports

c64503e

pradyunsg requested a review from cjerdonek February 6, 2019 01:59

pradyunsg approved these changes Feb 8, 2019

View reviewed changes

pradyunsg removed the request for review from cjerdonek February 8, 2019 06:22

pradyunsg merged commit 7eb79b1 into pypa:master Feb 8, 2019

zooba deleted the issue-6169 branch February 8, 2019 23:40

cjerdonek mentioned this pull request Mar 10, 2019

Can't uninstall "cryptography" after installing "requests[security]" #6321

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label May 29, 2019

lock bot locked as resolved and limited conversation to collaborators May 29, 2019

AdjacentTempDirectory should fail on unwritable directory #6215

AdjacentTempDirectory should fail on unwritable directory #6215

Conversation

zooba commented Jan 28, 2019 • edited by pradyunsg Loading

zooba commented Jan 28, 2019

dstufft commented Jan 28, 2019

cjerdonek commented Jan 28, 2019

pradyunsg commented Jan 28, 2019

YannickJadoul commented Jan 28, 2019

zooba commented Jan 28, 2019

zooba commented Jan 28, 2019

zooba commented Jan 28, 2019

YannickJadoul commented Jan 28, 2019 • edited Loading

zooba commented Jan 28, 2019

zooba commented Jan 28, 2019

YannickJadoul commented Jan 28, 2019

zooba commented Jan 28, 2019

YannickJadoul commented Jan 28, 2019

cjerdonek commented Jan 28, 2019

zooba commented Jan 28, 2019

zooba commented Jan 28, 2019

benoit-pierre commented Jan 28, 2019

zooba commented Jan 29, 2019

cjerdonek commented Jan 29, 2019

zooba commented Jan 29, 2019

cjerdonek commented Jan 29, 2019

zooba commented Jan 29, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg commented Feb 3, 2019

BrownTruck commented Feb 3, 2019

zooba commented Feb 4, 2019

pradyunsg commented Feb 5, 2019 • edited Loading

zooba commented Feb 6, 2019

pradyunsg commented Feb 7, 2019

pradyunsg left a comment

Choose a reason for hiding this comment

cjerdonek commented Feb 8, 2019

pradyunsg commented Feb 8, 2019

pradyunsg commented Feb 9, 2019

xavfernandez commented Feb 11, 2019

pradyunsg commented Feb 11, 2019

lock bot commented May 29, 2019

zooba commented Jan 28, 2019 •

edited by pradyunsg

Loading

YannickJadoul commented Jan 28, 2019 •

edited

Loading

pradyunsg commented Feb 5, 2019 •

edited

Loading