brson / wasm-opt-rs

Rust bindings for Binaryen's wasm-opt
Apache License 2.0
61 stars 10 forks source link

`cargo clean` can't delete out\cxxbridge\crate\wasm-opt-cxx-sys on Windows #116

Open nazar-pc opened 1 year ago

nazar-pc commented 1 year ago

I have hit following error in CI: https://github.com/subspace/subspace/actions/runs/3434067151/jobs/5725766740

  error: failed to remove build artifact

  Caused by:
    failed to remove file `D:\a\subspace\subspace\target\debug\wbuild\substrate-test-runtime\target\release\build\wasm-opt-cxx-sys-fae30863e99b4faf\out\cxxbridge\crate\wasm-opt-cxx-sys`

  Caused by:
    Access is denied. (os error 5)
Error: The process 'C:\Users\runneradmin\.cargo\bin\cargo.exe' failed with exit code 101

All that code does is cargo clean. On Linux I see that that location is a symbolic link, still it doesn't make much sense to me why it can't be removed. I need that to essentially work around https://github.com/brson/wasm-opt-rs/issues/115

brson commented 1 year ago

I have reproduced this on my own CI on windows 2019: https://github.com/brson/wasm-opt-rs/actions/runs/3447794546/jobs/5754178052

Simply running cargo clean immediately after the build fails with access denied.

I originally suspected this was an interaction between windows file system semantics and the specific structure of the substrate build in the linked PR, but that does not seem to be the case.

Edit: this was also with the old Rust 1.48 toolchain, not a recent toolchain.

brson commented 1 year ago

I could not reproduce this locally on windows 10 running cargo clean by hand. Perhaps there is a timing factor here.

brson commented 1 year ago

I've also verified this happens on CI windows 2019 with Rust stable: https://github.com/brson/wasm-opt-rs/actions/runs/3447938335/jobs/5754469499

So it's not related to an old cargo.

Still can't reproduce locally.

brson commented 1 year ago

cc @dtolnay I can't investigate much deeper yet, but maybe the issue will be obvious to you.

After running the build on GitHub actions, on either Windows 2019 or Windows 2022, cargo clean fails to delete out\cxxbridge\crate\wasm-opt-cxx-sys.

I don't see the same behavior locally on Windows 10.

This is a symlink so maybe there's something special about the GitHub windows environment's handling of symlinks.

nazar-pc commented 1 year ago

Tried deleting with https://docs.rs/remove_dir_all/0.7.0/remove_dir_all/fn.remove_dir_all.html (after reading https://github.com/rust-lang/rust/issues/29497), but still hit the same issue :confused:

ChrisDenton commented 1 year ago

If it's just a symlink, would you be able to try a plain std::fs::remove_file (or std::fs::remove_dir) directly on the offending path? This would help isolate if it's an issue specifically with remove_dir_all (which is a bit thorny) or a more general problem.

I'd also note that CI does have special file scanners etc so it's possible they're preventing delete in some way. If that's the case retrying (after a delay) may work.

brson commented 1 year ago

When I tested this I did try throwing some sleeps into the script, like sleep 2 || cargo clean || sleep 2 || cargo clean, etc. and it still failed. Still might be worth someone else trying though.

Edit: Er the way I wrote this with ||s can't be correct. Hopefully I wrote it correctly when I tested it.

This is actually what I tested:

          sleep 2
          cargo clean || cargo clean || cargo clean || cargo clean
nazar-pc commented 1 year ago

I used a third-party remove_dir_all and had the same issue, also tried deleting in a loop with timeout for attempts, it always fails the same way.

ChrisDenton commented 1 year ago

I think this is probably an issue with cargo. I created a wee app that simply calls std::fs::remove_dir_all on a path. Using this to delete the target directory worked without error. See https://github.com/ChrisDenton/wasm-opt-rs/pull/1/files

brson commented 1 year ago

@ChrisDenton thank you very much for the investigation. That is a fascinating result.

leighmcculloch commented 1 year ago

As another input/example, this same issue just started happening in the builds of soroban-cli, with Rust 1.70 (stable-x86_64-pc-windows-msvc unchanged - rustc 1.70.0).

info: running `cargo --config source.crates-io.replace-with = 'vendored-sources' --config source.vendored-sources.directory = 'vendor' package --target x86_64-pc-windows-msvc --no-default-features --features default,opt` on soroban-cli (4/5)
   Packaging soroban-cli v0.8.5 (D:\a\soroban-tools\soroban-tools\cmd\soroban-cli)
   Verifying soroban-cli v0.8.5 (D:\a\soroban-tools\soroban-tools\cmd\soroban-cli)
error: failed to verify package tarball

Caused by:
  failed to remove file `D:\a\soroban-tools\soroban-tools\target\package\soroban-cli-0.8.5\target\x86_64-pc-windows-msvc\debug\build\wasm-opt-cxx-sys-7458c11355075ff3\out\cxxbridge\crate\wasm-opt-cxx-sys`

Caused by:
  Access is denied. (os error 5)
error: process didn't exit successfully: `cargo --config source.crates-io.replace-with = 'vendored-sources' --config source.vendored-sources.directory = 'vendor' package --target x86_64-pc-windows-msvc --manifest-path cmd\soroban-cli\Cargo.toml --no-default-features --features default,opt` (exit code: 101)

Ref: https://github.com/stellar/soroban-tools/actions/runs/5275255477/jobs/9540530625?pr=699 From: https://github.com/stellar/soroban-tools/pull/699

leighmcculloch commented 1 year ago

My apologies. I thought this had been already fixed in cargo, and was maybe recurring, but I see it was merged only 2 weeks ago, so not in 1.70.0. My output above adds nothing to the data above already.

trevyn commented 1 year ago

@leighmcculloch Is your example reliably reproducible? Does it happen with nightly cargo?

izolyomi commented 2 months ago

We've just encountered the same behaviour of the cxxbridge directory on multiple machines using Windows 10 stable-x86_64-pc-windows-msvc. This seems to be related to a cargo clean implementation error specific to directory symlinks on Windows: https://github.com/rust-lang/cargo/issues/13752

nazar-pc commented 2 months ago

So looks like it was fixed upstream?

izolyomi commented 2 months ago

The fix seems to be coming in version 1.80.