1215526 - Neither mach build faster nor mach build browser/themes nor mach build (no arguments) updates tab-selected-start/end.svg after making a change in browser/themes/windows/windowsShared.inc (or osx/linux include equivalents)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:Gijs (he/him)]

This is:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/tab-svgs.mozbuild

and

https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/tab-selected.svg

I don't know why it's not getting built, but I suspect timestamps. Dolske or Mike, do you have ideas? (per bug 1204182 and friends)

Comment 1

[Justin Dolske [:Dolske]]

Doesn't seem like 1204182 in involved (I assume you're specifically talking about bug 1204182 comment 12), the timestamps shouldn't matter unless the preprocessing doesn't update the output file's timestamp.

Isn't GENERATED_FILES supposed to be taking care of input/output dependancies? Guess it's not working?

Comment 2

[Mike Hommey [:glandium]]

GENERATED_FILES tracks input/output dependencies as defined in moz.build. But that's not where the dependency on the *Shared.inc files comes from, and the GENERATED_FILES script would need to handle those dependencies on its own. Which points to something missing in the GENERATED_FILES infrastructure, because even if the script would create dep files, there's nothing that would include them for the dependencies to be picked up by the build system.

Nathan, iirc, you implemented GENERATED_FILES, would you mind taking a look?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #2)
> GENERATED_FILES tracks input/output dependencies as defined in moz.build.
> But that's not where the dependency on the *Shared.inc files comes from, and
> the GENERATED_FILES script would need to handle those dependencies on its
> own. Which points to something missing in the GENERATED_FILES
> infrastructure, because even if the script would create dep files, there's
> nothing that would include them for the dependencies to be picked up by the
> build system.
> 
> Nathan, iirc, you implemented GENERATED_FILES, would you mind taking a look?

I think mshal had mentioned needing something like this, possibly even in the initial review of the GENERATED_FILES work; looks like the time has come.

I think the least invasive way to do it would be to have the script's main function return an (optional) set of additional dependencies to take into account:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/file_generate.py#56

Then file_generate.py can write out extra dependencies (if necessary) to a filename provided as input to file_generate.py:

http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#510

This filename will need to be included in EXTRA_MDDEPEND_FILES so those extra dependencies are included properly by rules.mk:

http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1343

As a bonus of doing all this, we'll be able to eliminate accessible/xpcom/Makefile.in, I think.

The other way I thought of to do it involved passing an outparam-style arg identifying the set of extra dependencies into the script's main function, but that requires a lot of boilerplate for each GENERATED_FILE and it's not very pythonic; I think having the dependencies as the return value makes more sense.  WDYT?

Comment 4

[Mike Hommey [:glandium]]

Sounds sensible.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 1 - pass dependencies file to file_generate.py

In addition to their inputs declared in moz.build files, generated files
may also depend on other files, such as #includes in preprocessed files.
Let's provide a place for file_generate.py to write out those extra
dependencies.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 2 - write dependencies to file_generate.py's depfile

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 3 - make preprocessor.preprocessor report all included files

For GENERATED_FILES scripts that want to report dependencies, this
change makes it easy to use |preprocess|, rather than having to
construct and use |Preprocessor| manually.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 4 - make preprocess-tab-svgs.py report extra dependencies

Building on the preprocessor.py changes, we can now return the set of
included files from the preprocessed tab SVGs.

Comment 9

[(not currently active) Ted Mielczarek]

I had a more complicated solution to this in bug 1058662, but this is probably more sensible.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> I had a more complicated solution to this in bug 1058662, but this is
> probably more sensible.

I dunno, you can certainly argue that expecting people to compute accurate deps in their file generation functions (and have the reviewers catch it if they don't) is a lost cause, and just watching all file opens is a better way of going about things.  I kinda like where you're going.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8676911 [details] [diff] [review]
part 1 - pass dependencies file to file_generate.py

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +504,5 @@
>          elif isinstance(obj, Exports):
>              self._process_exports(obj, obj.exports, backend_file)
>  
>          elif isinstance(obj, GeneratedFile):
> +            dep_file = "%s.pp" % obj.output

obj.output can be relative can't it? would be better to use mozpath.basename()

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/888b08961891
https://hg.mozilla.org/integration/mozilla-inbound/rev/936491b64350
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77130c95e52
https://hg.mozilla.org/integration/mozilla-inbound/rev/41913aea0fb0

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/888b08961891
https://hg.mozilla.org/mozilla-central/rev/936491b64350
https://hg.mozilla.org/mozilla-central/rev/b77130c95e52
https://hg.mozilla.org/mozilla-central/rev/41913aea0fb0

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> > I had a more complicated solution to this in bug 1058662, but this is
> > probably more sensible.
> 
> I dunno, you can certainly argue that expecting people to compute accurate
> deps in their file generation functions (and have the reviewers catch it if
> they don't) is a lost cause, and just watching all file opens is a better
> way of going about things.  I kinda like where you're going.

That's what I was thinking. Unfortunately short of having something like tup's filesystem proxying or at least bazel/buck's "build in a sandbox" behavior it's really hard to get dependencies right.