Skip to content

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Mar 27, 2025

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

madsmtm and others added 23 commits March 27, 2025 03:34
I've intentionally used slightly vague language ("roughly"), as we don't
want to guarantee the exact invocation of `xcrun`, just hint that it's
close to that.
It can be a fairly expensive operation when the output is not cached, so
it's nice to get some visibility into the runtime cost.
Also allow the SDK path to be non-UTF-8.
We haven't had any Windows XP targets for a long while now...
…en first non-crate items and the crate items
…rsing, r=fmease

Greatly simplify doctest parsing and information extraction

The original process was pretty terrible, as it tried to extract information such as attributes by performing matches over tokens like `#!`, which doesn't work very well considering you can have `#   ! [`, which is valid.

Also, it now does it in one pass: if the parser is happy, then we try to extract information, otherwise we return early.

r? `@fmease`
…eywiser

Improve `xcrun` error handling

The compiler invokes `xcrun` on macOS when linking Apple targets, to find the Xcode SDK which contain all the necessary linker stubs. The error messages that `xcrun` outputs aren't always that great though, so this PR tries to improve that by providing extra context when an error occurs.

Fixes rust-lang#56829.
Fixes rust-lang#84534.
Part of rust-lang#129432.
See also the alternative rust-lang#131433.

Tested on:
- `x86_64-apple-darwin`, MacBook Pro running Mac OS X 10.12.6
    - With no tooling installed
    - With Xcode 9.2
    - With Xcode 9.2 Commandline Tools
