Closed Bug 1370296 Opened 7 years ago Closed 7 years ago

Run `force-cargo-build` as early during the build process as possible

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: nika, Assigned: glandium)

References

Details

Attachments

(3 files)

Bug 1370296 - Ensure rust libraries are fully in the compile graph.
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1370296 - Add compile graphs nodes without dependencies as direct dependencies of the top recursion target.
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1370296 - Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild.
59 bytes, text/x-review-board-request
gps
: review+
Details 
Rust takes a while to build. It would be nice if we could try to run it as early as possible during the build to avoid the chance that we're waiting on rust code to compile when the build is complete.

This is especially a problem when using icecream, as the C++ code ends up compiling much more quickly than rust code.

Ehsan reported that enabling stylo builds on his machine increased build times significantly, due to rust not starting to build until midway through.
We did something similar for js/src in bug 1262241. I think this makes sense for Rust code as well.
Assignee: nobody → mh+mozilla
Without the patches:
$ make -j --no-print-directory -C obj-x86_64-pc-linux-gnu/ compile RECURSE_TRACE_ONLY=1 | grep -n '\(js/src/target\|rust/target\|toolkit/library/target\)'
22:js/src/target
103:toolkit/library/gtest/rust/target
172:toolkit/library/rust/target
608:toolkit/library/target

With the patches:
$ make -j --no-print-directory -C obj-x86_64-pc-linux-gnu/ compile RECURSE_TRACE_ONLY=1 | grep -n '\(js/src/target\|rust/target\|toolkit/library/target\)'
2:js/src/target
44:toolkit/library/gtest/rust/target
45:toolkit/library/rust/target
606:toolkit/library/target
We can do better, but as mentioned in the last commit message, I'd rather do that in a followup because the changes I want to do in top-level moz.build are risky.
Comment on attachment 8877422 [details]
Bug 1370296 - Ensure rust libraries are fully in the compile graph.

https://reviewboard.mozilla.org/r/148828/#review153322
Attachment #8877422 - Flags: review?(gps) → review+
Comment on attachment 8877423 [details]
Bug 1370296 - Add compile graphs nodes without dependencies as direct dependencies of the top recursion target.

https://reviewboard.mozilla.org/r/148830/#review153324
Attachment #8877423 - Flags: review?(gps) → review+
Comment on attachment 8877424 [details]
Bug 1370296 - Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild.

https://reviewboard.mozilla.org/r/148832/#review153326

I guess the order of DIRS impacts build order then. I guess that was already true. But I hadn't really considered that! Feels a bit fragile and something that could break at any time. I guess that's something we'll need to keep in mind whenever we touch DIRS processing code.

This series reminds me that we should set up a build variation that basically does `mach build -j8` without sccache and a default mozconfig so we have Perfherder metrics for how long a simulated developer build takes. Not sure if there is a bug on that. I know Ted was saying something about it a few weeks ago...
Attachment #8877424 - Flags: review?(gps) → review+
(In reply to Mike Hommey [:glandium] from comment #7)
> We can do better, but as mentioned in the last commit message, I'd rather do
> that in a followup because the changes I want to do in top-level moz.build
> are risky.

I'd be tempted to add special syntax to DIRS values to denote them as high priority. e.g. "path:priority" or a tuple of (path, priority) or something. The super hacky thing would be to write a custom sort function for target names in the backend. But that would be a gross layering violation. But if the build system isn't a bunch of hacks, I don't know what is!
(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 8877424 [details]
> Bug 1370296 - Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild.
> 
> https://reviewboard.mozilla.org/r/148832/#review153326
> 
> I guess the order of DIRS impacts build order then. I guess that was already
> true. But I hadn't really considered that! Feels a bit fragile and something
> that could break at any time. I guess that's something we'll need to keep in
> mind whenever we touch DIRS processing code.

As you note, that always was true, to some degree. To do things like this and bug 1262241, we need something signaling that we prefer some things going in as early as possible. It could be something else than DIRS, but it could just as well stay DIRS...
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/30424746d0f7
Ensure rust libraries are fully in the compile graph. r=gps
https://hg.mozilla.org/integration/autoland/rev/b27d3da9e60e
Add compile graphs nodes without dependencies as direct dependencies of the top recursion target. r=gps
https://hg.mozilla.org/integration/autoland/rev/32edaa349976
Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild. r=gps
Blocks: 1372813
Backed out for build bustage, failing test_recursivemake.py:

https://hg.mozilla.org/integration/autoland/rev/ad3f1138ce6f199408ad58d65c7476636e924909
https://hg.mozilla.org/integration/autoland/rev/6cb64eddc3df7ad195c36452485e7e511f92a73d
https://hg.mozilla.org/integration/autoland/rev/e3a144eae0e13e323ee547dc5fda9431819b4a7e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=32edaa3499760c5effbf625f8029006a737941cb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=106921207&repo=autoland

task 2017-06-14T07:00:41.594381Z] 07:00:41     INFO - TEST-PASS | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/backend/test_recursivemake.py | TestRecursiveMakeBackend.test_resources
[task 2017-06-14T07:00:41.594421Z] 07:00:41     INFO - TEST-PASS | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/backend/test_recursivemake.py | TestRecursiveMakeBackend.test_rust_library
[task 2017-06-14T07:00:41.594462Z] 07:00:41     INFO - TEST-PASS | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/backend/test_recursivemake.py | TestRecursiveMakeBackend.test_rust_library_with_features
[task 2017-06-14T07:00:41.594528Z] 07:00:41  WARNING - TEST-UNEXPECTED-FAIL | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/backend/test_recursivemake.py | TestRecursiveMakeBackend.test_rust_programs, line 840: False is not true
[task 2017-06-14T07:00:41.594562Z] 07:00:41     INFO - FAIL: Test that {HOST_,}RUST_PROGRAMS are written to backend.mk correctly.
[task 2017-06-14T07:00:41.594583Z] 07:00:41     INFO - Traceback (most recent call last):
[task 2017-06-14T07:00:41.594619Z] 07:00:41     INFO -   File "/home/worker/workspace/build/src/python/mozbuild/mozbuild/test/backend/test_recursivemake.py", line 840, in test_rust_programs
[task 2017-06-14T07:00:41.594649Z] 07:00:41     INFO -     self.assertTrue(any(l == 'recurse_compile: code/host code/target' for l in lines))
[task 2017-06-14T07:00:41.594670Z] 07:00:41     INFO - AssertionError: False is not true
Flags: needinfo?(mh+mozilla)
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/052b55f072ac
Ensure rust libraries are fully in the compile graph. r=gps
https://hg.mozilla.org/integration/autoland/rev/40895af8a56f
Add compile graphs nodes without dependencies as direct dependencies of the top recursion target. r=gps
https://hg.mozilla.org/integration/autoland/rev/95a364ae6de1
Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild. r=gps
Flags: needinfo?(mh+mozilla)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.