Closed
Bug 1218999
Opened 9 years ago
Closed 9 years ago
Not hard to end up with a tree that will always build GENERATED_FILES output
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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.
Reporter | ||
Comment 1•9 years ago
|
||
Hmm, bug 1188468 did something sort of bizarre for this.
Reporter | ||
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8679699 -
Flags: review?(mh+mozilla)
Comment 4•9 years ago
|
||
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)
Reporter | ||
Comment 5•9 years ago
|
||
(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.
Reporter | ||
Updated•9 years ago
|
Attachment #8679699 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 6•9 years ago
|
||
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.
Reporter | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Reporter | ||
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 12•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
Comment 13•9 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•