delete read-only file in Windows by robberphex · Pull Request #1745 · pypa/pip (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Modify rmtree_errorhandler can delete read-only file in stage clean up.
That means resolve #1744
Fixed Issue #1744 delete read-only file in Windows
| raise |
|---|
| # file type should currently be read only |
| if ((os.stat(path).st_mode & stat.S_IREAD) != stat.S_IREAD): |
| # if file type currently read only |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code looks like it was intended to already catch this case - can you clarify why it did not do so in your case?
In principle, the change looks reasonable (the current code seems unnecessarily complex and full of magic numbers) but I'd like to make sure the change doesn't miss some odd corner case that the old code handled before committing. I'll do a fuller review soon.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Windows+Python 3.4.0(both is 64bit), when try to delete read-only file, it will raise a PermissionError.
Old version will match it(Line 56), and raise the exception. We wouldn't clean up the files.
New version: if the file is read-only, will try to remove read-only attribute and try to remove file again.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so it's a change in the exception raised, that isn't caught by the original logic. Thanks for the clarification. Did just adding the 3.4 exception work, and if so, what was the logic for the larger change (refactoring for robustness?)
I think I'd prefer to see a minimal change that just added the 3.4 exception, possibly followed by a "refactor and tidy up" change. But my actual opinion's still waiting on a proper review - I'll get to it today.
Looks like the build's failing. I can't immediately see how the failure is related to your code, but I also apparently can't rerun the build in case it's an intermittent Travis funny.
Having said that, now that I've read the change in context (by checking out the actual source rather than reading the diff!) it looks OK to me. So when we get the builds to pass I'm OK with merging it.
Build has now rerun successfully. Unless any of the other devs raise an objection in the next day or so, I'll probably merge this. @pypa/pip-developers does anyone know of any reason why the old, more complex code might be needed for Unix?
Is this code path tested atm? If so, then I'd have no problems with it.
Especially problematic is that travis doesn't test Windows atm...
Certainly not on Travis. And given that the tests basically don't always fail on Windows, I guess it isn't tested at all :-( But we very rarely test on Windows (I mostly don't run the full test suite) so I'm not inclined to block a useful patch for a test we'll rarely, if ever, run... (I wouldn't object to a test being added, but I'm reluctant to require it).
@qwcode had a Jenkins instance that ran the tests on Windows at one point - I don't know if it's still working.
AIUI, the issue is going to happen every time for git+https URLs on Python 3.4, so I think it's important to fix it reasonably quickly (and while I think, thanks to @robberphex for spotting it and providing a fix).
@pfmoore More than once I've assumed some code should be useful, but it turned out not to be. Testing helps you be sure that it in fact is, both at the time you make the change, and in the future when you change code around it.
I would prefer adding tests if we can. Getting CI on Windows and OSX so that it runs on every PR is a future goal of mine.
@dstufft @Ivoz Hmm, test_install_vcs appears to be failing hard on Windows:
TypeError: object of type 'LocalPath' has no len()
in ntpath.py from tmpdir in tests\conftest.py. Looks like nobody's running the tests on Windows (at least not with Python 3.4, which is what I'm trying to do) so it's quite possible we would already have a test failure for this, we just don't know about it :-(
As a stopgap, I manually ran pip install "git+https://github.com/noamraph/tqdm.git" both with and without this patch, using Python 3.4 on Windows, and I can confirm that it fails without the patch but succeeds with. Far from perfect I know, but it's the best I can do for now.
@pfmoore Yes, the unittest is failed in Windows.
But I think it is because that pytest using LocalPath to mock the system's path(it's str). And in function splitdrive(called by join,in file ntpath.py), it assume that path is str. So, it raise the TypeError in Windows.
BTW, I want to help pytest fix it, but I am not understand it's logic.
@robberphex it would be awesome if you could file an issue with details of the test failure for Windows, especially if it's a separate issue that has come up but we have failed to note its failure on Windows because we don't automatically run tests there.
Merging. We can handle the test issues separately under #1762 Thanks @robberphex for the fix.
pfmoore added a commit that referenced this pull request
delete read-only file in Windows
Are there any plans to release this fix in the near future?
It's in develop and will be included in the next release. I don't have an ETA for that at the moment.
The original logic didn't simply "fail to catch" the 3.4 exception and re-raise it (at least for me); it outright broke - because in 3.4, the exctype is PermissionError, but len(value.args) == 2, so the attempt to detect a 3.3 exception will raise a new IndexError. Thanks in part to exception chaining, there is a lot of resulting red text in pip.
The new logic seems like a good idea to me, and is what I would have suggested myself :) If a file is supposed to be deleted, deleting it failed, and the file is currently read-only, then attempting to set it writable and trying a second time is IMHO a good idea - regardless of why deletion failed the first time around. The worst case is that it does a little extra work trying to delete things that couldn't (but should) be deleted for other reasons, which should be rare anyway.
Looking forward to the next release :)
Outdated issues that have been locked by automation
label
Labels
Outdated issues that have been locked by automation