[MERGE] [BUG 363837] Catch _win32_delete_readonly failure to remove file or directory and try to recover

[Alexander Belchenko]

Hi Maritza,

Because I'm the one who was involved in writing _win32_delete_readonly,
I'll try to provide some hints for you.

     def _win32_delete_readonly(function, path, excinfo):
         """Error handler for shutil.rmtree function [for win32]
         Helps to remove files and dirs marked as read-only.
         """
         exception = excinfo[1]
         if function in (os.remove, os.rmdir) \
             and isinstance(exception, OSError) \
             and exception.errno == errno.EACCES:
             make_writable(path)
             function(path)
         else:
             raise

The problem in the code above is that it used too-broad testing for 
error, e.g.

             and exception.errno == errno.EACCES:

errno.EACCES it's a POSIX-style error constant, IIUC it's about Access 
Denied.

Fortunately it seems like actually we have there WindowsError exception, 
and therefore can get real Windows Error Code (as of GetLastError() 
returns).

So, if you're adding following debugging prints to the function:

     def _win32_delete_readonly(function, path, excinfo):
         """Error handler for shutil.rmtree function [for win32]
         Helps to remove files and dirs marked as read-only.
         """
         exception = excinfo[1]

+       print type(exception), dir(exception)
+       print getattr(exception, 'winerror', None)

         if function in (os.remove, os.rmdir) \
             and isinstance(exception, OSError) \
             and exception.errno == errno.EACCES:
             make_writable(path)
             function(path)
         else:
             raise


You will obtain additional information about your error with 
BeyondCompare3. Especially the second print should print to you actual 
Windows error code 
(http://msdn.microsoft.com/en-us/library/ms681381(VS.85).aspx)

Knowing actual error code there is possible to imprtove 
_win32_delete_readonly function to make more fine-grained analysis why 
deletion of file fails and provide appropriate solution for different 
cases: deleting read-only files, files currently in use etc.

HTH

Maritza Mendez пишет:
> 
> 
> On Wed, May 6, 2009 at 2:01 AM, Alexander Belchenko <bialix at ukr.net 
> <mailto:bialix at ukr.net>> wrote:
> 
>     Maritza Mendez пишет:
> 
> 
> 
>         On Tue, May 5, 2009 at 8:54 AM, Aaron Bentley
>         <aaron at aaronbentley.com <mailto:aaron at aaronbentley.com>
>         <mailto:aaron at aaronbentley.com <mailto:aaron at aaronbentley.com>>>
>         wrote:
> 
>            -----BEGIN PGP SIGNED MESSAGE-----
>            Hash: SHA1
> 
>            Maritza Mendez wrote:
>             > The more I think about this, the more I think that a cure
>         may be
>            worse
>             > than the problem.  So I am leaning toward...
> 
>             > bzr would catch the exception, print a polite message
>             > about the external misbehavior, and just keep going.
> 
>            I think that this would be fine, so long as it's an opt-in
>         behaviour.
>            If you do it as an onerror callback, note that problems may
>         cascade,
>            because failure to delete a file can cause failure to delete its
>            containing directory.
> 
>            Aaron
> 
>         I think it is "opt-in" enough to limit the response to catching
>          OSError and ignoring all other exceptions.  What do you think?
>          At worst, bzr would continue to miss some things it misses now.
> 
>         I have to remind myself that this whole discussion comes from
>         the fact that _win32_delete_readonly does not robustly handle
>         failures in osutils.rmtree call to shutil.rmtree.  Since that
>         seems to be the whole purpose of _win32_delete_readonly, I might
>         suggest that osutils.rmtree and _win32_delete_readonly do not
>         meet design requirements.
> 
> 
>     Which one design requirements?
> 
>     The _win32_delete_readonly function intended to help in the cases
>     where bzr need to remove file marked as read-only. Nothing more.
>     There is special test in test_osutils.py to ensure this behavior.
> 
> 
> Point  taken.  I have not seen any design document for bzr, unless email 
> archives count.  I'm just saying that the purpose of osutils.rmtree 
> _appears_ to be to deal with files which are (for whatever reason) 
> readonly on Windows and need to be rendered into a removable state.  If 
> that is the (derived) requirement, then the current implemnatation is 
> good, but not foolproof, as we see.
> 
> -M
>