Ralph Giles (:rillian) Reporter
	Description • 8 years ago

We have a configure-time check for the required rust version for building with --enable-rust. A similar check in the `mach boostrap` code to decide when to upgrade.

It is easy for these to get out of date. If we want to enforce that they're updated before code requiring new rust or cargo features lands, we should run a build on try and/or landing pushes to verify new commits build with the declared minimum version.

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1321696#c3

	Ralph Giles (:rillian) Reporter
	Comment 1 • 8 years ago

It would be nice if this didn't involve a full build, but I'm not sure how to short-circuit verification. Perhaps we could just build the top-level crates in the tree, if there's a reliable way to enumerate those. Right now, `gkrust` and `gkrust-gtest` are declared as RustLibrary()s to mozbuild, but there will presumedly be separate tool targets and utilities, `cargo test` jobs and so on in the future.

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

Wouldn't it just be simpler to have rust.configure check that the Rust version being used is equivalent to the minimum Rust version if we're running in MOZ_AUTOMATION?

	Ralph Giles (:rillian) Reporter
	Comment 3 • 8 years ago

We could. I think that means:

- Bumping the minimum version more often. I want to continue updating the rust we use in automation to keep the feedback loop as short as possible.
- The version we're running in automation is not always well-defined. Right now we're running 1.14.0-beta; should we require that? It may also be a patched build.

If we're ok with the first one, I could also make bumping the minimum part of the update script which would help a lot, and there'd be code review to help with the second.

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

(In reply to Ralph Giles (:rillian) needinfo me from comment #3)
> - Bumping the minimum version more often. I want to continue updating the
> rust we use in automation to keep the feedback loop as short as possible.

What feedback loop is this?

> - The version we're running in automation is not always well-defined. Right
> now we're running 1.14.0-beta; should we require that? It may also be a
> patched build.

I feel about as comfortable running beta versions of Rust as I do running beta versions of all our other compilers.  That is to say: not very.

I'm inclined to say that we should not require that in configure, but if we absolutely had to do it, we'd need some mechanism to override the check.

	Mike Hommey [:glandium] Assignee
	Comment 5 • 8 years ago

(In reply to Ralph Giles (:rillian) needinfo me from comment #3)
> We could. I think that means:
> 
> - Bumping the minimum version more often.

On the contrary, it could (should) mean bumping less often. As in, don't touch anything until you really need to.

	Simon Sapin (:SimonSapin)
	Comment 6 • 7 years ago

With bug 1351031 fixed CI is now using Rust 1.16.0, but rustc_min_version in build/moz.configure/rust.configure is still 1.15.1. There is a small risk that we check in code that use some of 1.16’s new standard library APIs [1], making 1.16 required by accident.

At the same time, we also want to (at least occasionally) run new (beta or nightly) compiler versions in order to catch compiler regressions. (This is bug 1337955.)

Doubling all of CI is overkill, but at the same time it is possible that the code accidentally using new std API or triggering a new compiler bug is platform-specific. A CI job dedicated to catching the former could use "cargo check". cargo check is new in 1.16 [2], it runs the analysis phase of rustc (type checking, etc.) but not code generation, so it is much faster.

[1] https://blog.rust-lang.org/2017/03/16/Rust-1.16.html#library-stabilizations
[2] https://blog.rust-lang.org/2017/03/16/Rust-1.16.html#whats-in-1160-stable

	Ralph Giles (:rillian) Reporter
	Comment 7 • 7 years ago

(In reply to Simon Sapin (:SimonSapin) from comment #6)

> A CI job dedicated to catching the former could use "cargo check".

This is a great idea! I've opened bug 1354401 to connect this to a mach command so it's easy for anyone to invoke. We can then add a per-checkin or cron-triggered task to verify compatibility the minimum rust version.

For checking upcoming rust releases, we'll still want to do a full build periodically to verify linkage and run unit tests, but that can happen much less frequently.

	Mike Hommey [:glandium] Assignee
	Comment 8 • 7 years ago

Bug 1367734 added code that's only supported with 1.17, so the configure check is now outdated (still on 1.15.1).
Considering there's interest in having a job for the minimal supported version of GCC (while possibly bumping the version we use for what we ship), I think it makes sense to add a complete build job.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 7 years ago

Is there particular try syntax we can use to trigger this new job? I'd like to make sure I use it when doing webrender updates so we don't get more instances of bug 1369615 errors.

	Michael Shal [:mshal]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8873702 [details]
Bug 1321847 - Allow to override the mozharness tooltool manifest from the environment.

https://reviewboard.mozilla.org/r/145096/#review149292

	Michael Shal [:mshal]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8873703 [details]
Bug 1321847 - Add new linux jobs using the baseline of supported toolchains.

https://reviewboard.mozilla.org/r/145098/#review149296

::: taskcluster/ci/build/linux.yml:122
(Diff revision 2)
> +        script: "mozharness/scripts/fx_desktop_build.py"
> +        secrets: true
> +        tooltool-downloads: public
> +        need-xvfb: true
> +
> +linux64-base-toolchains/debug:

Do you anticipate the opt and debug versions of this to be sufficiently different that we'd need both? Or can we get away with just opt?

	Mike Hommey [:glandium] Assignee
	Comment 15 • 7 years ago

(In reply to Michael Shal [:mshal] from comment #14)
> Comment on attachment 8873703 [details]
> Bug 1321847 - Add new linux jobs using the baseline of supported toolchains.
> 
> https://reviewboard.mozilla.org/r/145098/#review149296
> 
> ::: taskcluster/ci/build/linux.yml:122
> (Diff revision 2)
> > +        script: "mozharness/scripts/fx_desktop_build.py"
> > +        secrets: true
> > +        tooltool-downloads: public
> > +        need-xvfb: true
> > +
> > +linux64-base-toolchains/debug:
> 
> Do you anticipate the opt and debug versions of this to be sufficiently
> different that we'd need both? Or can we get away with just opt?

There's been enough build failures happening on one and not the other that it's safer to do both.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> Is there particular try syntax we can use to trigger this new job? I'd like
> to make sure I use it when doing webrender updates so we don't get more
> instances of bug 1369615 errors.

-p linux64-base-toolchains with this landed. It should also happen for -p all, and, obviously it will be happening on every push on integration branches. Specifically for webrender, before even landing in gecko, another way to ensure things will not break is something like https://github.com/servo/webrender/pull/1334

	Pulsebot
	Comment 16 • 7 years ago

Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/4db381159f27
Allow to override the mozharness tooltool manifest from the environment. r=mshal
https://hg.mozilla.org/integration/autoland/rev/d9b7971a270c
Add new linux jobs using the baseline of supported toolchains. r=mshal

	Gregory Szorc [:gps]
	Comment 17 • 7 years ago

Thank you for doing this, glandium! It should be a real time saver to have continuous coverage of these build configurations.

Regarding the implementation, do you think we should do a follow-up to run these build variations for "-p linux64" try syntax? It won't add that much load and I think it could be potentially useful and catch failures on Try before they land. Scope bloating out loud, perhaps we should also fold other build variations like Valgrind and static analysis in as well. I question making developers specify those explicitly because they seem like a critical test for patch development. Perhaps we could be clever and only fold them in if e.g. C++ files change?

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4db381159f27
https://hg.mozilla.org/mozilla-central/rev/d9b7971a270c