(not currently active) Ted Mielczarek Reporter
	Description • 8 years 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).

	(not currently active) Ted Mielczarek Reporter
	Comment 1 • 8 years 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.

	Lars Bergstrom (:larsberg) Assignee
	Comment 2 • 8 years 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

	Nathan Froyd [:froydnj]
	Comment 3 • 8 years ago

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

	Lars Bergstrom (:larsberg) Assignee
	Comment 4 • 8 years 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.

	Nicholas Nethercote [inactive]
	Comment 5 • 8 years ago

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

	Chris Peterson [:cpeterson]
	Comment 6 • 8 years ago

Assigning to Lars because he volunteered to drive this bug and its blockers.

	(not currently active) Ted Mielczarek Reporter
	Comment 7 • 8 years 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.

	Nathan Froyd [:froydnj]
	Comment 8 • 8 years ago

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.

	Nicholas Nethercote [inactive]
	Comment 9 • 8 years ago

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

	(not currently active) Ted Mielczarek Reporter
	Comment 10 • 8 years 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.

	(not currently active) Ted Mielczarek Reporter
	Comment 11 • 8 years 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.

	Lars Bergstrom (:larsberg) Assignee
	Comment 12 • 8 years ago

Poor crash dump support should be fixed by the Rust updates to include debug info, tracked by Bug 1268328.

	(not currently active) Ted Mielczarek Reporter
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8814400 [details]
Bug 1283898 - Enable rust by default.

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

Yay!

	Mike Hommey [:glandium]
	Comment 15 • 8 years ago

You should remove --enable-rust from in-tree mozconfigs.

	Gregory Szorc [:gps]
	Comment 16 • 8 years 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.

	Ralph Giles (:rillian)
	Comment 21 • 8 years ago

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.

	Michael Shal [:mshal]
	Comment 22 • 8 years 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.

	Michael Shal [:mshal]
	Comment 23 • 8 years 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.

	Gregory Szorc [:gps]
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8816160 [details]
Bug 1283898 - Put rust in path for Spidermonkey automation.

https://reviewboard.mozilla.org/r/96936/#review97562

	Ralph Giles (:rillian)
	Comment 32 • 8 years 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`.

	Ralph Giles (:rillian)
	Comment 33 • 8 years 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.

	Gregory Szorc [:gps]
	Comment 34 • 8 years 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

	Ralph Giles (:rillian)
	Comment 35 • 8 years ago

Looks like this version is green on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d12680642747a1ce4a56a08842b4664edf8580dc

	Gregory Szorc [:gps]
	Comment 36 • 8 years ago
mozreview-review

Comment on attachment 8816161 [details]
Bug 1283898 - Make tooltool rust available in all automation.

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

	Gregory Szorc [:gps]
	Comment 37 • 8 years ago
mozreview-review

Comment on attachment 8818382 [details]
Bug 1283898 - Update linux64 tooltool rust to support i686.

https://reviewboard.mozilla.org/r/98442/#review99126

	Gregory Szorc [:gps]
	Comment 38 • 8 years ago
mozreview-review

Comment on attachment 8818384 [details]
Bug 1283898 - Don't set RUSTC for artifact builds.

https://reviewboard.mozilla.org/r/98446/#review99128

	Ralph Giles (:rillian)
	Comment 39 • 8 years ago

Thanks, gps. Taking this r+ since mshal seems to be away.

	Pulsebot
	Comment 40 • 8 years 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

	Ralph Giles (:rillian)
	Comment 42 • 8 years 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.

	Pulsebot
	Comment 43 • 8 years 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

	Gregory Szorc [:gps]
	Comment 44 • 8 years 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.

	Carsten Book [:Tomcat]
	Comment 45 • 8 years 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

	aleth [:aleth]
	Comment 46 • 8 years ago

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

	Michael Shal [:mshal]
	Comment 47 • 7 years ago

Sorry, I forgot to set away status on bugzilla.