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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Hmm, bug 1188468 did something sort of bizarre for this.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: 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, 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.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 4

[Mike Hommey [:glandium]]

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)

Comment 5

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1218999 - Back out changeset 5f32b2bcfa43 (bug 1188468) in favor of a more efficient solution. r=glandium

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.

Comment 8

[Mike Hommey [:glandium]]

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

Comment 9

[Mike Hommey [:glandium]]

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

Comment 10

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3326214cc3c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd3d049a39ad

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd3d049a39ad

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bd3d049a39ad

Comment 13

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion