Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 7 years ago

With bug 1218999, the modified time of generated file gets updated regardless of whether the content changes.

It's not clear to me how it is still useful to avoid writing if the modified time is updated anyway, because modified time getting updated means all dependents will also get rebuilt.

I hope there can be a mechanism that, when we try to update a file but its content wouldn't change, we cache the current timestamp of the file (but don't update its modified time for now). When we finish building, we update the modified time of that file as well as all its dependents with the cached timestamp. That way, we wouldn't do any unnecessary rebuild on either the file itself or its dependents.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 1 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> I hope there can be a mechanism that, when we try to update a file but its
> content wouldn't change, we cache the current timestamp of the file (but
> don't update its modified time for now).

I mean we cache the current timestamp, not the timestamp of that file.

	(not currently active) Ted Mielczarek
	Comment 2 • 7 years ago

I think mshal said that tup can do something smarter than make here, since it's not doing comparisons of timestamps between different files. I'm fuzzy on the specifics, though.

It does seem like this doesn't buy us anything for the make backend, since we have to update the mtime or make will just run the rule again anyway.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 3 • 6 years ago

Do you have an estimation on when would we be able to start using tup instead of make?

If that's not going to happen very soonish, I'd like to add some compromise solution on generated files.

There are files which have tons of files depend on it, while generating its content is fast enough, and its dependencies can easily get updated without changing the content. In that case, repeatedly generating those files in each build is probably better than rebuilding all files depend on it when its dependency gets touched.

That includes files like nsCSSPropertyID.h which is indirectly included in tons of files, and a fully cached build can take several minutes, while its dependency - ServoCSSPropList.py can get updated whenever anything in servo/components/style/properties gets changed, which can be common when hacking with the style system.

I'd like to propose an opt-in flag for generated files to use FileAvoidWrite without updating mtime. What do you think about this?

	Michael Shal [:mshal] Assignee
	Comment 4 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> Do you have an estimation on when would we be able to start using tup
> instead of make?

We'll hopefully have it available as an experimental backend soon (end of Q2?) - the main blocker at this point is integrating it with cargo.

> There are files which have tons of files depend on it, while generating its
> content is fast enough, and its dependencies can easily get updated without
> changing the content. In that case, repeatedly generating those files in
> each build is probably better than rebuilding all files depend on it when
> its dependency gets touched.

This is the reason we want to use FileAvoidWrite on the outputs, but when make's target ($@) is a FileAvoidWrite, then we run into the always-rebuild-things mode that prompted bug 1218999 to update the timestamp of the first output. I think we can get the best of both worlds if we just disassociate the first output from the actual target and always use a stub file in the make backend. So instead of something like:

export:: ServoCSSPropList.h
ServoCSSPropList.h: GenerateServoCSSPropList.py ServoCSSPropList.py
        $(call py_action,...)

We'd instead do:

export:: ServoCSSPropList.h.stub
ServoCSSPropList.h: ServoCSSPropList.h.stub ;
ServoCSSPropList.h.stub: GenerateServoCSSPropList.py ServoCSSPropList.py
        $(call py_action,...) # Same as above
        @$(TOUCH) $@

This way we FileAvoidWrite all the things we care about (.h and such), but the stub file is always touched so make doesn't re-execute.

We do this manually for midl now (as of bug 1420119), but we could generalize it for all GENERATED_FILES.

I'll put together a patch and see if it works...

	Michael Shal [:mshal] Assignee
	Comment 9 • 6 years ago

Can you test out the patchset and see if you find any issues?

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8974561 [details]
Bug 1454912 - remove spurious generated files;

https://reviewboard.mozilla.org/r/242898/#review248744

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8974562 [details]
Bug 1454912 - Only output dependencies for GENERATED_FILES with scripts;

https://reviewboard.mozilla.org/r/242900/#review248746

I think this makes sense, but it's quite hard to reason through all the implications.  Green locally and in try is good enough for me.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8974563 [details]
Bug 1454912 - Use a .stub file as the target for all GENERATED_FILES rules;

https://reviewboard.mozilla.org/r/242902/#review248750

::: commit-message-e02ae:14
(Diff revision 1)
> +no-op build.
> +
> +The solution here is to use a stub file as the target of the script
> +invocation which will always be touched when the script runs. Since
> +nothing else in the build depends on the stub, we don't need to
> +FileAvoidWrite it. All actual outputs of the script can be

Countdown to a Make rule depending on the stub behaviour starts now...

::: python/mozbuild/mozbuild/backend/recursivemake.py:536
(Diff revision 1)
>                      raise ValueError('%s not in %s is not a valid substitution in %s'
>                                       % (e.args[0], ', '.join(sorted(substs.keys())), o))
>  
>              first_output = outputs[0]
>              dep_file = "%s.pp" % first_output
> +            # The stub target file needs to go in MDDEPDIR so that it doesn't

... doesn't get written into generated Android resource directories, breaking Gradle tooling and/or polluting the Android packages.

Note that this pollution could silently occur with lots of file patterns elsewhere in the tree.

::: python/mozbuild/mozbuild/backend/recursivemake.py:594
(Diff revision 1)
>                  backend_file.write('EXTRA_MDDEPEND_FILES += %s\n' % dep_file)
>  
> -                backend_file.write("""{output}: {script}{inputs}{backend}{force}
> +                backend_file.write("""{stub}: {script}{inputs}{backend}{force}
>  \t$(REPORT_BUILD)
>  \t$(call py_action,file_generate,{locale}{script} {method} {output} $(MDDEPDIR)/{dep_file}{inputs}{flags})
> +\t@$(TOUCH) $@

Hmm.  This is a very `RecursiveMake`-specific way to handle this.  Does tup Just Work without these hijinks?  (That would be great.)

I guess this is in line with our general push to haven `RecursiveMake` write out explicit deps and rules rather than vector through the functionality in `rules.mk`.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 13 • 6 years ago
mozreview-review

Comment on attachment 8974564 [details]
Bug 1454912 - Revert "Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change.";

https://reviewboard.mozilla.org/r/242904/#review248752

I wasn't aware we were doing this.  Terrifying stuff!

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8974563 [details]
Bug 1454912 - Use a .stub file as the target for all GENERATED_FILES rules;

https://reviewboard.mozilla.org/r/242902/#review248754

::: commit-message-e02ae:1
(Diff revision 1)
> +Bug 1454912 - Use a .stub file as the target for all GENERATED_FILES rules; r?Build

Oh!  I forgot -- we should be able to remove 4 instances where I hand implement some version of this stub file pattern; see

https://searchfox.org/mozilla-central/search?q=first%20output%20specially&path=

One other small improvement with the stub file pattern is that the stub file name looks good in the Make output.  What does it look like after your patch?

	Michael Shal [:mshal] Assignee
	Comment 15 • 6 years ago

(In reply to Nick Alexander :nalexander from comment #12)
> ::: python/mozbuild/mozbuild/backend/recursivemake.py:536
> (Diff revision 1)
> >                      raise ValueError('%s not in %s is not a valid substitution in %s'
> >                                       % (e.args[0], ', '.join(sorted(substs.keys())), o))
> >  
> >              first_output = outputs[0]
> >              dep_file = "%s.pp" % first_output
> > +            # The stub target file needs to go in MDDEPDIR so that it doesn't
> 
> ... doesn't get written into generated Android resource directories,
> breaking Gradle tooling and/or polluting the Android packages.

Fixed.

> Note that this pollution could silently occur with lots of file patterns
> elsewhere in the tree.

Wouldn't it error out if that were the case? I only noticed it in the first place because the Android builds failed when I pushed to try if it wrote to foo.stub instead of $(MDDEPDIR)/foo.stub

> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py:594
> (Diff revision 1)
> >                  backend_file.write('EXTRA_MDDEPEND_FILES += %s\n' % dep_file)
> >  
> > -                backend_file.write("""{output}: {script}{inputs}{backend}{force}
> > +                backend_file.write("""{stub}: {script}{inputs}{backend}{force}
> >  \t$(REPORT_BUILD)
> >  \t$(call py_action,file_generate,{locale}{script} {method} {output} $(MDDEPDIR)/{dep_file}{inputs}{flags})
> > +\t@$(TOUCH) $@
> 
> Hmm.  This is a very `RecursiveMake`-specific way to handle this.  Does tup
> Just Work without these hijinks?  (That would be great.)

Yeah, this is a RecursiveMake-specific bug. Tup has a different mechanism for the skip-downstream-stuff-if-the-file-is-unchanged functionality, but that is not affected by these patches.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 16 • 6 years ago

> > Note that this pollution could silently occur with lots of file patterns
> > elsewhere in the tree.
> 
> Wouldn't it error out if that were the case? I only noticed it in the first
> place because the Android builds failed when I pushed to try if it wrote to
> foo.stub instead of $(MDDEPDIR)/foo.stub

I feel like error is the odd case here, not success.  Think about any file pattern that globs things (say, jar.mn globs).  They're not going to error; that'll just stuff garbage into the omnijar.  The fact that aapt is opinionated is weird to me.

	Michael Shal [:mshal] Assignee
	Comment 17 • 6 years ago

(In reply to Nick Alexander :nalexander from comment #14)
> Oh!  I forgot -- we should be able to remove 4 instances where I hand
> implement some version of this stub file pattern; see
> 
> https://searchfox.org/mozilla-central/
> search?q=first%20output%20specially&path=

There is also xpcom/idl-parser/xpidl/moz.build from bug 1304131, though in that case the issue is that code in other-licenses/ply writes to the files, and can't take a pre-opened file handle from the mozbuild action. Are the instances you reference similar? If so, this patchset won't help with that unfortunately - I think the fix for that would be to have the file_generate.py not do a FileAvoidWrite on the first file, and let all the actions open their own files. This is due to the evolution of the file_generate action, since we wanted to enforce FileAvoidWrite for generated files, and only later started to support multiple generated files via a single action.

> One other small improvement with the stub file pattern is that the stub file
> name looks good in the Make output.  What does it look like after your patch?

Hmm, good question. Probably less good? Eg:

 0:00.68 mozilla-config.h.stub
 0:00.68 buildid.h.stub
 0:00.68 source-repo.h.stub
 0:00.95 stl.sentinel.stub

and due to the above xpidl issue we now have:

 0:02.24 xpidl.stub.stub

I'm not sure how easy this would be to fix, since those lines are displayed by REPORT_BUILD (https://dxr.mozilla.org/mozilla-central/rev/0cd106a2eb78aa04fd481785257e6f4f9b94707b/config/rules.mk#34), which prints $@, and the point of this bug is that the target that make sees is a stub file rather than a FileAvoidWrite file. Maybe we can just change REPORT_BUILD to patsubst away the .stub extension if there is one?

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 18 • 6 years ago

(In reply to Michael Shal [:mshal] from comment #17)
> (In reply to Nick Alexander :nalexander from comment #14)
> > Oh!  I forgot -- we should be able to remove 4 instances where I hand
> > implement some version of this stub file pattern; see
> > 
> > https://searchfox.org/mozilla-central/
> > search?q=first%20output%20specially&path=
> 
> There is also xpcom/idl-parser/xpidl/moz.build from bug 1304131, though in
> that case the issue is that code in other-licenses/ply writes to the files,
> and can't take a pre-opened file handle from the mozbuild action. Are the
> instances you reference similar? If so, this patchset won't help with that
> unfortunately - I think the fix for that would be to have the
> file_generate.py not do a FileAvoidWrite on the first file, and let all the
> actions open their own files. This is due to the evolution of the
> file_generate action, since we wanted to enforce FileAvoidWrite for
> generated files, and only later started to support multiple generated files
> via a single action.
> 
> > One other small improvement with the stub file pattern is that the stub file
> > name looks good in the Make output.  What does it look like after your patch?
> 
> Hmm, good question. Probably less good? Eg:
> 
>  0:00.68 mozilla-config.h.stub
>  0:00.68 buildid.h.stub
>  0:00.68 source-repo.h.stub
>  0:00.95 stl.sentinel.stub
> 
> and due to the above xpidl issue we now have:
> 
>  0:02.24 xpidl.stub.stub

Thanks for letting me see these.

> I'm not sure how easy this would be to fix, since those lines are displayed
> by REPORT_BUILD
> (https://dxr.mozilla.org/mozilla-central/rev/
> 0cd106a2eb78aa04fd481785257e6f4f9b94707b/config/rules.mk#34), which prints
> $@, and the point of this bug is that the target that make sees is a stub
> file rather than a FileAvoidWrite file. Maybe we can just change
> REPORT_BUILD to patsubst away the .stub extension if there is one?

Meh, this level of polish will just slow down folks trying to dig into why their situation isn't working.  Might as well give them a hint that it's in the `.stub` handling.  Maybe call it `.generated_file_stub` to help them along, even.

Anyway, I don't think we should change this; it's good enough.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 23 • 6 years ago

It doesn't seem to work properly. I suspect that it doesn't follow the dependencies returned from build script.

For example, when I touch servo/components/style/properties/longhand/ui.mako.rs, the build script of ServoCSSPropList.py should be triggered, because it is one of the files returned by the build script. However, it doesn't happen.

ServoCSSPropList.py only gets regenerated when I touch the input files specified in moz.build. So probably the new code doesn't handle the dependency returned from build script properly somehow?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 24 • 6 years ago

But other than that issue, it seems everything works as expected that when a file is not changed, nothing gets built unexpectedly.

	Michael Shal [:mshal] Assignee
	Comment 25 • 6 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #23)
> It doesn't seem to work properly. I suspect that it doesn't follow the
> dependencies returned from build script.
> 
> For example, when I touch
> servo/components/style/properties/longhand/ui.mako.rs, the build script of
> ServoCSSPropList.py should be triggered, because it is one of the files
> returned by the build script. However, it doesn't happen.
> 
> ServoCSSPropList.py only gets regenerated when I touch the input files
> specified in moz.build. So probably the new code doesn't handle the
> dependency returned from build script properly somehow?

Good catch! Looks like the file_generate action still used the first output as its target for the runtime dependencies. We can pass in the stub target filename pretty easily though.

	Michael Shal [:mshal] Assignee
	Comment 30 • 6 years ago

Comment on attachment 8974563 [details]
Bug 1454912 - Use a .stub file as the target for all GENERATED_FILES rules;

Meant to ask for re-review on this part to address #c23.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 31 • 6 years ago
mozreview-review

Comment on attachment 8974563 [details]
Bug 1454912 - Use a .stub file as the target for all GENERATED_FILES rules;

https://reviewboard.mozilla.org/r/242902/#review249766

Ah, I see -- good catch from Xidorn.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 32 • 6 years ago

Looks like everything works as expected now, thanks!

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 33 • 6 years ago

But I guess you may want to touch CLOBBER for this. It seems to me when I switched from a tree with this patchset to a tree without, some generated files are not correctly updated.

	Michael Shal [:mshal] Assignee
	Comment 38 • 6 years ago

Updated to touch CLOBBER per #c33.

	Pulsebot
	Comment 39 • 6 years ago

Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dbe6c1bea4d
remove spurious generated files; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/82c74467f638
Only output dependencies for GENERATED_FILES with scripts; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/1eb04a9bfb7a
Use a .stub file as the target for all GENERATED_FILES rules; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/9260cc524bb5
Revert "Bug 1218999 - Update mtimes when building a GENERATED_FILES target, even when contents don't change."; r=nalexander

	Dorel Luca [:dluca]
	Comment 40 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4dbe6c1bea4d
https://hg.mozilla.org/mozilla-central/rev/82c74467f638
https://hg.mozilla.org/mozilla-central/rev/1eb04a9bfb7a
https://hg.mozilla.org/mozilla-central/rev/9260cc524bb5