Don't fail early if try_run returns an error

[GuillaumeGomez]

Fixes #113208.

Follow-up of #112962.

r? @jyn514

[jyn514]

going to be busy the next two weeks

r? bootstrap

note that i haven't actually confirmed fail-fast by running it, just by glancing at the code, so it probably makes sense to verify locally both that it was broken before and fixed after.

[bors]

☔ The latest upstream changes (presumably #113256) made this pull request unmergeable. Please resolve the merge conflicts.

[onur-ozkan]

I don't see this PR making any difference on the behaviour.

--

I don't think --no-fail-fast flag is even functioning. Whether I set --no-fail-fast or not, tests always wait for others to finish, even on failure.

I check that by simply executing the ui tests with and without --no-fail-fast on 32d81ec and 76e79ca

[jyn514]

@ozkanonur it only affects the behavior between different Steps, not within a single compiletest invocation. try running both UI and rustdoc-ui.

[onur-ozkan]

@ozkanonur it only affects the behavior between different Steps, not within a single compiletest invocation. try running both UI and rustdoc-ui.

Oh, my bad!

I don't see this PR making any difference on the behaviour.

Same, --no-fail-fast already works.

I break one of the test in rustdoc and then run x test rustdoc rustdoc-gui --no-fail-fast.

here is the result

  ~/devspace/.other/rust-dist/general  $ ~/devspace/ozkanonur/rust/x test rustdoc rustdoc-gui --no-fail-fast
Building bootstrap
    Finished dev [unoptimized] target(s) in 0.02s
warning: creating symbolic link `/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/ci-rustc-sysroot/lib/rustlib/rustc-src/rust` to `/home/nimda/devspace/ozkanonur/rust` failed with File exists (os error 17)
warning: creating symbolic link `/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/rustc-src/rust` to `/home/nimda/devspace/ozkanonur/rust` failed with File exists (os error 17)
Creating a sysroot for stage2 compiler (use `rustup toolchain link 'name' build/host/stage2`)
Building stage0 library artifacts (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.07s
Building stage0 tool compiletest (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.07s
Building tool rustdoc (stage1 -> stage2, x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.08s
Testing stage2 compiletest suite=rustdoc mode=rustdoc (x86_64-unknown-linux-gnu)
warning: `tidy` is not installed; diffs will not be generated

running 637 tests
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii  88/637
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii 176/637
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii 264/637
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii 352/637
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii 440/637
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii 528/637
iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii 616/637
iiiiiiiiiiiiii......F

failures:

---- [rustdoc] tests/rustdoc/primitive/no_std.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/bin/python3" "/home/nimda/devspace/ozkanonur/rust/src/etc/htmldocck.py" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/test/rustdoc/primitive/no_std" "/home/nimda/devspace/ozkanonur/rust/tests/rustdoc/primitive/no_std.rs"
stdout: none
--- stderr -------------------------------
6: @has check failed
        `XPATH PATTERN` did not match
        // @has no_std/fn.foo.html '//a/[@href="{{channel}}/core/primitive.u5.html"]' 'primitive link'

Encountered 1 errors
------------------------------------------



failures:
    [rustdoc] tests/rustdoc/primitive/no_std.rs

test result: FAILED. 6 passed; 1 failed; 630 ignored; 0 measured; 0 filtered out; finished in 163.09ms

Some tests failed in compiletest suite=rustdoc mode=rustdoc host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
Building stage0 tool rustdoc-gui-test (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.07s
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
    Finished dev [unoptimized + debuginfo] target(s) in 0.28s
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Running 107 rustdoc-gui (24 concurrently) ...
.......... (10/107)
.......... (20/107)
.......... (30/107)
.......... (40/107)
.......... (50/107)
.......... (60/107)
.......... (70/107)
.......... (80/107)
.......... (90/107)
.......... (100/107)
.......    (107/107)

        finished in 11.978 seconds

1 command(s) did not execute successfully:
  - BOOTSTRAP_CARGO="/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" DOC_RUST_LANG_ORG_CHANNEL="https://doc.rust-lang.org/nightly" LD_LIBRARY_PATH="/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage0/lib" RUSTC="/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage0/bin/rustc" RUSTC_BOOTSTRAP="1" RUSTC_FORCE_RUSTC_VERSION="compiletest" RUST_TEST_THREADS="24" RUST_TEST_TMPDIR="/home/nimda/devspace/.other/rust-dist/general/build/tmp" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--rustdoc-path" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" "--src-base" "/home/nimda/devspace/ozkanonur/rust/tests/rustdoc" "--build-base" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/test/rustdoc" "--sysroot-base" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/stage2" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--suite" "rustdoc" "--mode" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/ci-llvm/bin/FileCheck" "--nodejs" "/bin/node" "--npm" "/bin/npm" "--optimize-tests" "--host-rustcflags" "-Crpath" "--host-rustcflags" "-Cdebuginfo=0" "--host-rustcflags" "-Lnative=/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath" "--target-rustcflags" "-Cdebuginfo=0" "--target-rustcflags" "-Lnative=/home/nimda/devspace/.other/rust-dist/general/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--python" "/bin/python3" "--json" "--llvm-version" "16.0.5-rust-1.72.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets
analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfologicalview debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker dwarflinkerparallel dwp engine executionengine extensions filecheck frontendhlsl frontendopenacc frontendopenmp fuzzercli fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irprinter irreader jitlink libdriver lineeditor linker loongarch loongarchasmparser loongarchcodegen loongarchdesc loongarchdisassembler loongarchinfo lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo mc
mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts objcopy object objectyaml option orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvtargetmca runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target targetparser textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo webassemblyutils windowsdriver windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info x86targetmca xray" "--cc" "" "--cxx" "" "--cflags" "" "--cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/local/tmp/work" "--android-cross-path" "" "--channel" "dev"

Build completed unsuccessfully in 0:00:12
  ~/devspace/.other/rust-dist/general  $

[jyn514]

hmm. looking at the code those Steps don't use try_run. maybe try x test tidy bootstrap --no-fail-fast?

[onur-ozkan]

hmm. looking at the code those Steps don't use try_run. maybe try x test tidy bootstrap --no-fail-fast?

Right, I forgot try_run is involved in the rustdoc-gui-test tool not directly in the bootstrapping. Now I can see that PR fixes the issue.

--

Shouldn't we update this too?

https://github.com/rust-lang/rust/blob/70f25e580fdd5a3c3f6c10a4b26acec6b02bbd1c/src/bootstrap/test.rs#L2675

[jyn514]

fwiw i am reconsidering whether try_run should even return a Result, a boolean seems fine

[onur-ozkan]

fwiw i am reconsidering whether try_run should even return a Result, a boolean seems fine

Agree. This could cause problems with future PRs because it's very easy to break --no-fail-fast again by adding unwrap at the end of the fn try_run which is not easy to catch on large PRs.

Do you have time to handle that change? You can pass it to me if you don't have enough time. @GuillaumeGomez

[GuillaumeGomez]

That makes me sad to not rely on the type system to prevent the issue we encountered but yes, I can put back a boolean.

[jyn514]

the function you modified wasn't even in bootstrap, it was in build_helper. build_helper uses it very differently.

[GuillaumeGomez]

I think the same rationale should be applied: it's easy to forget to check a boolean, it's impossible to forget to check a Result. However it's out of the debate here. I reverted the changes to bool. As long as rustdoc-gui is failing when it's supposed to, I'm happy. I was just hoping that it would make life of people working on bootstrap easier. Hopefully a better solution will be found. :)

[onur-ozkan]

I think the same rationale should be applied: it's easy to forget to check a boolean, it's impossible to forget to check a Result

True, but the amount of work involved is smaller compared to the bootstrap and bootstrap gets more changes from PRs than rustdoc-gui-test.

What do you think about adding a wrapper function called run_or_fail for try_run in rustdoc-gui-test which will give a Result output? Alternatively, we could create a simple macro called true_or_fail(maybe we already have something like this in rust tree?) to check the boolean values. By implementing one of these, we can minimize the number of potential issues in the tests for rustdoc-gui.

[GuillaumeGomez]

The problem in rustdoc-gui is solved, so I'd rather have a global solution rather than a local one. I'll keep an eye on rustdoc-gui tests to ensure we don't have a regression until a better solution is reached.

[jyn514]

there are two different try_run functions. you should revert the change to the one in bootstrap but keep the change to the one in build_helper.

[onur-ozkan]

there are two different try_run functions

Maybe rename that one to safe_run or something ? :) Seems like its causing confusions

[GuillaumeGomez]

I only reverted the change in build_helper this time.

[GuillaumeGomez]

jyn514 provided a nice explanation for this: if the state is stored in try_run, the function return a bool, otherwise a Result. I followed their advice and updated as explained/

[onur-ozkan]

Thanks for the fix!

Just so you know, there is another try_run with Result in the bootstrap code.

rust/src/bootstrap/lib.rs

Line 336 in d8899c5

try_run(cmd: &mut Command) -> Result<(), ()>,

It doesn't cause any specific issues like the ones you fixed in this pull request. I wanted to bring it to your attention so that you're aware of it in case you want to update it as well.

The PR already looks good to me, r=me whether or not you make changes to that function.

[GuillaumeGomez]

This one doesn't store a state so I didn't update it. If I'm wrong we can always fix in a follow-up. ;)

Thanks to both of you for the reviews!

@bors r=ozkanonur,jyn514 rollup

[bors]

📌 Commit 98336f8 has been approved by ozkanonur,jyn514

It is now in the queue for this repository.

[bors]

⌛ Testing commit 98336f8 with merge 420abeba7674df8f37dbafe0a18d3b8e93eda61a...

[bors]

💥 Test timed out

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[GuillaumeGomez]

@bors retry

[bors]

⌛ Testing commit 98336f8 with merge 1e6db34...

[bors]

☀️ Test successful - checks-actions
Approved by: ozkanonur,jyn514
Pushing 1e6db34 to master...

[rust-timer]

Finished benchmarking commit (1e6db34): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.774s -> 657.557s (-0.03%)