- `aarch64-apple-darwin`, MacBook M2 Pro running macOS 14.7.4
    - With Xcode 13.4.1
    - With Xcode 16.2
    - Inside `nix-shell -p xcbuild` (nixpkgs' `xcrun` shim)
- `aarch64-apple-darwin`, VM running macOS 15.3.1
    - With no tooling installed
    - With Xcode 16.2 Commandline Tools

`@rustbot` label O-apple
r? compiler
CC `@BlackHoleFox` `@thomcc`
…Denton

std: get rid of pre-Vista fallback code

We haven't had any Windows XP targets for a long while now...

r? ChrisDenton
…errors

Use `abs_diff` where applicable

Very small cleanup, dogfooding a [new clippy lint](rust-lang/rust-clippy#14482) I'm trying to add
saethlin goes on vacation

Someone should bug me in about 2 weeks if I don't remember to undo this
@rustbot rustbot added the rollup A PR which is a rollup label Mar 27, 2025
@jhpratt
Copy link
Member Author

jhpratt commented Mar 27, 2025

@bors r+ rollup=never p=5

@bors
Copy link
Collaborator

bors commented Mar 27, 2025

📌 Commit 57672fb has been approved by jhpratt

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 27, 2025
@bors
Copy link
Collaborator

bors commented Mar 28, 2025

⌛ Testing commit 57672fb with merge d125f87...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2025
Rollup of 5 pull requests

Successful merges:

 - rust-lang#138104 (Greatly simplify doctest parsing and information extraction)
 - rust-lang#139010 (Improve `xcrun` error handling)
 - rust-lang#139021 (std: get rid of pre-Vista fallback code)
 - rust-lang#139026 (Use `abs_diff` where applicable)
 - rust-lang#139030 (saethlin goes on vacation)

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- [coverage-run] tests/coverage-run-rustdoc/doctest.rs stdout ----
Saved the actual coverage to "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/doctest.coverage"
diff of coverage:

58    LL|       |//!
59    LL|       |//! doctest with custom main:
60    LL|       |//! ```
-    LL|      1|//! fn some_func() {
-    LL|      1|//!     println!("called some_func()");
-    LL|      1|//! }
+    LL|       |//! fn some_func() {
+    LL|       |//!     println!("called some_func()");
+    LL|       |//! }
+    LL|      1|//!
+    LL|      1|//! #[derive(Debug)]
+    LL|      1|//! struct SomeError;
64    LL|       |//!
-    LL|       |//! #[derive(Debug)]
-    LL|       |//! struct SomeError;
-    LL|       |//!
68    LL|       |//! extern crate doctest_crate;
69    LL|       |//!
-    LL|      1|//! fn doctest_main() -> Result<(), SomeError> {
+    LL|       |//! fn doctest_main() -> Result<(), SomeError> {
71    LL|      1|//!     some_func();
72    LL|      1|//!     doctest_crate::fn_run_in_doctests(2);
73    LL|      1|//!     Ok(())

74    LL|      1|//! }
-    LL|       |//!
+    LL|      1|//!
76    LL|       |//! // this `main` is not shown as covered, as it clashes with all the other
77    LL|       |//! // `main` functions that were automatically generated for doctests
78    LL|       |//! fn main() -> Result<(), SomeError> {


The actual coverage differed from the expected coverage.

error: an error occurred comparing coverage output.
status: exit status: 0
command: "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/ci-llvm/bin/llvm-cov" "show" "--format=text" "--show-line-counts-or-regions" "--Xdemangler" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/stage0-tools-bin/coverage-dump" "--Xdemangler" "--demangle" "--instr-profile" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/default.profdata" "--object" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/a" "--object" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/doc_bins/tests_coverage_run_rustdoc_doctest_rs_18_0/rust_out" "--object" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/doc_bins/tests_coverage_run_rustdoc_doctest_rs_24_0/rust_out" "--object" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/doc_bins/tests_coverage_run_rustdoc_doctest_rs_46_0/rust_out" "--object" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/doc_bins/tests_coverage_run_rustdoc_doctest_rs_70_0/rust_out" "--object" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/coverage-run-rustdoc/doctest.coverage-run/doc_bins/tests_coverage_run_rustdoc_doctest_rs_7_0/rust_out"
--- stdout -------------------------------
/Users/runner/work/rust/rust/tests/coverage-run-rustdoc/auxiliary/doctest_crate.rs:
    1|       |/// A function run only from within doctests
    2|      3|pub fn fn_run_in_doctests(conditional: usize) {
    3|      3|    match conditional {
    4|      1|        1 => assert_eq!(1, 1), // this is run,
    5|      1|        2 => assert_eq!(1, 1), // this,
    6|      1|        3 => assert_eq!(1, 1), // and this too
    7|      0|        _ => assert_eq!(1, 2), // however this is not
    8|       |    }
    9|      3|}

/Users/runner/work/rust/rust/tests/coverage-run-rustdoc/doctest.rs:
    1|       |//@ aux-build:doctest_crate.rs
    2|       |
    3|       |//! This test ensures that code from doctests is properly re-mapped.
    4|       |//! See <https://github.com/rust-lang/rust/issues/79417> for more info.
    5|       |//!
    6|       |//! Just some random code:
    7|      1|//! ```
    8|      1|//! if true {
    9|       |//!     // this is executed!
   10|      1|//!     assert_eq!(1, 1);
   11|       |//! } else {
   12|       |//!     // this is not!
   13|      0|//!     assert_eq!(1, 2);
   14|       |//! }
   15|      1|//! ```
   16|       |//!
   17|       |//! doctest testing external code:
   18|       |//! ```
   19|      1|//! extern crate doctest_crate;
   20|      1|//! doctest_crate::fn_run_in_doctests(1);
   21|      1|//! ```
   22|       |//!
   23|       |//! doctest returning a result:
   24|      1|//! ```
   25|       |//! #[derive(Debug, PartialEq)]
   26|       |//! struct SomeError {
   27|       |//!     msg: String,
   28|       |//! }
   29|      1|//! let mut res = Err(SomeError { msg: String::from("a message") });
   30|      1|//! if res.is_ok() {
   31|      0|//!     res?;
   32|       |//! } else {
   33|      1|//!     if *res.as_ref().unwrap_err() == *res.as_ref().unwrap_err() {
   34|      1|//!         println!("{:?}", res);
   35|      1|//!     }
                  ^0
   36|      1|//!     if *res.as_ref().unwrap_err() == *res.as_ref().unwrap_err() {
   37|      1|//!         res = Ok(1);
   38|      1|//!     }
                  ^0
   39|      1|//!     res = Ok(0);
   40|       |//! }
   41|       |//! // need to be explicit because rustdoc cant infer the return type
   42|      1|//! Ok::<(), SomeError>(())
   43|      1|//! ```
   44|       |//!
   45|       |//! doctest with custom main:
   46|       |//! ```
   47|       |//! fn some_func() {
   48|       |//!     println!("called some_func()");
   49|       |//! }
   50|      1|//!
   51|      1|//! #[derive(Debug)]
   52|      1|//! struct SomeError;
   53|       |//!
   54|       |//! extern crate doctest_crate;
   55|       |//!
   56|       |//! fn doctest_main() -> Result<(), SomeError> {
   57|      1|//!     some_func();
   58|      1|//!     doctest_crate::fn_run_in_doctests(2);
   59|      1|//!     Ok(())
   60|      1|//! }
   61|      1|//!
   62|       |//! // this `main` is not shown as covered, as it clashes with all the other
   63|       |//! // `main` functions that were automatically generated for doctests
   64|       |//! fn main() -> Result<(), SomeError> {
   65|       |//!     doctest_main()
   66|       |//! }
   67|       |//! ```
   68|       |
   69|       |/// doctest attached to fn testing external code:
   70|       |/// ```
   71|      1|/// extern crate doctest_crate;
   72|      1|/// doctest_crate::fn_run_in_doctests(3);
   73|      1|/// ```
   74|       |///
   75|      1|fn main() {
   76|      1|    if true {
   77|      1|        assert_eq!(1, 1);
   78|       |    } else {
   79|      0|        assert_eq!(1, 2);
   80|       |    }
   81|      1|}
   82|       |
   83|       |// FIXME(Swatinem): Fix known issue that coverage code region columns need to be offset by the
   84|       |// doc comment line prefix (`///` or `//!`) and any additional indent (before or after the doc
   85|       |// comment characters). This test produces `llvm-cov show` results demonstrating the problem.
   86|       |//
   87|       |// One of the above tests now includes: `derive(Debug, PartialEq)`, producing an `llvm-cov show`
   88|       |// result with a distinct count for `Debug`, denoted by `^1`, but the caret points to the wrong
   89|       |// column. Similarly, the `if` blocks without `else` blocks show `^0`, which should point at, or
   90|       |// one character past, the `if` block's closing brace. In both cases, these are most likely off
   91|       |// by the number of characters stripped from the beginning of each doc comment line: indent
   92|       |// whitespace, if any, doc comment prefix (`//!` in this case) and (I assume) one space character
   93|       |// (?). Note, when viewing `llvm-cov show` results in `--color` mode, the column offset errors are
   94|       |// more pronounced, and show up in more places, with background color used to show some distinct
   95|       |// code regions with different coverage counts.
   96|       |//
   97|       |// NOTE: Since the doc comment line prefix may vary, one possible solution is to replace each
   98|       |// character stripped from the beginning of doc comment lines with a space. This will give coverage
   99|       |// results the correct column offsets, and I think it should compile correctly, but I don't know
  100|       |// what affect it might have on diagnostic messages from the compiler, and whether anyone would care
  101|       |// if the indentation changed. I don't know if there is a more viable solution.
------------------------------------------
stderr: none



@bors
Copy link
Collaborator

bors commented Mar 28, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 28, 2025
@jhpratt jhpratt closed this Mar 28, 2025
@jhpratt jhpratt deleted the rollup-b7ofkqw branch March 28, 2025 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup A PR which is a rollup S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants