mach build faster doesn't remove files when they are removed from moz.build or jar.mn

RESOLVED FIXED in Firefox 45

Status

Firefox Build System
General
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla45

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8695156 [details] [diff] [review]
Allow to restrict what unaccounted files to remove when copying from a FileCopier
Attachment #8695156 - Flags: review?(gps)
(Assignee)

Comment 2

2 years ago
Created attachment 8695157 [details] [diff] [review]
Add a --track option to process_install_manifest
Attachment #8695157 - Flags: review?(gps)
(Assignee)

Comment 3

2 years ago
Created attachment 8695158 [details] [diff] [review]
Don't fail to read an install manifest containing non-actionable items
Attachment #8695158 - Flags: review?(gps)
(Assignee)

Comment 4

2 years ago
Created attachment 8695159 [details] [diff] [review]
Use process_install_manifest's --track option in the FasterMake backend
Attachment #8695159 - Flags: review?(gps)
Comment on attachment 8695156 [details] [diff] [review]
Allow to restrict what unaccounted files to remove when copying from a FileCopier

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

I had a patch to do this with a general callback function.  I prefer that to a registry, since registries aren't that flexible.

Comment 6

2 years ago
Comment on attachment 8695156 [details] [diff] [review]
Allow to restrict what unaccounted files to remove when copying from a FileCopier

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

::: python/mozbuild/mozpack/copier.py
@@ +445,5 @@
> +                # correct list of directories to remove, so we shouldn't
> +                # hit ENOTEMPTY in this case. If remove_unaccounted is a
> +                # FileRegistry, then we have a list of directories that may
> +                # not be empty, so ignore rmdir ENOTEMPTY errors for them.
> +                if e.errno == errno.ENOTEMPTY:

I'd feel slightly better about this case if it were limited to when remove_unaccounted is a FileRegistry. Just because we shouldn't see this error when remove_unaccounted is True doesn't mean we won't see this error.
Attachment #8695156 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8695157 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8695158 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8695159 - Flags: review?(gps) → review+
(Assignee)

Comment 7

2 years ago
(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8695156 [details] [diff] [review]
> Allow to restrict what unaccounted files to remove when copying from a
> FileCopier
> 
> Review of attachment 8695156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/copier.py
> @@ +445,5 @@
> > +                # correct list of directories to remove, so we shouldn't
> > +                # hit ENOTEMPTY in this case. If remove_unaccounted is a
> > +                # FileRegistry, then we have a list of directories that may
> > +                # not be empty, so ignore rmdir ENOTEMPTY errors for them.
> > +                if e.errno == errno.ENOTEMPTY:
> 
> I'd feel slightly better about this case if it were limited to when
> remove_unaccounted is a FileRegistry. Just because we shouldn't see this
> error when remove_unaccounted is True doesn't mean we won't see this error.

OTOH... should we really error out if that happens?

Comment 8

2 years ago
(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > Comment on attachment 8695156 [details] [diff] [review]
> > Allow to restrict what unaccounted files to remove when copying from a
> > FileCopier
> > 
> > Review of attachment 8695156 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: python/mozbuild/mozpack/copier.py
> > @@ +445,5 @@
> > > +                # correct list of directories to remove, so we shouldn't
> > > +                # hit ENOTEMPTY in this case. If remove_unaccounted is a
> > > +                # FileRegistry, then we have a list of directories that may
> > > +                # not be empty, so ignore rmdir ENOTEMPTY errors for them.
> > > +                if e.errno == errno.ENOTEMPTY:
> > 
> > I'd feel slightly better about this case if it were limited to when
> > remove_unaccounted is a FileRegistry. Just because we shouldn't see this
> > error when remove_unaccounted is True doesn't mean we won't see this error.
> 
> OTOH... should we really error out if that happens?

I can see both sides of the argument. On one hand, assumptions are invalid. This could expose race conditions that could result in accidental "packaging" type things from occurring. On the other, it's probably no big deal most of the time. Let's be more conservative for now and error. If it becomes an issue, we can revisit later.
backed out for causing some kind of race condition that caused build problems like https://treeherder.mozilla.org/logviewer.html#?job_id=18278258&repo=mozilla-inbound
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 13

2 years ago
Found the cause and got a fixup r+ from gps on irc. Will land when the tree reopens.
Flags: needinfo?(mh+mozilla)

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff12954eac4f
https://hg.mozilla.org/mozilla-central/rev/2f7417fcd286
https://hg.mozilla.org/mozilla-central/rev/f6f6ce771a59
https://hg.mozilla.org/mozilla-central/rev/fa84e8d2340b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.