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.

Sign up for GitHub

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

Jump to bottom

Partial support for running UI tests with download-rustc #103969

Merged
merged 2 commits into from
Nov 19, 2022
Merged

Partial support for running UI tests with download-rustc #103969

merged 2 commits into from
Nov 19, 2022

Conversation

pietroalbini
Copy link
Member

Right now trying to run UI tests with download-rustc results in a bunch of test failures, requiring someone who wants to only work on tests to also build the full compiler. This PR partially addresses the problem by solving a lot of the errors (but not all).

  • This installs the rust-src component on CI toolchains, since the test output depends on whether the standard library source code is installed; We can't just symlink the current source because the rustc-dev component already includes the compiler sources, but not the library sources, and mixing things is worse IMO.
  • This ensures the $SRC_DIR normalization done by compiletest correctly handles the source code added above.
  • This unconditionally sets remap-prefix to the prefix used in the downloaded toolchain, to ensure compiletest normalizes it.

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2022

r? @jyn514

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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. labels Nov 4, 2022
jyn514
jyn514 reviewed Nov 4, 2022
View reviewed changes
src/bootstrap/config.rs Outdated Show resolved Hide resolved
src/bootstrap/lib.rs Outdated
@@ -1034,13 +1034,17 @@ impl Build {
}

fn debuginfo_map_to(&self, which: GitRepo) -> Option<String> {
if !self.config.rust_remap_debuginfo {
if !self.config.rust_remap_debuginfo && self.config.download_rustc_commit.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

why would this matter? if we're not remapping debuginfo, we shouldn't need this either way, right?

Copy link
Member Author

@pietroalbini pietroalbini Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Compiletest only replaces the /rustc/$sha1 paths with a placeholder when the CFG_VIRTUAL_RUST_SOURCE_BASE_DIR environment variable is passed to it at build time, and right now this is the way to set that.

I guess I could change the implementation and instead of changing this setting globally, pass to compiletest another prefix to normalize when download-rustc is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

ah I see, it needs to remap the source directory because the one we've downloaded from CI is not in the path the tests expect. That seems reasonable, can you add it as a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I could change the implementation and instead of changing this setting globally, pass to compiletest another prefix to normalize when download-rustc is enabled.

Ended up with this approach, as there is way less risk of unintended side effects this way.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment was marked as outdated.

Sign in to view
@bors
Copy link
Contributor

bors commented Nov 14, 2022

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

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2022
@jyn514
Copy link
Member

jyn514 commented Nov 17, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 17, 2022

📌 Commit 470423c has been approved by jyn514

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 17, 2022

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2022
bjorn3
bjorn3 reviewed Nov 18, 2022
View reviewed changes
src/tools/compiletest/src/runtest.rs
// Source base on the sysroot (from the src components downloaded by `download-rustc`):
Some(self.config.sysroot_base.join("lib").join("rustlib").join("src").join("rust")),
// Virtual `/rustc/$sha` remapped paths (if `remap-debuginfo` is enabled):
option_env!("CFG_VIRTUAL_RUST_SOURCE_BASE_DIR").map(PathBuf::from),
Copy link
Member

@bjorn3 bjorn3 Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nice! Could use this in cg_clif too: https://github.com/bjorn3/rustc_codegen_cranelift/blob/d1e499aa2d5e16eab6b70c04b28bd04b67de3c60/scripts/setup_rust_fork.sh#L30-L47

Edit: done in bjorn3/rustc_codegen_cranelift@9723c79

@matthiaskrgr matthiaskrgr mentioned this pull request Nov 19, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2022
@bors
Auto merge of rust-lang#104607 - matthiaskrgr:rollup-9s589me, r=matth…
2a43428 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#103117 (Use `IsTerminal` in place of `atty`)
 - rust-lang#103969 (Partial support for running UI tests with `download-rustc`)
 - rust-lang#103989 (Fix build of std for thumbv7a-pc-windows-msvc)
 - rust-lang#104076 (fix sysroot issue which appears for ci downloaded rustc)
 - rust-lang#104469 (Make "long type" printing type aware and trim types in E0275)
 - rust-lang#104497 (detect () to avoid redundant <> suggestion for type)
 - rust-lang#104577 (Don't focus on notable trait parent when hiding it)
 - rust-lang#104587 (Update cargo)
 - rust-lang#104593 (Improve spans for RPITIT object-safety errors)
 - rust-lang#104604 (Migrate top buttons style to CSS variables)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4451e28 into rust-lang:master Nov 19, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 19, 2022
@pietroalbini pietroalbini deleted the pa-download-rustc-ui-tests branch November 21, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
6 participants
@pietroalbini @rustbot @rust-log-analyzer @bors @jyn514 @bjorn3