Consider splitting up `build` and `test --no-run`

[aidanhs]

trafficstars

Low priority, but we are losing some information.

See https://github.com/rust-lang-nursery/crater/issues/186#issuecomment-373222369

[Mark-Simulacrum]

I consider build-pass to meant that the crate built (both main and tests, in theory also examples etc. but those I don't think we build today). So I don't actually think we should do this.

[pietroalbini]

I also agree this isn't needed: the build/test split exists because one it's a compiler error and the other is a logic error, but both build and test --no-run are compiler errors on the same codebase.

[aidanhs]

As an alternative point of view, I think it's worth taking a step back to consider why crater exists - it's to detect regressions in compilation of rust code (not detect how much broken rust code there is).

Along these lines, if there were a way to treat errors with more granularity (i.e. verify that the set of errors are identical before and after a PR) then I would say that we do that across the board, even if the crate has never compiled since submission to crates.io! If something will help us identify more cases of writing Rust code that has been broken by a PR, that seems like a good thing. Hence my interest in making these stages more granular.

[Mark-Simulacrum]

My reasoning is primarily based on the fact that if I know a given PR cannot break runtime behavior, then I can ignore changes in test state, and the reverse is also true.

I think I agree with your points about granularity - I'd somewhat expect the HTML view to not entirely reflect how we diff before/after via the string printed, though.

I think of "build" not as "cargo build" but more of "compile-{fail,pass}" and test as run-pass/fail... I think that's where the root of our disagreement lies...

[aidanhs]

Let's consider a concrete scenario:

• foo-rs provides a wrapper around the system library libfoo
• libfoo is not installed inside the crater docker container so will never work
• a PR we're cratering breaks the build of the foo-rs library

Unless we split the two build steps, we miss out on this breakage since we've probably excluded it entirely since the 'build' is always broken.

I'd somewhat expect the HTML view to not entirely reflect how we diff before/after via the string printed, though.

Sorry, not sure what you mean by this.

[Mark-Simulacrum]

Unless we split the two build steps, we miss out on this breakage since we've probably excluded it entirely since the 'build' is always broken.

I'm not clear on how this is the case -- the build of foo-rs will never succeed independent of any implementation on our side; therefore it will be "build-fail" in both proposals/situations as to how we split the build steps.

I'd somewhat expect the HTML view to not entirely reflect how we diff before/after via the string printed, though.

Sorry, not sure what you mean by this.

I mean that our HTML view may indicate build-fail for both test-build-fail and crate-build-fail while we decide whether to show it at all based on the finer-grained details.

Perhaps what I want is build-test-fail and build-crate-fail in the UI; that seems relatively uncontroversial -- however, I don't think the distinction is helpful.

[aidanhs]

When creating an rlib the system linker isn't invoked. Here's an example of library compiling succeeding but test compilation failing:

/tmp/tmp.DJmuaFBgkb $ cargo new --lib mylib
     Created library `mylib` project
/tmp/tmp.DJmuaFBgkb $ cd mylib
/tmp/tmp.DJmuaFBgkb/mylib $ echo -e '#[link = "nonexistinglib"]\nextern { pub fn x(); }' >> src/lib.rs
/tmp/tmp.DJmuaFBgkb/mylib $ mkdir tests
/tmp/tmp.DJmuaFBgkb/mylib $ echo -e 'extern crate mylib;\n#[test]\nfn foo() { unsafe { mylib::x() } }' > tests/foo.rs
/tmp/tmp.DJmuaFBgkb/mylib $ cargo build
   Compiling mylib v0.1.0 (file:///tmp/tmp.DJmuaFBgkb/mylib)
    Finished dev [unoptimized + debuginfo] target(s) in 0.17 secs
/tmp/tmp.DJmuaFBgkb/mylib $ cargo test --no-run
   Compiling mylib v0.1.0 (file:///tmp/tmp.DJmuaFBgkb/mylib)
error: linking with `cc` failed: exit code: 1
  |
[...]
  = note: /tmp/tmp.DJmuaFBgkb/mylib/target/debug/deps/foo-b1dde2a6a30ebed5.1qddbosld5q76e2k.rcgu.o: In function `foo::foo::h24418e3efd8c6806':
          /tmp/tmp.DJmuaFBgkb/mylib/tests/foo.rs:3: undefined reference to `x'
          collect2: error: ld returned 1 exit status
          

error: aborting due to previous error

error: Could not compile `mylib`.

To learn more, run the command again with --verbose.

[aidanhs]

For clarity (after discussing with @Mark-Simulacrum) this means that 'build-fail' splits into 'build-fail' and 'test-build-fail'.

[aidanhs]

Should also do a similar split for cargo check. And also somehow figure out how to differentiate examples and test failures (I don't think we do examples at all at the moment).