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

Enable two windows tests #10930

Merged
merged 2 commits into from
Aug 11, 2022
Merged

Enable two windows tests #10930

merged 2 commits into from
Aug 11, 2022

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Aug 2, 2022

These two tests were disabled on Windows a long time ago. AFAICT, the reasons are no longer relevant, and it should be safe to enable these tests. See each commit for a more detailed exposition.

AFAICT, we do not test on these older platforms anymore.
Regardless, the test seems to work fine on 32-bit windows-gnu
on Windows 10.

See rust-lang#3102 (comment)
where it was originally disabled.
This test was ignored in rust-lang#3189
without much discussion of explaining why.

AFAICT, this test works fine on Windows on both MSVC and GNU.
Empty paths do the expected behavior (preventing cargo from running
rustc). There are some special rules on Windows about discovering the
process to run (such as searching the app's launch directory), but
I do not think that is relevant here. Confirmed by trying to run
`cargo check` in this test fails to find `rustc`.
@rust-highfive
Copy link

@ehuss: no appropriate reviewer found, use r? to override

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2022
@ChrisDenton
Copy link
Member

ChrisDenton commented Aug 2, 2022

I think #3189 was referring to how using Command::new("rustc").env("PATH", "") will work on Windows but not on Linux (assuming "rustc" is in the current process' path). But that's not really relevant to the test as far as I can tell.

@joshtriplett
Copy link
Member

joshtriplett commented Aug 8, 2022

LGTM.

@weihanglo
Copy link
Member

LGTM as well. If someone faces any issue, we can consider re-ignore it later.

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 11, 2022

📌 Commit c0be32b has been approved by weihanglo

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 Aug 11, 2022
@bors
Copy link
Collaborator

bors commented Aug 11, 2022

⌛ Testing commit c0be32b with merge 34cba46...

@bors
Copy link
Collaborator

bors commented Aug 11, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 34cba46 to master...

@bors bors merged commit 34cba46 into rust-lang:master Aug 11, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2022
Update cargo

8 commits in ce40690a5e4e315d3dab0aae1eae69d0252c52ac..efd4ca3dc0b89929dc8c5f5c023d25978d76cb61
2022-08-09 22:32:17 +0000 to 2022-08-12 01:28:28 +0000
- Use `std::thread::scope` to replace crossbeam (rust-lang/cargo#10977)
- [docs] Remove extra "in" from `cargo-test.md` (rust-lang/cargo#10978)
- Enable two windows tests (rust-lang/cargo#10930)
- Improve error msg for get target runner (rust-lang/cargo#10968)
- Ensure rustc-echo-wrapper works with an overridden build.target-dir (rust-lang/cargo#10962)
- Switch back to `available_parallelism` (rust-lang/cargo#10969)
- Only override published resolver when the workspace is different (rust-lang/cargo#10961)
- Add `CARGO_LOG` to "Environment variables Cargo reads" (rust-lang/cargo#10967)
@ehuss ehuss added this to the 1.65.0 milestone Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants