Closed Bug 1215526 Opened 4 years ago Closed 4 years ago

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)

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: Gijs, Assigned: froydnj)

References

Details

Attachments

(4 files)

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)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(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?
Flags: needinfo?(dolske)
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?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(nfroyd)
(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?
Flags: needinfo?(nfroyd) → needinfo?(mh+mozilla)
Sounds sensible.
Flags: needinfo?(mh+mozilla)
Assignee: nobody → nfroyd
Blocks: 1217010
Blocks: 1217015
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.
Attachment #8676911 - Flags: review?(mh+mozilla)
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.
Attachment #8676913 - Flags: review?(mh+mozilla)
Building on the preprocessor.py changes, we can now return the set of
included files from the preprocessed tab SVGs.
Attachment #8676914 - Flags: review?(mh+mozilla)
Attachment #8676912 - Flags: review?(mh+mozilla)
I had a more complicated solution to this in bug 1058662, but this is probably more sensible.
(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 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()
Attachment #8676911 - Flags: review?(mh+mozilla) → review+
Attachment #8676912 - Flags: review?(mh+mozilla) → review+
Attachment #8676913 - Flags: review?(mh+mozilla) → review+
Attachment #8676914 - Flags: review?(mh+mozilla) → review+
(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.
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 44 → mozilla44
You need to log in before you can comment on or make changes to this bug.