-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adds ErrorIfLinkFails attribute for Copy Tasks. #4657
Conversation
1b7d632
to
b25a1ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! This looks like a good implementation of the idea, but I think it needs one more thing besides my comments before we can accept it: easy use of the new functionality!
Can you please introduce a new property similar to CreateHardLinksForCopyFilesToOutputDirectoryIfPossible
that is passed to the Copy task in the same places in Microsoft.Common.CurrentVersion.targets
? ErrorIfLinkFailsForCopyFilesToOutputDirectory
seems like a reasonable name, but if you can think of one that's less of a mouthful I'd love to hear it :)
src/Tasks/Resources/Strings.resx
Outdated
@@ -2682,6 +2682,14 @@ | |||
<data name="Copy.ExactlyOneTypeOfLink"> | |||
<value>MSB3891: Both "{0}" and "{1}" were specified in the project file. Please choose one or the other.</value> | |||
</data> | |||
<data name="Copy.ErrorIfLinkFailsSetWithoutLinkOption" xml:space="preserve"> | |||
<value>MSB3892: ErrorIfLinkedFails requires UseHardlinksIfPossible or UseSymbolicLinksIfPossible to be set.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>MSB3892: ErrorIfLinkedFails requires UseHardlinksIfPossible or UseSymbolicLinksIfPossible to be set.</value> | |
<value>MSB3892: ErrorIfLinkFails requires UseHardlinksIfPossible or UseSymbolicLinksIfPossible to be set.</value> |
src/Tasks/Resources/Strings.resx
Outdated
@@ -2682,6 +2682,14 @@ | |||
<data name="Copy.ExactlyOneTypeOfLink"> | |||
<value>MSB3891: Both "{0}" and "{1}" were specified in the project file. Please choose one or the other.</value> | |||
</data> | |||
<data name="Copy.ErrorIfLinkFailsSetWithoutLinkOption" xml:space="preserve"> | |||
<value>MSB3892: ErrorIfLinkedFails requires UseHardlinksIfPossible or UseSymbolicLinksIfPossible to be set.</value> | |||
<comment>{StrBegin="MSB3892: "}</comment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably clear enough from context, but we try to be explicit about it in other cases:
<comment>{StrBegin="MSB3892: "}</comment> | |
<comment>{StrBegin="MSB3892: "} LOCALIZATION: Do not localize "ErrorIfLinkFails", "UseHardLinksIfPossible", or "UseSymbolicLinksIfPossible".</comment> |
src/Tasks/Resources/Strings.resx
Outdated
</data> | ||
<data name="Copy.LinkFailed" xml:space="preserve"> | ||
<value>MSB3893: Could not use a link to copy "{0}" to "{1}".</value> | ||
<comment>{StrBegin="MSB3893: "}</comment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<comment>{StrBegin="MSB3893: "}</comment> | |
<comment>{StrBegin="MSB3893: "} LOCALIZATION: LOCALIZATION: {0} and {1} are paths.</comment> |
src/Tasks.UnitTests/Copy_Tests.cs
Outdated
@@ -1929,6 +1929,51 @@ public void TooFewRetriesThrows() | |||
engine.AssertLogContains("MSB3027"); | |||
} | |||
|
|||
[Fact(Skip = "Only run if either UseHardlinksIfPossible or UseSymboliclinksIfPossible is true")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to just not mark this as a Fact
, and let that be only in the overrides?
[Fact(Skip = "Only run if either UseHardlinksIfPossible or UseSymboliclinksIfPossible is true")] | |
// Base implementation; only run if either UseHardlinksIfPossible or UseSymboliclinksIfPossible is true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to use
[Fact(Skip = "Only run if either UseHardlinksIfPossible or UseSymboliclinksIfPossible is true")] | |
[ConditionalFact(nameof(LinkOptionIsSpecified)] |
(and implement such a method). That may be a bit cleaner but I don't feel strongly either way.
@@ -1929,6 +1929,51 @@ public void TooFewRetriesThrows() | |||
engine.AssertLogContains("MSB3027"); | |||
} | |||
|
|||
[Fact(Skip = "Only run if either UseHardlinksIfPossible or UseSymboliclinksIfPossible is true")] | |||
[PlatformSpecific(TestPlatforms.Windows)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? And does it actually apply when the "real" test is the overrides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to apply when set on the base method. I found this scenario (trying to copy over a read-only file) fails on Mac and Linux because the link is still successfully created. If you can think of another scenario that should fail to create a link for all platforms I can implement that instead.
f100afd
to
f42402c
Compare
@rainersigwald please see code review changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The targets change looks good to me.
While I was trying to see if I could easily cause the link-creation failures on macOS (I couldn't either, so let's just leave the test Windows-only for the moment), I made some changes (7ae4357) to use some new test infrastructure that makes things a bit more streamlined. This isn't how other tests in this file are written--so you wouldn't have easily known to do so yourself--but we're trying to move newer tests to the newer technique.
@@ -4194,6 +4194,7 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||
<!-- By default we're not using Hard Links to copy to the output directory, and never when building in VS --> | |||
<CreateHardLinksForCopyFilesToOutputDirectoryIfPossible Condition="'$(BuildingInsideVisualStudio)' == 'true' or '$(CreateHardLinksForCopyFilesToOutputDirectoryIfPossible)' == ''">false</CreateHardLinksForCopyFilesToOutputDirectoryIfPossible> | |||
<CreateSymbolicLinksForCopyFilesToOutputDirectoryIfPossible Condition="'$(BuildingInsideVisualStudio)' == 'true' or '$(CreateSymbolicLinksForCopyFilesToOutputDirectoryIfPossible)' == ''">false</CreateSymbolicLinksForCopyFilesToOutputDirectoryIfPossible> | |||
<ErrorIfLinkFailsForCopyFilesToOutputDirectory Condition="'$(BuildingInsideVisualStudio)' == 'true' or '$(ErrorIfLinkFailsForCopyFilesToOutputDirectory)' == ''">false</ErrorIfLinkFailsForCopyFilesToOutputDirectory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'd rather not have to have the in-VS condition, but since the other properties are conditioned on it and there's now an error for specifying this new option when not specifying one of the others, I guess this is the right way to go.
Great! Thanks for the review. |
Thanks again! |
When ErrorIfLinkFails is set, we error if a symbolic link or hard link fails. We also error if ErrorIfLinkFails is set without specifying we should use symbolic or hard linking for Copy tasks.
Addresses #4433.