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

Copy task errors when copying a non existent file on itself #9240

Closed
wants to merge 10 commits into from

Conversation

maridematte
Copy link
Member

Fixes #9148

Context

During the copy task, if the origin and destination file have the same path (are the same file), and that file does not exist, we do not get an error and the task succeeds. It was decided that an error should be thrown instead.

Changes Made

In the Copy Task, I added a check for file existence if the file paths are the same. The process now throws an error if the file does not exist.

Testing

Added a unit test for the case

@rokonec rokonec closed this Sep 25, 2023
@rokonec rokonec reopened this Sep 25, 2023
@maridematte maridematte reopened this Sep 27, 2023
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!


Assert.False(success);

((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); // Didn't do retries, nothing to do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably as well assert on presence of 'MSB3030' ("Could not copy the file "{0}" because it was not found.")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would a hypothetical AssertLogDoesntContainAnyMSBCodes() be useful

Copy link
Member

@JanKrivanek JanKrivanek Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssertLogDoesntContainAnyMSBCodes(List<string>? except = null) or AssertLogContainsOnlyMSBCodes(List<string>? allowedCodes = null)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't part of the point of this that MSB3030 should show up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - I meant we should assert that the code is present.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@@ -1406,6 +1406,32 @@ public void CopyFileOnItself2(bool isUseHardLinks, bool isUseSymbolicLinks)
}
}

[Theory]
[MemberData(nameof(GetHardLinksSymLinks))]
public void CopyFileItselfNonExistentFile(bool isUseHardLinks, bool isUseSymbolicLinks)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior doesn't care if you're using hard links or symlinks or neither, right? Why do you need to run it four times?

Also:

Suggested change
public void CopyFileItselfNonExistentFile(bool isUseHardLinks, bool isUseSymbolicLinks)
public void CopyNonexistentFileOntoItselfThrows(bool isUseHardLinks, bool isUseSymbolicLinks)

@maridematte maridematte closed this Nov 7, 2023
@maridematte maridematte deleted the 8924 branch January 10, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy task's "skip copies if source and destination are the same" optimization fires when file doesn't exist
5 participants