Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform deeper compiletest path normalization for $TEST_BUILD_DIR to account for compare-mode/debugger cases, and normalize long type file filename hashes #136865

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Feb 11, 2025

Caution

This PR is stacked on top of #136474 and #136542, and may not be merged before they do, and would require a rebase.

Fixes #136510.

Summary

  • Whereas previously $TEST_BUILD_DIR is a normalization of /path/to/build/test/<test_suite_name>/, we now more deeply normalize. $TEST_BUILD_DIR now becomes a normalization of /path/to/build/test/<test_suite_name>/<subdirs>/$name.$revision.$compare_mode.$debugger/ to normalize away path name differences when --compare-mode and/or --debugger are specified.
  • We also centralize the normalization of long type name hashes

cf. #136328 (comment).

Review advice

  • Best reviewed commit-by-commit.
  • Split into 3 commits:
    • Commit 1: compiletest changes to have $TEST_BUILD_DIR more deeply normalize.
    • Commit 2: remove per-test hacks for long type path hash normalizations, and rebless tests specifically affected by that.
    • Commit 3: rebless other tests that were changed as a side-effect of deeper $TEST_BUILD_DIR normalizations.

Commit 2 is created via first finding tests that try to perform long type file hash normalizations on an ad hoc, per-test basis:

rg --no-ignore -l --no-ignore -F -e "long-type" tests/ui/**/*.rs   
Tests with ad hoc long-type hash normalizations
tests/ui/type_length_limit.rs
tests/ui/traits/on_unimplemented_long_types.rs
tests/ui/regions/issue-102374.rs
tests/ui/recursion/recursion.rs
tests/ui/recursion/issue-83150.rs
tests/ui/recursion/issue-23122-2.rs
tests/ui/methods/inherent-bound-in-probe.rs
tests/ui/issues/issue-67552.rs
tests/ui/issues/issue-37311-type-length-limit/issue-37311.rs
tests/ui/issues/issue-20413.rs
tests/ui/issues/issue-8727.rs
tests/ui/infinite/infinite-instantiation.rs
tests/ui/infinite/infinite-instantiation-struct-tail-ice-114484.rs
tests/ui/higher-ranked/trait-bounds/hrtb-doesnt-borrow-self-1.rs
tests/ui/higher-ranked/trait-bounds/hrtb-doesnt-borrow-self-2.rs
tests/ui/higher-ranked/trait-bounds/hang-on-deeply-nested-dyn.rs
tests/ui/error-codes/E0275.rs
tests/ui/diagnostic-width/secondary-label-with-long-type.rs
tests/ui/diagnostic-width/long-e0277.rs
tests/ui/diagnostic-width/non-copy-type-moved.rs
tests/ui/diagnostic-width/long-E0308.rs
tests/ui/diagnostic-width/E0271.rs
tests/ui/diagnostic-width/binop.rs

These ad hoc normalizations were removed, and they are reblessed.

r? @lqd

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 11, 2025
@jieyouxu
Copy link
Member Author

@rustbot blocked (#136474, #136542)

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2025
@jieyouxu jieyouxu added A-compiletest-normalizations Area: compiletest normalizations A-compiletest-compare-modes Area: compiletest compare-modes labels Feb 11, 2025
//~ ERROR overflow evaluating the requirement `Map<&mut std::ops::Range<u8>, {closure@$DIR/issue-83150.rs:13:24: 13:27}>: Iterator`
//~ ERROR overflow evaluating the requirement `Map<&mut std::ops::Range<u8>, {closure@$DIR/issue-83150.rs:12:24: 12:27}>: Iterator`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closure LINE:COL numbers aren't normalized, not for this PR

Comment on lines -7 to +9
= note: candidate #1: $TEST_BUILD_DIR/crate-loading/crateresolve1/auxiliary/libcrateresolve1-1.somelib
= note: candidate #2: $TEST_BUILD_DIR/crate-loading/crateresolve1/auxiliary/libcrateresolve1-2.somelib
= note: candidate #3: $TEST_BUILD_DIR/crate-loading/crateresolve1/auxiliary/libcrateresolve1-3.somelib
= note: candidate #1: $TEST_BUILD_DIR/auxiliary/libcrateresolve1-1.somelib
= note: candidate #2: $TEST_BUILD_DIR/auxiliary/libcrateresolve1-2.somelib
= note: candidate #3: $TEST_BUILD_DIR/auxiliary/libcrateresolve1-3.somelib
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're now normalizing deeper, so this is to be expected, otherwise you'd have crateresolve1.polonius for instance

Copy link
Member

@lqd lqd Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that seems fine to me as well, it should be quite rare that we’re testing and looking for an actual path in diagnostics rather than the semantic contents of the output.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member Author

Of course, the ui-fulldeps test suite exists.

@bors
Copy link
Contributor

bors commented Feb 14, 2025

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

Instead of only having `--src-base` and `src_base` which *actually*
refers to the directory containing the test suite and not the sources
root. More importantly, kill off `find_rust_src_root` when we can simply
pass that info from bootstrap.
…ic build dir

- Introduce and use `--build-{root,test-suite-root}` over
  `--build-base`.
- A few minor cleanups.
This is to make test stderr insensitive to compare-mode / debugger that
changes the test build dir output name.

Previously, this normalized paths up to test-suite-specific build root,
e.g. `/path/to/build/test/ui/`. Now, this normalizes up to test-specific
build root, e.g.
`/path/to/build/test/ui/subdir/$name.$revision.$mode.$debugger/`.
@jieyouxu jieyouxu force-pushed the long-type-path-compare-mode branch from 3eedbe2 to 1ed7430 Compare February 14, 2025 10:53
Copy link
Member Author

@jieyouxu jieyouxu Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This merge-conflicted against master, not sure what changed here 🤔 Maybe just line number differences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-compiletest-compare-modes Area: compiletest compare-modes A-compiletest-normalizations Area: compiletest normalizations A-testsuite Area: The testsuite used to check the correctness of rustc S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compiletest: compare-mode false negatives w.r.t. long type file path
5 participants