From 1811906ae6048694bb3100c7a182414323472b82 Mon Sep 17 00:00:00 2001 From: "Eric St. John" Date: Thu, 5 Jan 2017 13:56:55 -0800 Subject: [PATCH] Handle conflicts between ref and copy local We need to look at target path of private references as well as compare that to copy-local items. --- .../tools/HandlePackageFileConflicts.cs | 154 ++++++++++++++---- netstandard/tools/MSBuildUtilities.cs | 69 ++++++++ netstandard/tools/NETStandard.Tools.csproj | 1 + 3 files changed, 192 insertions(+), 32 deletions(-) create mode 100644 netstandard/tools/MSBuildUtilities.cs diff --git a/netstandard/tools/HandlePackageFileConflicts.cs b/netstandard/tools/HandlePackageFileConflicts.cs index ecc4e460e4c8..2706a911f92d 100644 --- a/netstandard/tools/HandlePackageFileConflicts.cs +++ b/netstandard/tools/HandlePackageFileConflicts.cs @@ -5,11 +5,9 @@ using Microsoft.Build.Framework; using Microsoft.Build.Utilities; using System; -using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.IO; -using System.Reflection; namespace Microsoft.DotNet.Build.Tasks { @@ -36,9 +34,22 @@ public partial class HandlePackageFileConflicts : Task public override bool Execute() { - ReferencesWithoutConflicts = HandleConflicts(References, GetReferenceKey); + // remove References that will conflict at compile + var referencesWithoutConflicts = HandleConflicts(References, GetReferenceKey); - ReferenceCopyLocalPathsWithoutConflicts = HandleConflicts(ReferenceCopyLocalPaths, GetTargetPath); + // remove References that will conflict in output + Dictionary referencesByTargetPath; + referencesWithoutConflicts = HandleConflicts(referencesWithoutConflicts, GetReferenceTargetPath, out referencesByTargetPath); + + // remove ReferenceCopyLocalPaths that will conflict in output. + Dictionary referenceCopyLocalPathsByTargetPath; + var referenceCopyLocalPathsWithoutConflicts = HandleConflicts(ReferenceCopyLocalPaths, GetTargetPath, out referenceCopyLocalPathsByTargetPath); + + // remove ReferenceCopyLocalPaths & set References to Private=false that will conflict in output. + referenceCopyLocalPathsWithoutConflicts = HandleConflicts(referenceCopyLocalPathsWithoutConflicts, referenceCopyLocalPathsByTargetPath, referencesByTargetPath); + + ReferencesWithoutConflicts = referencesWithoutConflicts; + ReferenceCopyLocalPathsWithoutConflicts = referenceCopyLocalPathsWithoutConflicts; return !Log.HasLoggedErrors; } @@ -66,13 +77,28 @@ private void EnsurePackageRanks() } /// - /// Examines items for conflicting targetpath and chooses the best item. + /// Examines items for conflicting keys and chooses the best item. /// - /// - /// + /// Items from which to remove conflicts + /// Delegate to calculate key for an item + /// Items without conflicts private ITaskItem[] HandleConflicts(ITaskItem[] items, Func getItemKey) { - var winningItems = new Dictionary(StringComparer.OrdinalIgnoreCase); + Dictionary itemsByKeyUnused; + + return HandleConflicts(items, getItemKey, out itemsByKeyUnused); + } + + /// + /// Examines items for conflicting keys and chooses the best item. + /// + /// Items from which to remove conflicts + /// Delegate to calculate key for an item + /// Dictionary of items by key without conflicts + /// Items without conflicts + private ITaskItem[] HandleConflicts(ITaskItem[] items, Func getItemKey, out Dictionary winningItemsByKey) + { + winningItemsByKey = new Dictionary(StringComparer.OrdinalIgnoreCase); // ensure we use reference equality and not any overridden equality operators on items var conflictsToRemove = new HashSet(ReferenceComparer.Instance); @@ -80,14 +106,14 @@ private ITaskItem[] HandleConflicts(ITaskItem[] items, Func g { var itemKey = getItemKey(item); - if (itemKey == null) + if (String.IsNullOrEmpty(itemKey)) { continue; } ITaskItem existingItem; - if (winningItems.TryGetValue(itemKey, out existingItem)) + if (winningItemsByKey.TryGetValue(itemKey, out existingItem)) { // a conflict was found, determine the winner. var winner = HandleConflict(existingItem, item); @@ -99,10 +125,10 @@ private ITaskItem[] HandleConflicts(ITaskItem[] items, Func g continue; } - if (winner != existingItem) + if (!ReferenceEquals(winner, existingItem)) { // replace existing item and add it to conflict list. - winningItems[itemKey] = item; + winningItemsByKey[itemKey] = item; conflictsToRemove.Add(existingItem); } else @@ -113,13 +139,67 @@ private ITaskItem[] HandleConflicts(ITaskItem[] items, Func g } else { - winningItems[itemKey] = item; + winningItemsByKey[itemKey] = item; } } return RemoveConflicts(items, conflictsToRemove); } + /// + /// Examines private (copy-local) references amd referenceCopyLocalPaths for inter-conflicts and + /// demotes references to non-private or removes items from referenceCopyLocalPaths. + /// + /// ReferenceCopyLocalPaths item, already filtered for intra-conflicts + /// ReferenceCopyLocalPaths already filtered for intra-conflicts, indexed by TargetPath + /// References item for private references, already filtered for compile-time intra-conflicts and copy-local intra-conflicts, indexed by TargetPath. Items within this collection will be modified as neecessary to handle conflicts with ReferenceCopyLocalPaths by making the reference Private=false. + /// ReferenceCopyLocalPaths item filtered for inter-conflicts with References + private ITaskItem[] HandleConflicts(ITaskItem[] referenceCopyLocalPaths, Dictionary referenceCopyLocalPathsByTargetPath, Dictionary referencesByTargetPath) + { + if (referencesByTargetPath.Count == 0 || referenceCopyLocalPathsByTargetPath.Count == 0) + { + // nothing to do if either item set doesn't have targetpaths + return referenceCopyLocalPaths; + } + + // ensure we use reference equality and not any overridden equality operators on items + var conflictsToRemove = new HashSet(ReferenceComparer.Instance); + + // find conflicts between the two, arbitrarily choose ref to iterate over + foreach (var referenceByTargetPath in referencesByTargetPath) + { + var targetPath = referenceByTargetPath.Key; + var reference = referenceByTargetPath.Value; + + ITaskItem referenceCopyLocalPath = null; + + if (referenceCopyLocalPathsByTargetPath.TryGetValue(targetPath, out referenceCopyLocalPath)) + { + // a conflict was found, determine the winner. + var winner = HandleConflict(reference, referenceCopyLocalPath); + + if (winner == null) + { + // no winner, skip it. + continue; + } + + if (ReferenceEquals(winner, referenceCopyLocalPath)) + { + // ReferenceCopyLocalPath won, make the reference not private, so that it will not copy-local + reference.SetMetadata("Private", "False"); + } + else + { + // Reference won, remove the copy-local item. + conflictsToRemove.Add(referenceCopyLocalPath); + } + } + } + + return RemoveConflicts(referenceCopyLocalPaths, conflictsToRemove); + } + private Version GetFileVersion(string sourcePath) { var fvi = FileVersionInfo.GetVersionInfo(sourcePath); @@ -173,7 +253,13 @@ private static string GetReferenceKey(ITaskItem item) if (!String.IsNullOrEmpty(aliases)) { - // skip conflict detection for aliased assemblies + // skip compile-time conflict detection for aliased assemblies. + // An alias is the way to avoid a conflict + // eg: System, v1.0.0.0 in global will not conflict with System, v2.0.0.0 in `private` alias + // We could model each alias scope and try to check for conflicts within that scope, + // but this is a ton of complexity for a fringe feature. + // Instead, we'll treat an alias as an indication that the developer has opted out of + // conflict resolution. return null; } @@ -188,31 +274,35 @@ private static string GetReferenceKey(ITaskItem item) return null; } - // The following is similar to Path.GetFileName but does not throw and returns null - // rather than empty for a directory. - var length = sourcePath.Length; - for (int i = length - 1; --i >= 0;) + try + { + return Path.GetFileName(sourcePath); + } + catch(ArgumentException) { - var ch = sourcePath[i]; + // We won't even try to resolve a conflict if we can't open the file, so ignore invalid paths + return null; + } + } - if (ch == Path.DirectorySeparatorChar || ch == Path.AltDirectorySeparatorChar || ch == Path.VolumeSeparatorChar) - { - // string terminated with a separator: we were given a directory, ignore it. - if (i == length - 1) - { - return null; - } + private static string GetReferenceTargetPath(ITaskItem item) + { + // Determine if the reference will be copied local. + // We're only dealing with primary file references. For these RAR will + // copy local if Private is true or unset. - // we found a separator, return filename portion. - var startOfFileName = i + 1; - return sourcePath.Substring(startOfFileName); - } + var isPrivate = MSBuildUtilities.ConvertStringToBool(item.GetMetadata("Private"), defaultValue: true); + + if (!isPrivate) + { + // Private = false means the reference shouldn't be copied. + return null; } - // could not find a separator, return source path - return sourcePath; + return GetTargetPath(item); } + private static string GetSourcePath(ITaskItem item) { var sourcePath = item.GetMetadata("HintPath"); diff --git a/netstandard/tools/MSBuildUtilities.cs b/netstandard/tools/MSBuildUtilities.cs new file mode 100644 index 000000000000..2af2c2e27d9c --- /dev/null +++ b/netstandard/tools/MSBuildUtilities.cs @@ -0,0 +1,69 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. +using System; + +namespace Microsoft.DotNet.Build.Tasks +{ + /// + /// Internal utilties copied from microsoft/MSBuild repo. + /// + class MSBuildUtilities + { + /// + /// Converts a string to a bool. We consider "true/false", "on/off", and + /// "yes/no" to be valid boolean representations in the XML. + /// Modified from its original version to not throw, but return a default value. + /// + /// The string to convert. + /// Boolean true or false, corresponding to the string. + internal static bool ConvertStringToBool(string parameterValue, bool defaultValue = false) + { + if (String.IsNullOrEmpty(parameterValue)) + { + return defaultValue; + } + else if (ValidBooleanTrue(parameterValue)) + { + return true; + } + else if (ValidBooleanFalse(parameterValue)) + { + return false; + } + else + { + // Unsupported boolean representation. + return defaultValue; + } + } + + /// + /// Returns true if the string represents a valid MSBuild boolean true value, + /// such as "on", "!false", "yes" + /// + private static bool ValidBooleanTrue(string parameterValue) + { + return ((String.Compare(parameterValue, "true", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "on", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "yes", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "!false", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "!off", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "!no", StringComparison.OrdinalIgnoreCase) == 0)); + } + + /// + /// Returns true if the string represents a valid MSBuild boolean false value, + /// such as "!on" "off" "no" "!true" + /// + private static bool ValidBooleanFalse(string parameterValue) + { + return ((String.Compare(parameterValue, "false", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "off", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "no", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "!true", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "!on", StringComparison.OrdinalIgnoreCase) == 0) || + (String.Compare(parameterValue, "!yes", StringComparison.OrdinalIgnoreCase) == 0)); + } + } +} diff --git a/netstandard/tools/NETStandard.Tools.csproj b/netstandard/tools/NETStandard.Tools.csproj index 99fd2dc45e55..049899d57a9d 100644 --- a/netstandard/tools/NETStandard.Tools.csproj +++ b/netstandard/tools/NETStandard.Tools.csproj @@ -12,6 +12,7 @@ +