Skip to content

Conversation

@Forgind
Copy link
Contributor

@Forgind Forgind commented Mar 3, 2023

Fixes https://github.com/dotnet/msbuild/pull/8238/files#r1125001007

Context

#8238 added a reference to "Copy.LinkLibraryFailedPrefix" but didn't actually add that to our .resx. This adds it.

Changes Made

Added a resource

Testing

Notes

dotnet#8238 added a reference to "Copy.LinkLibraryFailedPrefix" but didn't actually add that to our .resx. This adds it.
@Forgind Forgind changed the title Add resource Add resource for failing to copy via hardlink Mar 3, 2023
@radical
Copy link
Member

radical commented Mar 3, 2023

Can we add a test too - for a hardlink creation failing?

@rainersigwald rainersigwald linked an issue Mar 3, 2023 that may be closed by this pull request
Copy link
Member

Choose a reason for hiding this comment

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

Also validate that it contains Copy.RetryingAsFileCopy?

Comment on lines 114 to 116
Copy link
Member

Choose a reason for hiding this comment

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

Why not AssertLogContainsMessageFromResource as other places in this file?

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me. Isn't it unix-only because we have an OS-specific p/invoke?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have matching p/invokes per OS, so one could suppose we could easily craft the appropriate error message a la:
if (NativeMethodsShared.IsWindows)
{
errorMessage = ...
}
else
{
errorMessage = ...
}

But that doesn't work because the windows version isn't from MSBuild and isn't preserved in any particularly relevant form. We can't craft an appropriate error from what's in the test, and I don't think it's worth it to try to plumb the error message back out to some output variable.

Copy link
Member

Choose a reason for hiding this comment

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

Can you put this in CopyHardLink_Tests instead of the base class so it isn't run multiple times?

@JaynieBai
Copy link
Member

@Forgind Sorry for awkward error. The new added test failed since there are not hard link creating failures in the same parent directory. we need a good way to inject failures for hardlink or symlink creation.

@Forgind
Copy link
Contributor Author

Forgind commented Mar 6, 2023

Try making a 000 directory

@JaynieBai
Copy link
Member

JaynieBai commented Mar 7, 2023

Try making a 000 directory

That doesn't work. @Forgind I'm thinking if we could merge the product code fixes and skip the test cases at first. When we have the way to fail the hyperlink, we can enable the test case.

@Forgind
Copy link
Contributor Author

Forgind commented Mar 8, 2023

I agreed with @rainersigwald that since it's difficult to concoct a unit test that fails to create a hardlink on linux, I'd just test it manually and delete the unit test. My results from manual testing show:
Could not use a link to copy "/home/forgind/dotnet-install.sh" to "/mnt/c/Users/forgind/Desktop/di.sh". Copying the file instead. The link() library call failed with the following error code: 18. (TaskId:37)

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 8, 2023
@JaynieBai JaynieBai merged commit 0fb1955 into dotnet:main Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[browser][Codespaces] Build does not pass on Codespaces

4 participants