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

Add support for shell argfiles #118680

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Conversation

djkoloski
Copy link
Contributor

@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2023

r? @compiler-errors

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 6, 2023
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2023

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@djkoloski
Copy link
Contributor Author

Tidy is mad about tests/ui containing too many tests, but I put my tests in a subdirectory. This PR is ready for review.

@compiler-errors
Copy link
Member

Tidy is mad about tests/ui containing too many tests, but I put my tests in a subdirectory.

You still need to fix tidy. Change the limits in src/tools/tidy/src/ui_tests.rs.

@rust-log-analyzer

This comment has been minimized.

@djkoloski djkoloski force-pushed the shell_argfiles branch 2 times, most recently from d68ac50 to 3bf1ad5 Compare December 6, 2023 23:47
@djkoloski
Copy link
Contributor Author

Thanks @compiler-errors, should be good to go now

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Another option would be to remove the unstable flag and gate this behind -Zunstable-options instead. I'm not sure what's better.

compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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 Dec 17, 2023
@rust-log-analyzer

This comment has been minimized.

@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 Jan 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#118680 (Add support for shell argfiles)
 - rust-lang#119151 (Hide foreign `#[doc(hidden)]` paths in import suggestions)
 - rust-lang#119350 (Imply outlives-bounds on lazy type aliases)
 - rust-lang#119354 (Make `negative_bounds` internal & fix some of its issues)
 - rust-lang#119506 (Use `resolutions(()).effective_visiblities` to avoid cycle errors in `report_object_error`)
 - rust-lang#119554 (Fix scoping for let chains in match guards)
 - rust-lang#119563 (Check yield terminator's resume type in borrowck)
 - rust-lang#119589 (cstore: Remove unnecessary locking from `CrateMetadata`)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r-
looks like this failed in #119630 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 5, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Jan 5, 2024

- error: Failed to load argument file: Invalid shell-style arguments in $DIR/shell-argfiles-badquotes.args
+ error: Failed to load argument file: Invalid shell-style arguments in C:\a\rust\rust\tests\ui/shell-argfiles/shell-argfiles-badquotes.args

@djkoloski
Copy link
Contributor Author

Hmm looks like something funny is going on on windows. I'm attempting a repro.

@djkoloski
Copy link
Contributor Author

It looks like the underlying issue is that the shell-argfiles-badquotes.rs test is passing @shell:{{src-base}}/shell-argfiles/shell-argfiles-badquotes.args as an argument. On Windows, this path resolves to C:\some\path\with\backslashes\ui/shell-argfiles/shell-argfiles-badquotes.args. This path gets emitted verbatim in the error message.

In runtest.rs, the path normalization method replaces self.testpaths.file.parent() with $DIR in the output. This path will be target-specific, and on Windows it will be C:\some\path\with\backslashes\ui\shell-argfiles. Because it's a simple string replace, the final backslash after ui causes it not to match, and so is compared verbatim with the .stderr file.

A quick search through the other tests in tests doesn't turn up any other tests which 1. are located in a subdirectory of ui, 2. pass a file via the command line, and 3. are intended to fail with a .stderr file to compare against. So this is the only test affected.

I'll see if I can canonicalize the path before emitting it in the shell argfiles diagnostic. If there's no easy way to do that, I'll take a crack at changing the behavior of $DIR replacement in runtest.rs.

@djkoloski
Copy link
Contributor Author

djkoloski commented Jan 5, 2024

@rustbot ready

I have verified that this change fixes the failing windows test.

@rustbot rustbot 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 Jan 5, 2024
@djkoloski
Copy link
Contributor Author

An alternative to adding {{sep}} would be to move these tests up out of a subdirectory. That would avoid the underlying issue.

@djkoloski
Copy link
Contributor Author

@rustbot ready

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

This could have used revisions:

// revisions: windows nonwindows
//[windows] only-windows
//[windows] compile-flags: \back\slashes
//[nonwindows] ignore-windows
//[nonwindows] compile-flags: /fwd/slashes

@compiler-errors
Copy link
Member

but whatever

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2024

📌 Commit 684aa2c has been approved by compiler-errors

It is now in the queue for this repository.

@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 Jan 9, 2024
@djkoloski
Copy link
Contributor Author

I'd be happy to file a follow-up PR to clean the test up! And thanks for bearing with me while I continue to learn the ropes. ❤️

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#118241 (Making `User<T>` and `User<[T]>` `Send`)
 - rust-lang#118645 (chore: Bump compiler_builtins)
 - rust-lang#118680 (Add support for shell argfiles)
 - rust-lang#119721 (`~const` trait and projection bounds do not imply their non-const counterparts)
 - rust-lang#119768 (core: panic: fix broken link)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3da96ae into rust-lang:master Jan 9, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
Rollup merge of rust-lang#118680 - djkoloski:shell_argfiles, r=compiler-errors

Add support for shell argfiles

Closes rust-lang/compiler-team#684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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) 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.

Support response files generated by ninja with @ninja:path syntax
7 participants