Closed Bug 1218999 Opened 4 years ago Closed 4 years ago

Not hard to end up with a tree that will always build GENERATED_FILES output

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: chmanchester, Unassigned)

References

Details

Attachments

(2 files)

I found this writing a patch for bug 1217015 and trying to make sure the generated dependencies were correct. I noticed my target was getting regenerated on successive builds, and actually this was the case for some other generated files output as well. The scenario is:

GENERATED_FILES uses script foo.py to generate out.h.
foo.py is a prerequisite for out.h in backend.mk.
foo.py is updated in a way that doesn't update the contents of out.h.
The target for out.h runs the next time the tree is built, but GENERATED_FILES uses FileAvoidWrite and avoids that write, so the mtime for out.h is unchanged (and is older than that for foo.py).
Successive build finds out.h has an older timestamp than foo.py, and builds the target even though its contents are up to date.

This is all pretty fast right now, but maybe we should have a version of FileAvoidWrite that updates mtimes for cases like these.
Hmm, bug 1188468 did something sort of bizarre for this.
Bug 1218999 - Update mtimes when building a GENERATED_FILES, even when contents don't change. r=glandium

When a make target is generated with FileAvoidWrite, this can cause targets to
get rebuilt perpetually when a prerequisite is updated, because FileAvoidWrite
will leave the target's mtime older than the prerequisite's when the target's
contents are unchanged. To avoid this issue, GENERATED_FILES is modified to
unconditionally update its target's mtime.
Attachment #8679699 - Flags: review?(mh+mozilla)
Comment on attachment 8679699 [details]
MozReview Request: Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium

Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium

When a make target is generated with FileAvoidWrite, this can cause targets to
get rebuilt perpetually when a prerequisite is updated, because FileAvoidWrite
will leave the target's mtime older than the prerequisite's when the target's
contents are unchanged. To avoid this issue, GENERATED_FILES is modified to
unconditionally update its target's mtime.
Attachment #8679699 - Attachment description: MozReview Request: Bug 1218999 - Update mtimes when building a GENERATED_FILES, even when contents don't change. r=glandium → MozReview Request: Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium
Attachment #8679699 - Flags: review?(mh+mozilla)
Comment on attachment 8679699 [details]
MozReview Request: Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium

https://reviewboard.mozilla.org/r/23509/#review20957

::: python/mozbuild/mozbuild/action/file_generate.py:73
(Diff revision 2)
> +        os.utime(args.output_file, None)

This should be in its own try block (possibly ignoring exceptions), because the error reporting that would happen if this throws is not really useful.

Could you also take care of backing out what landed in bug 1188468? (and double check that this does address bug 1188468)
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8679699 [details]
> MozReview Request: Bug 1218999 - Update mtimes when building a
> GENERATED_FILES target, even when contents don't change. r=glandium
> 
> https://reviewboard.mozilla.org/r/23509/#review20957
> 
> ::: python/mozbuild/mozbuild/action/file_generate.py:73
> (Diff revision 2)
> > +        os.utime(args.output_file, None)
> 
> This should be in its own try block (possibly ignoring exceptions), because
> the error reporting that would happen if this throws is not really useful.
> 
> Could you also take care of backing out what landed in bug 1188468? (and
> double check that this does address bug 1188468)

Certainly. I just tested this, and it does.
Attachment #8679699 - Flags: review?(mh+mozilla)
Comment on attachment 8679699 [details]
MozReview Request: Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium

Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium

When a make target is generated with FileAvoidWrite, this can cause targets to
get rebuilt perpetually when a prerequisite is updated, because FileAvoidWrite
will leave the target's mtime older than the prerequisite's when the target's
contents are unchanged. To avoid this issue, GENERATED_FILES is modified to
unconditionally update its target's mtime.
Bug 1218999 - Back out changeset 5f32b2bcfa43 (bug 1188468) in favor of a more efficient solution. r=glandium

Bug 118468 landed an option for FileAvoidWrite to always write to an output
file, whether or not the contents would be changed. This was to address a
problem caused by not updating mtimes when building GENERATED_FILES, but
undoes the purpose of FileAvoidWrite and isn't really necessary.
This is addressed in a subsequent commit by unconditionally updating
mtimes when processing GENERATED_FILES.
Attachment #8680275 - Flags: review?(mh+mozilla)
Comment on attachment 8679699 [details]
MozReview Request: Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change. r=glandium

https://reviewboard.mozilla.org/r/23509/#review21095
Attachment #8679699 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8680275 [details]
MozReview Request: Bug 1218999 - Back out changeset 5f32b2bcfa43 (bug 1188468) in favor of a more efficient solution. r=glandium

https://reviewboard.mozilla.org/r/23603/#review21097
Attachment #8680275 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/bd3d049a39ad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Product: Core → Firefox Build System
Depends on: 1454912
You need to log in before you can comment on or make changes to this bug.