cargo
cargo copied to clipboard
rust-lang
Test if reserved filenames are allowed in Windows
Recent versions of Windows have removed the limitation on filenames like aux or con. This change allows the package::reserved_windows_name to still pass by first trying to create a file with a reserved name to see if Windows supports it. If so, it skips the rest of the test. Otherwise, we keep the same behavior as before.
r? @ehuss
(rust-highfive has picked a reviewer for you, use r? to override)
Hi. If I want to verify the behavior of this patch. What environment should I prepare? Specifically, can you provide some official doc about this change? Thanks!
I'm on the Windows 11 Insiders Beta channel, and winver says it's version 21H2 (OS Build 22000.466).
Unfortunately, I haven't been able to find official documentation about the change, but I did confirm the behavior with the Windows dev who made the change.
Could you post a transcript of the test failure (including with nocapture so we can see the actual test output)?
Sure, here's the output from cargo test -- package::reserved_windows_name --nocapture on master:
Finished test [unoptimized + debuginfo] target(s) in 0.27s
Running unittests src/cargo/lib.rs (target\debug\deps\cargo-9040197b200da57a.exe)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 34 filtered out; finished in 0.00s
Running tests\build-std\main.rs (target\debug\deps\build_std-9cc2dc147da3e21f.exe)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out; finished in 0.00s
Running tests\internal.rs (target\debug\deps\internal-870b738d2af5cbc1.exe)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.00s
Running tests\testsuite\main.rs (target\debug\deps\testsuite-508cfd0591301aff.exe)
running 1 test
running `C:\Users\ericholk\repo\cargo\target\debug\cargo.exe package`
thread 'package::reserved_windows_name' panicked at '
test failed running `C:\Users\ericholk\repo\cargo\target\debug\cargo.exe package`
error: process exited with code 0 (expected 101)
--- stdout
--- stderr
warning: manifest has no documentation, homepage or repository.
See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info.
Packaging foo v0.0.1 (C:\Users\ericholk\repo\cargo\target\tmp\cit\t0\foo)
Updating `dummy-registry` index
Verifying foo v0.0.1 (C:\Users\ericholk\repo\cargo\target\tmp\cit\t0\foo)
Downloading crates ...
Downloaded bar v1.0.0 (registry `dummy-registry`)
Compiling bar v1.0.0
Compiling foo v0.0.1 (C:\Users\ericholk\repo\cargo\target\tmp\cit\t0\foo\target\package\foo-0.0.1)
Finished dev [unoptimized + debuginfo] target(s) in 0.56s
', tests\testsuite\package.rs:1939:10
stack backtrace:
0: std::panicking::begin_panic_handler
at /rustc/f0c4da49983aa699f715caf681e3154b445fb60b\/library\std\src\panicking.rs:584
1: core::panicking::panic_fmt
at /rustc/f0c4da49983aa699f715caf681e3154b445fb60b\/library\core\src\panicking.rs:143
2: cargo_test_support::panic_error::pe
at .\crates\cargo-test-support\src\lib.rs:48
3: cargo_test_support::panic_error<anyhow::Error>
at .\crates\cargo-test-support\src\lib.rs:40
4: cargo_test_support::Execs::run
at .\crates\cargo-test-support\src\lib.rs:802
5: testsuite::package::reserved_windows_name
at .\tests\testsuite\package.rs:1915
6: testsuite::package::reserved_windows_name::closure$0
at .\tests\testsuite\package.rs:1890
7: core::ops::function::FnOnce::call_once<testsuite::package::reserved_windows_name::closure_env$0,tuple$<> >
at /rustc/f0c4da49983aa699f715caf681e3154b445fb60b\library\core\src\ops\function.rs:227
8: core::ops::function::FnOnce::call_once
at /rustc/f0c4da49983aa699f715caf681e3154b445fb60b\library\core\src\ops\function.rs:227
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
test package::reserved_windows_name ... FAILED
failures:
failures:
package::reserved_windows_name
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2345 filtered out; finished in 1.02s
error: test failed, to rerun pass '--test testsuite'
So, reading that test case, I don't think that's a test that should pass in general, even if the platform doesn't actually reserve such filenames. The logic that checks for aux.rs and avoids unpacking the file should not depend on what Windows does, at least not until we stop supporting all versions of Windows with that limitation.
I think, rather than changing the test so its behavior depends on the OS, this should instead change the implementation of the code that this test tests to ensure that it catches reserved filenames on all versions of Windows.
That seems reasonable. If I remember right, the code for publishing packages tries to prevent you from publishing packages that couldn't be unpacked on all OSes supported by Cargo, so it makes sense to have similar behavior for unpacking packages.
In that case though, I think we should also remove the #[cfg(windows)] on this test.
If I remember right, the code for publishing packages tries to prevent you from publishing packages that couldn't be unpacked on all OSes supported by Cargo, so it makes sense to have similar behavior for unpacking packages.
Exactly.
In that case though, I think we should also remove the
#[cfg(windows)]on this test.
Agreed.
I decided to close this PR in favor of #10461 which cleanly does what was decided in the discussion here.
Reopening this since there's disagreement about whether we want this change or the one in #10461. We only need one of these to land though.
Per discussion on Zulip, this approach seems fine to me, as long as it doesn't actually use the same code in both this check and the code being tested (which would be circular).
In that spirit, and in the spirit of not providing this as a public API of Cargo, could you move this function into the test code rather than the util module?
In that spirit, and in the spirit of not providing this as a public API of Cargo, could you move this function into the test code rather than the util module?
Yup, that's done now! I moved it to the cargo_test_support crate, which required adding winapi as a dependency there too. Hopefully that's okay!
Seems reasonable to me. Does anyone see an issue with adding that dependency to the test support crate?
:pushpin: Commit 8de9adfb0db77bb5fd0742fd59904f810f0e2cbb has been approved by ehuss
It is now in the queue for this repository.
:hourglass: Testing commit 8de9adfb0db77bb5fd0742fd59904f810f0e2cbb with merge 7259757dc618479363aca6176ee5e76fd534e68c...
:sunny: Test successful - checks-actions Approved by: ehuss Pushing 7259757dc618479363aca6176ee5e76fd534e68c to master...