Default to --enable-rust (but still allow --disable-rust)

RESOLVED FIXED in Firefox 53

Status

()

Core
Build Config
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: ted, Assigned: larsberg)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Reporter)

Description

10 months ago
Right now building Rust code in Gecko is opt-in, you have to `--enable-rust`. We should switch that to opt-out with `--disable-rust`.

The only blockers to this are making sure that it's easy for developers to get Rust--`mach bootstrap` should install it, and MozillaBuild should ship a copy (until we get msys2 bootstrapping to a point where that's usable by everyone).
(Reporter)

Updated

10 months ago
Blocks: 1284816
(Reporter)

Updated

10 months ago
Depends on: 1286799
(Reporter)

Comment 1

10 months ago
If we fix bug 1286799 I think we could make this change. Since we're already shipping a Firefox release with --enable-rust, we might as well make that the default so that developer builds match what we ship.
(Assignee)

Comment 2

9 months ago
There were some conversations about this during the London workweek:
https://github.com/servo/servo/wiki/London-Oxidation#when-do-we-start-requiring-rust-for-developer-builds-as-well-as-automation

The points there were:
- blocks on Android working properly
- blocks on Windows crash reporting being better
- possibly blocks on jemalloc integration
- question about whether we're going to require libclang and rust-bindgen
- add something to mach bootstrap
(In reply to Lars Bergstrom (:larsberg) from comment #2)
> There were some conversations about this during the London workweek:
> https://github.com/servo/servo/wiki/London-Oxidation#when-do-we-start-
> requiring-rust-for-developer-builds-as-well-as-automation
> 
> The points there were:
> - blocks on Android working properly

I believe this is waiting on a newer Rust version that comes with an armv7 target by default.

> - blocks on Windows crash reporting being better

I am not sure where this one is at.

> - possibly blocks on jemalloc integration

I think we have resolved this issue.

> - question about whether we're going to require libclang and rust-bindgen

Hoo boy. :)

> - add something to mach bootstrap

gps, in conversations on dev-platform today, made it sound like rustup support is in mach bootstrap now?
(Assignee)

Comment 4

