839786 - "mach clobber" should catch WindowsError [Error 5] "Access is denied" and print a friendlier error message

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Matt Brubeck (:mbrubeck)]

Currently if "mach clobber" runs while Firefox or some other process is using files in the objdir, it prints a stack trace and the "consider filing a bug" message.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

When the build tries to overwrite an in-use DLL, you get Error 5.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 713598 [details] [diff] [review]
patch v2

Review of attachment 713598 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/mach_commands.py
@@ +160,5 @@
> +                    "Could not clobber because a file was in use. If the "
> +                    "application is running, try closing it. {error}")
> +                return 1
> +            else:
> +                raise

Hmmm. It seems silly to trap a WindowsError this high up in the stack. I almost feel this logic should live in mozfile.mozfile.rmtree() and rmtree() should raise its own Exception type detailing what failed.

Also, I don't like this only handles Windows. Surely other platforms have similar issues. Perhaps just trapping OSError and displaying a generic error message (the same one?) and printing the exception details would be sufficient.

What do you think?

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Gregory Szorc [:gps] from comment #3)
> Hmmm. It seems silly to trap a WindowsError this high up in the stack. I
> almost feel this logic should live in mozfile.mozfile.rmtree() and rmtree()
> should raise its own Exception type detailing what failed.

Sure... although this may still be a windows-only code path (see below)...

> Also, I don't like this only handles Windows. Surely other platforms have
> similar issues. Perhaps just trapping OSError and displaying a generic error
> message (the same one?) and printing the exception details would be
> sufficient.

I don't know of similar issues on other platforms.  Linux will happily unlink an open file, and I assume OS X is similar though I haven't tested it.  On those systems, OSError probably means something else failed, like the permissions on the objdir are wrong.

So if rmtree traps OSError and always raises a custom error, the custom error would probably be less informative than the original OSError.  I personally think that letting the caller handle OSError is more useful.

Comment 5

[Jeff Hammel]

I'm +1 for putting in mozfile. Also, AIUI, :mbrubeck is correct: this particular issue is windows only.  That said, there are plenty of other bad things that can happen on POSIX....

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 713598 [details] [diff] [review]
patch v2

Review of attachment 713598 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is fine for now. Something is better than nothing.

If mozfile comes up with a new API, we can switch to that.

I just wanted to ask questions to ensure we had considered things :)

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2984ebe7f222

I'll leave any changes to mozfile to someone else in another bug, since I don't really know about what similar use cases other callers might have.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2984ebe7f222

Comment 9

[Jeff Hammel]

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2984ebe7f222
> 
> I'll leave any changes to mozfile to someone else in another bug, since I
> don't really know about what similar use cases other callers might have.

filed: bug 841808