Breaking change in Microsoft.Common.targets (UseSymboliclinksIfPossible)

[AndyGerlicher]

This is introduced in #742. We (community contribution!) added support for symbolic links on the Copy task. I just happened to be looking into an issue with Xamarin targets and stumbled upon an issue.

In Microsoft.Common.CurrentVersion.targets we now call Copy like:

<Copy
    SourceFiles = "@(_SourceItemsToCopyToOutputDirectory)"
    DestinationFiles = "@(_SourceItemsToCopyToOutputDirectory->'$(OutDir)%(TargetPath)')"
    OverwriteReadOnlyFiles="$(OverwriteReadOnlyFiles)"
    Retries="$(CopyRetryCount)"
    RetryDelayMilliseconds="$(CopyRetryDelayMilliseconds)"
    UseHardlinksIfPossible="$(CreateHardLinksForAdditionalFilesIfPossible)"
    UseSymboliclinksIfPossible="$(CreateSymbolicLinksForAdditionalFilesIfPossible)"

I don't see any problem with this in general. However, the Xamarin targets (specifically iOS are the targets I'm looking at) add <UsingTask TaskName="Microsoft.Build.Tasks.Copy" AssemblyFile="Xamarin.iOS.Tasks.dll"/>. This causes all Copy tasks to use the one from Xamarin.iOS.Tasks.dll rather than the builtin. Unfortunetely, this Copy task derives from Microsoft.Build.Tasks.v4.0.dll which does not have the new UseSymboliclinksIfPossible property.

It seems like we have a few options. 1) Fix Xamarin targets to bind to a more recent version of MSBuild (Microsoft.Build.Tasks.Core). This would have implications as to what version the target could run on (MSBuild 14+) but would be preferable if that's ok. 2) Modify the targets to not specify the property unless it's set. This seems like it could be pretty ugly. Or 3) find a way to turn on this feature in a backwards compatible way?

@kzu do you have any input on this? Is option 1 something you could do?

[rainersigwald]

Another option would be to add a property with this name to the Xamarin override. It could be ignored--it just has to exist. That would be backward compatible since you can always add an optional property and it'll just get a default value.

[kzu]

Yes, but the problem with that @rainersigwald is that the local-side of the build would not have this new feature, since we'd continue to inherit from the 4.0 task.

We can implement a mix of option 1 and 3:

• Move the tasks that override the built-in ones to two assemblies: Xamarin.Build.Tasks and Xamarin.Build.Tasks.Core, with the latter inheriting from the 14+ ones.
• Conditionally declare the Using depending on the MSBuildToolsVersion being >= than 14
• Declare the property in the pre-14 Tasks assembly so that they are no-op and "just work" (do nothing) on pre-14 builds (i.e. xbuild)

[cdmihai]

How about we change the common targets to something like this, to allow old and new behaviour. Xamarin msbuild code can make sure CreateSymbolicLinksForAdditionalFilesIfPossible is not set to true

<Copy Condition="'$(CreateSymbolicLinksForAdditionalFilesIfPossible)' == 'true'"
    SourceFiles = "@(_SourceItemsToCopyToOutputDirectory)"
    DestinationFiles = "@(_SourceItemsToCopyToOutputDirectory->'$(OutDir)%(TargetPath)')"
    OverwriteReadOnlyFiles="$(OverwriteReadOnlyFiles)"
    Retries="$(CopyRetryCount)"
    RetryDelayMilliseconds="$(CopyRetryDelayMilliseconds)"
    UseHardlinksIfPossible="$(CreateHardLinksForAdditionalFilesIfPossible)"
    UseSymboliclinksIfPossible="$(CreateSymbolicLinksForAdditionalFilesIfPossible)"

<Copy Condition="'$(CreateSymbolicLinksForAdditionalFilesIfPossible)' != 'true'"
    SourceFiles = "@(_SourceItemsToCopyToOutputDirectory)"
    DestinationFiles = "@(_SourceItemsToCopyToOutputDirectory->'$(OutDir)%(TargetPath)')"
    OverwriteReadOnlyFiles="$(OverwriteReadOnlyFiles)"
    Retries="$(CopyRetryCount)"
    RetryDelayMilliseconds="$(CopyRetryDelayMilliseconds)"
    UseHardlinksIfPossible="$(CreateHardLinksForAdditionalFilesIfPossible)"

[AndyGerlicher]

@cdmihai that was option 2 and is super ugly as there are quite a few call sites to consider. I don't think that's a very reasonable option :(.

@rainersigwald suggestion seems good. Xamarin task can have that property (public bool UseSymboliclinksIfPossible { get; set; } that does nothing. This would solve the issue completely and is probably the least impactful. The task could still inherit from 4.0 and be fine since it will technically have the property. You could also have the set throw if set to true so if someone expected to use it they would know they could not. @kzu this seems like it could be the option that would have the least impact on mono msbuild as well since it would still be the v4.0 dll.

[kzu]

@AndyGerlicher my concern is that we'd be breaking symlink behavior on Xamarin projects so people won't be able to leverage it at all, even for targets that don't involve remotely building on the Mac (i.e. you can still use Copy on Windows only if you add a Condition=" '$(BuildSessionId)' == '' " for example. But maybe it's too much of a corner case.

We can fix it with the given suggestion, and wait for users to ask for the additional support if needed.

[kzu]

PRs on their way at dotnet/macios#648 (master) and dotnet/macios#649 (cycle8 / next dev15 drop)

[AndyGerlicher]

@kzu is there a build I could try and verify the fix?

[kzu]

@AndyGerlicher not yet, likely tonight or early tomorrow