Getting Servo tested by crater

[SimonSapin]

rust-lang/rust#43880 proposed a change that could potentially break some code, and cargobomb was used to evaluate the impact. This found a couple issues that were fixed, and then the PR was landed. It only after this change reached the Nightly channel that the Travis-CI cron job at https://github.com/servo/servo-with-rust-nightly/ found that this change broke Servo.

I’d like Servo, or parts of it, to be added to the set of crates that are tested by cargobomb for PRs like rust-lang/rust#43880. Is there some list we can add to?

The two main entry points in https://github.com/servo/servo/ are:

• Servo itself. Build with ./mach build --dev or (cd ports/servo && cargo build). This uses unstable features and is known to compile with the Nightly version (date) specified in ./rust-toolchain. That version is generally updated within days of breakage reaching the Nightly channel.
• The parts going into Firefox (currently only Stylo a.k.a. Quantum CSS). Build with ./mach build-geckolib or (cd ports/geckolib && cargo build). This is known to compile with the release specified in ./rust-stable-version and should work with any later version or Nightly.

By default mach will download appropriate versions of Rust and Cargo and not use those in $PATH. This can be changed with a config file, but running cargo directly might be easier.

We can definitely change or add things to the repository to make it easier for cargobomb to discover what to build/run.

CC @metajack @jonathandturner

[est31]

Is mach needed at all? I've read in servo/servo#18343 that mach is only required on Windows.

[SimonSapin]

@est31 Right, I mentioned running cargo directly without mach.

[est31]

Is there some list we can add to?

There is https://github.com/rust-lang-nursery/cargobomb/blob/master/gh-apps.txt . There is also a gh-candidates.txt not sure what the difference is, both seem to be known by the code.

[aidanhs]

I'll experiment with getting servo tested on the test cargobomb instance. As a first step, I'd probably just get cargo build of the whole thing working (I assume that should work) - don't know how tricky it'll be to add subdirectory support to cargobomb.

Assuming it goes fine: because I'll initially just be doing a build of the whole thing and you use nightly features, I believe you won't listed in beta breakage - just individual PRs.

[SimonSapin]

gh-apps is the subset of gh-candidates repos that have a Cargo.lock file at the root. Only the former is used in lists::read_all_lists.

I see that https://github.com/rust-lang-nursery/cargobomb/blob/46bb193357/src/gh.rs#L16-L29 makes queries to the GitHub search APIs, each time with language:rust. At the root of some repositories like this one, GitHub show some percentages for programming languages used. Presumably, language:foo in search queries is based on the same data.

But for https://github.com/servo/servo/ that data is just missing. I assume that GitHub gives up counting beyond some number of files, and the Servo repo contains many files by vendoring a copy of https://github.com/w3c/web-platform-tests.

I assume that repositories that large are exceptional. What do you think of adding an exception list to the code?

[michiel-de-muynck]

I'd like to voice my support for this as well. Rust PR #44287 was known to break type inference in some cases, and cargobomb was used to preemptively fix the breakage as much as possible. But since cargobomb doesn't test servo, it was only discovered that it broke servo after the PR was already merged.

[SimonSapin]

With various changes in Servo, running cargo test at the root of the repository should now compile and run unit tests.

#163 adds servo/servo to the list of repositories to be tested.

Maybe these two things are enough?

[aidanhs]

Unfortunately servo doesn't pass after #163. It seems that git dependencies aren't supported by crater. I've raised #166.

[SimonSapin]

It’s been a while, and Crater has evolved quite a bit.

In a recent run, https://crater-reports.s3.amazonaws.com/pr-65819/master%2323f890f10202a71168c6424da0cdf94135d3c40c/gh/servo.servo/log.txt shows that the Servo repository is found and cloned, but then the task fails with:

[ERROR] overridden task result to broken:cargo-toml
[ERROR] caused by: invalid Cargo.toml syntax
[ERROR] note: run with `RUST_BACKTRACE=1` to display a backtrace.

This is a bit surprising, since Servo’s root Cargo.toml is valid enough for homu-based CI to run. A GitHub search suggests this error might come from here:

https://github.com/rust-lang/rustwide/blob/f93966386c1dda8d0c9ed3779a389a40b3e707b5/src/prepare.rs#L59-L66

Running cargo read-manifest in my Servo clone, I get:

error: manifest path `/home/simon/projects/servo/Cargo.toml` is a virtual manifest, but this command requires running against an actual package in this workspace

Does Crater not support testing any git repo that contains a virtual workspace?

[pietroalbini]

To be honest Crater doesn't fully support git repositories. For example workspaces don't work, and neither do git dependencies.

[est31]

I wonder if at least Firefox could be tested by crater. It's an important product built on Rust to say the least. There has been a report that rust is showing back compat warnings during the Firefox ESR build. I'm not sure whether the issue would have been caught had it not been reported, although maybe there is an internal Mozilla CI I don't know about that tests for it.

The source code of Firefox is available and regularly published in tarball form, so no need for git or hg support. One job for the latest release of Firefox stable and one for Firefox ESR would be helpful.