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

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

Closed
moizzd opened this issue Apr 13, 2024 · 10 comments · Fixed by #13910
Closed

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

moizzd opened this issue Apr 13, 2024 · 10 comments · Fixed by #13910
Assignees
Labels
C-bug Category: bug Command-clean E-easy Experience: Easy O-windows OS: Windows S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@moizzd
Copy link

moizzd commented Apr 13, 2024

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]
@moizzd moizzd added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Apr 13, 2024
@weihanglo
Copy link
Member

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 weihanglo added E-easy Experience: Easy O-windows OS: Windows S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-triage Status: This issue is waiting on initial triage. labels Apr 13, 2024
@weihanglo
Copy link
Member

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.

@hi-rustin
Copy link
Member

@rustbot claim

I will take a look.

@hi-rustin
Copy link
Member

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

@hi-rustin
Copy link
Member

@moizzd #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
Copy link
Author

moizzd commented May 14, 2024 via email

@hi-rustin
Copy link
Member

@moizzd Have you tested it? Did it work?

@moizzd
Copy link
Author

moizzd commented May 20, 2024

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

@moizzd
Copy link
Author

moizzd commented May 23, 2024

@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
Copy link
Author

moizzd commented May 23, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-clean E-easy Experience: Easy O-windows OS: Windows S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants