search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.63k stars 2.4k forks source link

cargo clean fails on projects using cxx for rust <> c++ bindings #13752

Closed moizzd closed 4 months ago

moizzd commented 5 months ago

Problem

When using the cxx crate to generate rust <> C++ bindings, cargo clean fails. The error is due to failure to delete directories generated by the cxx tool chain under the target directory.

Steps

  1. Clone https://github.com/dtolnay/cxx
  2. cd to the demo directory
  3. Run cargo build
  4. Run cargo clean
  5. It will fail with the following error: error: failed to remove file <homedir>\projects\other\cxx\target\debug\build\cxx-test-suite-2dda1da2c847afa7\out\cxxbridge\crate\tests\ffi

Possible Solution(s)

It seems that it is failing to remove a directory that is not empty and/or has sub-directories.

Notes

No response

Version

cargo version --verbose
cargo 1.77.1 (e52e36006 2024-03-26)
release: 1.77.1
commit-hash: e52e360061cacbbeac79f7f1215a7a90b6f08442
commit-date: 2024-03-26
host: x86_64-pc-windows-msvc
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.5.0-DEV (sys:0.4.70+curl-8.5.0 vendored ssl:Schannel)
os: Windows 10.0.19045 (Windows 10 Pro) [64-bit]
weihanglo commented 5 months ago

Thanks for the report!

<homedir>\projects\other\cxx\target\debug\build\cxx-test-suite-2dda1da2c847afa7\out\cxxbridge\crate\tests\ffi is a symlink pointing to a directory. According to Win32 API doc, symlinks created by CreateSymbolicLinkA should be removed by RemoveDirectory not DeleteFile.

The fix seems to be straightforward: here whenever we delete a file, on Windows we additionally check if it is a symlink to a directory. If yes then call remove_dir instead on the symlink.

weihanglo commented 5 months ago

When submitting a pull request, the first commit should be a test demonstrating the current “bad behavior”, followed by other commits fixing the bug and tests, so that we know we're fixing an issue that really exists.

Rustin170506 commented 5 months ago

@rustbot claim

I will take a look.

Rustin170506 commented 4 months ago

Get back from my holiday and start working on this now!

Rustin170506 commented 4 months ago

@moizzd https://github.com/rust-lang/cargo/pull/13910 Could you please help test this patch? I had some issues when I tried to test on my virtual machine.

  1. You need to clone my repo or clone the Cargo repo and apply my patch.
  2. Build Cargo with my patch.
  3. Try `some-path/cargo/target/debug/cargo clean
moizzd commented 4 months ago

@hi-rustin I will try it later today.

Thanks

From: "二手掉包工程师" @.> Sent: 5/14/24 8:46 AM To: rust-lang/cargo @.> Cc: Moiz Dohadwala @.>, Mention @.> Subject: Re: [rust-lang/cargo] cargo clean fails on projects using cxx for rust <> c++ bindings (Issue #13752)

@moizzd #13910 Could you please help test this patch? I had some issues when I tried to test on my virtual machine.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

Rustin170506 commented 4 months ago

@moizzd Have you tested it? Did it work?

moizzd commented 4 months ago

@hi-rustin No, I haven't had a chance unfortunately. Work related urgency intervened. I will do it today.

moizzd commented 4 months ago

@hi-rustin

I built the cargo repo with your patch. I then ran the build on the cxx demo directory and then ran clean:


Z:\Projects\other\demo>..\cargo\target\debug\cargo build
   Compiling windows_x86_64_msvc v0.52.5
   Compiling proc-macro2 v1.0.83
   Compiling cc v1.0.98
   Compiling unicode-ident v1.0.12
   Compiling scratch v1.0.7
   Compiling windows-targets v0.52.5
   Compiling windows-sys v0.52.0
   Compiling cxxbridge-flags v1.0.122
   Compiling quote v1.0.36
   Compiling winapi-util v0.1.8
   Compiling syn v2.0.65
   Compiling termcolor v1.4.1
   Compiling unicode-width v0.1.12
   Compiling link-cplusplus v1.0.9
   Compiling codespan-reporting v0.11.1
   Compiling cxx v1.0.122
   Compiling once_cell v1.19.0
   Compiling cxx-build v1.0.122
   Compiling cxxbridge-macro v1.0.122
   Compiling demo v0.0.0 (Z:\Projects\other\demo)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 19.34s

Z:\Projects\other\demo>..\cargo\target\debug\cargo clean
     Removed 319 files, 106.3MiB total

Z:\Projects\other\demo>dir
 Volume in drive Z is Shared Folders
 Volume Serial Number is 0000-0064

 Directory of Z:\Projects\other\demo

05/22/2024  09:35 PM    <DIR>          .
05/22/2024  09:35 PM    <DIR>          ..
03/26/2024  09:10 PM               686 BUCK
03/26/2024  06:36 PM               726 BUILD
03/26/2024  09:10 PM               323 build.rs
05/22/2024  09:33 PM             6,119 Cargo.lock
03/26/2024  06:36 PM               345 Cargo.toml
05/22/2024  09:08 PM    <DIR>          include
05/22/2024  09:08 PM    <DIR>          src
               5 File(s)          8,199 bytes
               4 Dir(s)  339,151,474,688 bytes free

``
So, it seems to have worked.
moizzd commented 4 months ago

with the unpatched ( installed cargo):

Z:\projects\other\demo>cargo clean error: failed to remove file C:\Users\<user>\projects\other\demo\target\debug\build\demo-5950ef69aeb759a3\out\cxxbridge\crate\demo

Caused by: Access is denied. (os error 5)

izolyomi commented 3 months ago

Thank you for the efforts. Is this fix already included in the upcoming version 1.79? I can't see it mentioned in the changelists anywhere.

ChrisDenton commented 3 months ago

The PR that fixes this (https://github.com/rust-lang/cargo/pull/13910) is tagged with the 1.80.0 milestone.