[Bug]: Copying a file onto itself deletes the file #8684
Comments
Forgind
changed the title
This was referenced Apr 20, 2023
JaynieBai
pushed a commit
that referenced
this issue
Apr 23, 2023
This reverts commit a93882f. This is a temporary fix for #8684 The current plan is to revert #8275 in 17.6, as it caused some difficulties, and try to bring it back in 17.7 via #8685. Summary #8275 fixed a longstanding confusing and unfortunate behavior in MSBuild in which passing the Copy task a symlink as its destination would copy the source file onto the destination of the symlink rather than overwriting the symlink. Unfortunately, it also introduced a new issue in which copying a file onto itself could often just delete the file instead of copying anything. Customers reported this issue. Customer Impact Projects that copy a file onto itself using the Copy task without passing identical paths for source and destination instead delete the file without necessarily even logging an error. Regression? Yes, from #8275. Testing Unit tests and manually tested that the repro described in #8684 no longer works. Risk Minimal (straight revert of the commit that caused the bug) --------- Co-authored-by: Forgind <Forgind@users.noreply.github.com>
Forgind
added a commit
that referenced
this issue
May 5, 2023
Fixes #8684 Fixes #8273 Context After #8275, we delete any destination file as part of the Copy task if we determine that we really should copy onto it. Unfortunately, if we try to copy a file onto itself, we delete it before we can copy onto itself, which just means it's gone. Fortunately, we have a check earlier that ensures that we skip any copy operation from a location to the same location. Unfortunately, it's a direct string comparison that doesn't evaluate to full paths first, so it misses slightly more complicated examples. Changes Made Take into account full paths Testing Unit tests + manual test that it doesn't delete the file anymore Notes This implementation tries to remove now-unnecessary full path derivations downstream, hence some added complexity, but it still means extra computation on the happy path if we end up creating a hard/symbolic link. An alternate direction eliminating any full path derivations on the happy path would save about 4% of Copy's execution time, per a quick perf test. (With how many samples I used, "no change" is within a standard deviation.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Issue Description
When calling the Copy task, we now delete the destination file (if present) before copying the source file to that location:
#8275
That ensures that if the destination file is actually a symbolic link pointing to, say, the user's nuget cache, we don't write through the link and corrupt the file at the other end.
However, that causes a problem when the source file and destination file are the same. We delete the source file, and then there's no file to copy to the destination location!
Confusingly, this doesn't even throw an exception when the real problem occurs because we're already past the existence checks in Copy, and File.Copy doesn't know how to get its exception to MSBuild loggers. An exception is thrown on the second build when the Copy task is invoked with a file that never existed or during the first build if the copied file is subsequently used.
Steps to Reproduce
Create a new console app with
dotnet new console. Add this target:Execute the target with
msbuild <project> /t:BadCopyExpected Behavior
Copies Program.cs on top of itself
Actual Behavior
Deletes Program.cs
Analysis
The
.\in the Target is intentional and necessary. We have a check earlier in Copy that the SourceFile and DestinationFile have different "Name"s. This is the path passed in, however, not the full path. This can also occur if someone wants to copy a relative path on top of a full path or vice versa.Versions & Configurations
No response
The text was updated successfully, but these errors were encountered: