1321847 - Add a task to verify builds with the minimum rust version

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ralph Giles (:rillian)]

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

Comment 1

[Ralph Giles (:rillian)]

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.

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Ralph Giles (:rillian)]

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.

Comment 4

[Nathan Froyd [:froydnj]]

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

Comment 5

[Mike Hommey [:glandium]]

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

Comment 6

[Simon Sapin (:SimonSapin)]

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

Comment 7

[Ralph Giles (:rillian)]

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

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1321847 - Allow to override the mozharness tooltool manifest from the environment.

The main motivation behind this change is that going towards toolchain
tasks hooked up in the task graph (bug 1313111), we're going to end up
with jobs using both taskcluster toolchain job and tooltool artifacts
for their toolchain needs. With the current setup, this means the
toolchain dependencies will be spread between taskcluster task graph
definition and mozharness configuration.

It also makes things more complex to provide a command that pulls the
right toolchains from both taskcluster and tooltool (bug 1356529),
because one needs to find and parse the mozharness config (which also
happens to be python code that uses platform-specific things, so e.g.
reading windows mozharness config fails on other platforms).

All in all, moving the tooltool manifest path to the taskcluster task
definitions would make things simpler, and would also allow make patches
switching from tooltool to taskcluster artifacts more straightforward to
validate.

But since some build types still run on buildbot, we'll have to keep
part of the current setup using mozharness configs. So we allow to
override the tooltool manifest path from the environment, and we'll rely
on taskcluster task definitions being able to set environment variables.

Actually moving the relevant tooltool manifest paths from mozharness
config to taskcluster task definitions is left for a followup.

Another followup is to move the tooltool manifest paths declared in
some ad-hoc build scripts to taskcluster task definitions as well.

The immediate need for this, though, is to allow to have duplicated jobs
that only differ in their tooltool manifest, without duplicating a
complete mozharness config that will end up stale (the goal being that
really only the tooltool manifest differs, even when the original jobs
change).

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

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1321847 - Add new linux jobs using the baseline of supported toolchains.

For a long time, we've kind of forced the GCC version used to compile
Firefox on automation to the minimum version we do support, because
using a newer version would pretty much guarantee that builds with older
versions would break.

Ideally, the same would be true of rust, but it's not the case, and sure
enough, building with older versions breaks. The most recent example is
bug 1367734 making rustc 1.17.0 required but leaving configure checking
for version 1.15.1.

There are multiple reasons why we'd want to use newer versions of rust
to build shipping versions of Firefox other than language requirements,
but we should still ensure building with supported versions of rust
doesn't break silently.

Here we add a set of new linux jobs that build opt and debug build with
the baseline of supported toolchains. At the moment, that's GCC 4.9,
rust 1.15.1, and clang 3.9 (for bindgen). That's a copy of the current
toolchains used for normal linux jobs, with rustc downgraded to the
package used after bug 1338311.

Further down the line, we'll be able to bump the versions of GCC, rust
and/or clang for the shipped Firefox builds, while keeping those jobs on
GCC 4.9, rust 1.15.1 and clang 3.9, until we do intentionally want to
bump those versions (as well as the corresponding configure checks).

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

Comment 11

[Mike Hommey [:glandium]]

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

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

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 13

[Michael Shal [:mshal]]

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

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

Comment 14

[Michael Shal [:mshal]]

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?

Comment 15

[Mike Hommey [:glandium]]

(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:[e-mail]) 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

Comment 16

[Pulsebot]

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

Comment 17

[Gregory Szorc [:gps]]

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?

Comment 18

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

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