Test if reserved filenames are allowed in Windows

[eholk]

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.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[weihanglo]

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!

[eholk]

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.

[joshtriplett]

Could you post a transcript of the test failure (including with nocapture so we can see the actual test output)?

[eholk]

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'

[joshtriplett]

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.

[eholk]

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.

[joshtriplett]

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.

[eholk]

I decided to close this PR in favor of #10461 which cleanly does what was decided in the discussion here.

[eholk]

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.

[joshtriplett]

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?

[eholk]

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!

[joshtriplett]

Seems reasonable to me. Does anyone see an issue with adding that dependency to the test support crate?

[ehuss]

I'm sorry for the super long delay. I didn't have a Windows 11 system handy. I would be happy to get this merged with a small change.

[ehuss]

Thanks!

@bors r+

[bors]

📌 Commit 8de9adf has been approved by ehuss

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8de9adf with merge 7259757...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 7259757 to master...