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

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Gregory Szorc [:gps]]

We did something similar for js/src in bug 1262241. I think this makes sense for Rust code as well.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1370296 - Ensure rust libraries are fully in the compile graph.

This was not strictly necessary because they would appear as
dependencies of other things in the compile graph, but it makes it
easier to iterate the compile graph for all the node.

Incidentally, this makes the rust libraries on par with rust programs.

Review commit: https://reviewboard.mozilla.org/r/148828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148828/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1370296 - Add compile graphs nodes without dependencies as direct dependencies of the top recursion target.

Incidentally, this prioritizes js/src even more than it was after bug 1262241.

Review commit: https://reviewboard.mozilla.org/r/148830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148830/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1370296 - Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild.

It is technically possible to move it even further up with adjustments
to the top-level moz.build, but that would either be too hackish, or
more involved (and possibly risky for other targets), so I'd rather
keep that for a followup.

Review commit: https://reviewboard.mozilla.org/r/148832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148832/

Comment 6

[Mike Hommey [:glandium]]

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

Comment 7

[Mike Hommey [:glandium]]

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 8

[Gregory Szorc [:gps]]

Comment on attachment 8877422 [details]
Bug 1370296 - Ensure rust libraries are fully in the compile graph.

https://reviewboard.mozilla.org/r/148828/#review153322

Comment 9

[Gregory Szorc [:gps]]

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

Comment 10

[Gregory Szorc [:gps]]

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

Comment 11

[Gregory Szorc [:gps]]

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

Comment 12

[Mike Hommey [:glandium]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8877423 [details]
Bug 1370296 - Add compile graphs nodes without dependencies as direct dependencies of the top recursion target.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148830/diff/1-2/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8877424 [details]
Bug 1370296 - Move toolkit/library/**/rust first in toolkit/toolkit.mozbuild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148832/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/052b55f072ac
https://hg.mozilla.org/mozilla-central/rev/40895af8a56f
https://hg.mozilla.org/mozilla-central/rev/95a364ae6de1