7 months ago
Most of these issues have now been resolved. At this point, the plan of record is to wait until after Firefox 52 (the next ESR build) to enable Rust by default. The major linux distributions have indicated that this plan would be the best for their packaging requirements around Rust and libclang/rust-bindgen.
Blocks: 1243581
(In reply to Lars Bergstrom (:larsberg) from comment #4)
> Most of these issues have now been resolved.

Can you be more specific about what you mean by "most"? E.g. as far as I know there hasn't been much progress on "blocks on Windows crash reporting being better".
Flags: needinfo?(larsberg)
Assigning to Lars because he volunteered to drive this bug and its blockers.
Assignee: nobody → larsberg
(Reporter)

Comment 7

6 months ago
I don't think crash reporting is particularly relevant here--we've already shipped Rust code in Firefox releases. We'll need to track crash reporting improvements as we plan to land more important bits of Rust code, but that ship has sailed.

The only thing I would block this on is the bug that's still open--bug 1286799. We need to make sure that developers don't need to do more than `mach bootstrap` to get a build environment that's ready to build Firefox with the default settings. It ought to be a simple patch, FWIW.
I don't believe we've turned Rust support on for Android.  We were waiting for the official ARMv7 builds before we did that, due to performance problems.  I was driving that one forward; I'll put it back on my list for this week.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> I don't think crash reporting is particularly relevant here--we've already
> shipped Rust code in Firefox releases. We'll need to track crash reporting
> improvements as we plan to land more important bits of Rust code, but that
> ship has sailed.

As more Rust code gets into the code base and the number of crashes/panics coming from Rust code grows it will become increasingly relevant. Describing this problem in binary terms ("that ship has sailed") is not useful, IMO.
(Reporter)

Comment 10

6 months ago
(In reply to Nicholas Nethercote [:njn] from comment #9)
> As more Rust code gets into the code base and the number of crashes/panics
> coming from Rust code grows it will become increasingly relevant. Describing
> this problem in binary terms ("that ship has sailed") is not useful, IMO.

I am fully in agreement with you, but this bug doesn't really have any bearing on that, does it? This is just about making the default build configuration match what we're already shipping. I definitely think the crash reporting issues should block things like "ship stylo", but we didn't let them block shipping what we have now (for better or worse) and making everyone's builds match that reality seems like the sensible thing to do.
(Reporter)

Comment 11

6 months ago
(In reply to Nathan Froyd [:froydnj] from comment #8)
> I don't believe we've turned Rust support on for Android.  We were waiting
> for the official ARMv7 builds before we did that, due to performance
> problems.  I was driving that one forward; I'll put it back on my list for
> this week.

Thanks for the reminder! We could add bug 1220307 as a blocker for this, or we could not enable rust by default on Android.
(Assignee)

Comment 12

6 months ago
Poor crash dump support should be fixed by the Rust updates to include debug info, tracked by Bug 1268328.
Flags: needinfo?(larsberg)
(Assignee)

Updated

6 months ago
Depends on: 1268328

Updated

6 months ago
Depends on: 1314477
Depends on: 1319332
Comment hidden (mozreview-request)
(Reporter)

Comment 14

5 months ago
mozreview-review
Comment on attachment 8814400 [details]
Bug 1283898 - Enable rust by default.

https://reviewboard.mozilla.org/r/95506/#review95756

Yay!
Attachment #8814400 - Flags: review?(ted) → review+
You should remove --enable-rust from in-tree mozconfigs.

Comment 16

5 months ago
I understand the excitement to make this change. But as I'm going through the Build Config component backlog from the long weekend, a lot of people ran into `mach bootstrap` issues installing Rust. I'm a bit concerned that forcing Rust onto people with a known buggy bootstrap will be a bit frustrating and disruptive.

That being said, Ralph seems to be addressing many of the bootstrap issues (go Ralph!). My only request is we try to fix the high-impact bootstrapper bugs before landing this change. But knowing Ralph and judging by his quick reactions to bootstrap bugs, my guess is this was his plan all along. So carry on and \o/ for this milestone.

Updated

5 months ago
Depends on: 1320940

Updated

5 months ago
Depends on: 1321073
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
In addition to the bootstrap bugs, it turns out we have a bunch of automation tasks which call configure, but didn't have a rust toolchain in tooltool or add the paths through mozconfig.rust. These additional patches are a start on fixing those, but are not a complete solution. If someone who understands the gradle stuff wants to take a look, that would help.

Comment 22

5 months ago
mozreview-review
Comment on attachment 8816159 [details]
Bug 1283898 - Add rust to more automation builds.

https://reviewboard.mozilla.org/r/96934/#review97272

These look fine, though from the try results it seems you might still be missing some.
Attachment #8816159 - Flags: review?(mshal) → review+

Comment 23

5 months ago
mozreview-review
Comment on attachment 8816161 [details]
Bug 1283898 - Make tooltool rust available in all automation.

https://reviewboard.mozilla.org/r/96938/#review97274

::: build/mozconfig.common:27
(Diff revision 1)
>  MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-0}
>  
>  ac_add_options --enable-js-shell
>  
>  . "$topsrcdir/build/mozconfig.automation"
> +. "$topsrcdir/build/mozconfig.rust"

I'm pretty sure we include mozconfig.common from every mozconfig, which means we should be able to remove the "$topsrcdir/build/mozconfig.rust" lines from all the others. Please remove them so we're only including mozconfig.rust once.

::: build/mozconfig.common:27
(Diff revision 1)
>  MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-0}
>  
>  ac_add_options --enable-js-shell
>  
>  . "$topsrcdir/build/mozconfig.automation"
> +. "$topsrcdir/build/mozconfig.rust"

Also I believe froydnj ran into some issues including mozconfig.rust from configs that have the compile environment disabled. It looks like this might be another source of errors in your try push. We'll probably need some way to conditionally include mozconfig.rust or find some other workaround in configure.
Attachment #8816161 - Flags: review?(mshal) → review-

Comment 24

5 months ago
mozreview-review
Comment on attachment 8816160 [details]
Bug 1283898 - Put rust in path for Spidermonkey automation.

https://reviewboard.mozilla.org/r/96936/#review97562
Attachment #8816160 - Flags: review?(gps) → review+
Depends on: 1322323
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

5 months ago
mozreview-review-reply
Comment on attachment 8816161 [details]
Bug 1283898 - Make tooltool rust available in all automation.

https://reviewboard.mozilla.org/r/96938/#review97274

> Also I believe froydnj ran into some issues including mozconfig.rust from configs that have the compile environment disabled. It looks like this might be another source of errors in your try push. We'll probably need some way to conditionally include mozconfig.rust or find some other workaround in configure.

Yes, we can't even pass `RUSTC=path` for `--disable-compile-environment` builds. I addressed this by unsetting the variables in the artifact-build mozconfigs like we do for `CC` and `CXX`.

Comment 33

5 months ago
mozreview-review
Comment on attachment 8816161 [details]
Bug 1283898 - Make tooltool rust available in all automation.

https://reviewboard.mozilla.org/r/96938/#review97278

::: build/mozconfig.common:27
(Diff revision 1)
>  MOZ_REQUIRE_SIGNING=${MOZ_REQUIRE_SIGNING-0}
>  
>  ac_add_options --enable-js-shell
>  
>  . "$topsrcdir/build/mozconfig.automation"
> +. "$topsrcdir/build/mozconfig.rust"

Fair enough. I've removed the redundant includes.

Comment 34

5 months ago
mozreview-review
Comment on attachment 8818383 [details]
Bug 1283898 - Add tooltool rust to path for js hazard jobs.

https://reviewboard.mozilla.org/r/98444/#review98766
Attachment #8818383 - Flags: review?(gps) → review+
Looks like this version is green on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d12680642747a1ce4a56a08842b4664edf8580dc

Comment 36

5 months ago
mozreview-review
Comment on attachment 8816161 [details]
Bug 1283898 - Make tooltool rust available in all automation.

https://reviewboard.mozilla.org/r/96938/#review99124
Attachment #8816161 - Flags: review+

Comment 37

5 months ago
mozreview-review
Comment on attachment 8818382 [details]
Bug 1283898 - Update linux64 tooltool rust to support i686.

https://reviewboard.mozilla.org/r/98442/#review99126
Attachment #8818382 - Flags: review+

Comment 38

5 months ago
mozreview-review
Comment on attachment 8818384 [details]
Bug 1283898 - Don't set RUSTC for artifact builds.

https://reviewboard.mozilla.org/r/98446/#review99128
Attachment #8818384 - Flags: review+
Thanks, gps. Taking this r+ since mshal seems to be away.

Comment 40

5 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7aba018f7f13
Add rust to more automation builds. r=mshal
https://hg.mozilla.org/integration/autoland/rev/32b4d0c6aef9
Put rust in path for Spidermonkey automation. r=gps
https://hg.mozilla.org/integration/autoland/rev/71bc383bef23
Enable rust by default. r=ted
https://hg.mozilla.org/integration/autoland/rev/71ca80cc382f
Make tooltool rust available in all automation. r=gps
https://hg.mozilla.org/integration/autoland/rev/b39b3fe699dc
Update linux64 tooltool rust to support i686. r=gps
https://hg.mozilla.org/integration/autoland/rev/a740323ace17
Add tooltool rust to path for js hazard jobs. r=gps
https://hg.mozilla.org/integration/autoland/rev/32c25e3f57f6
Don't set RUSTC for artifact builds. r=gps
Comment hidden (mozreview-request)

Comment 42

5 months ago
mozreview-review
Comment on attachment 8819055 [details]
Bug 1283898 - Don't check for rust outside of toolkit builds.

https://reviewboard.mozilla.org/r/98922/#review99172

r=me pending good results on try. Delegated r=bustage review authority from gps on irc.
Attachment #8819055 - Flags: review+

Comment 43

5 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9fdc170dbdd
Don't check for rust outside of toolkit builds. r=rillian

Comment 44

5 months ago
mozreview-review
Comment on attachment 8819055 [details]
Bug 1283898 - Don't check for rust outside of toolkit builds.

https://reviewboard.mozilla.org/r/98922/#review99196

I know this already landed, but why not.
Attachment #8819055 - Flags: review?(gps) → review+

Comment 45

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7aba018f7f13
https://hg.mozilla.org/mozilla-central/rev/32b4d0c6aef9
https://hg.mozilla.org/mozilla-central/rev/71bc383bef23
https://hg.mozilla.org/mozilla-central/rev/71ca80cc382f
https://hg.mozilla.org/mozilla-central/rev/b39b3fe699dc
https://hg.mozilla.org/mozilla-central/rev/a740323ace17
https://hg.mozilla.org/mozilla-central/rev/32c25e3f57f6
https://hg.mozilla.org/mozilla-central/rev/f9fdc170dbdd
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 46

5 months ago
https://hg.mozilla.org/comm-central/rev/fe15036210c7f18a05cd15c570b50b4500259cc2
Keep mozconfig.common in sync (Bug 1283898). rs=bustage-fix CLOSED TREE DONTBUILD

Updated

5 months ago
Depends on: 1324120

Updated

4 months ago
Attachment #8816161 - Flags: review?(mshal)

Updated

4 months ago
Attachment #8818384 - Flags: review?(mshal)

Updated

4 months ago
Attachment #8818382 - Flags: review?(mshal)
Sorry, I forgot to set away status on bugzilla.
status-firefox50: affected → ---
Depends on: 1325932
You need to log in before you can comment on or make changes to this bug.