Allow overriding fallback paths for MSBuildExtensionsPath via a prope…

…rty (dotnet#59)

* Add support for fallback paths to be specified via property references

Currently, the list of fallback paths in app.config can have a property
reference, like:

```
 <searchPaths os="osx">
	<property name="MSBuildExtensionsPath" value="$(FallbackPath);/x/y/z" />
```

.. but it is assumed that such a property will expand to a single path.

This commit adds support for such properties to expand to multiple ';'
separated list of paths. The rules for expansion do not change, they are still
expanded at the time of project import.

With this, given `$(FallbackPath)="/a/b/c;/q/w/e"` and the list of fallback paths
would become `"/a/b/c;/q/w/e;/x/yz"`.

* [osx] Add property allowing users to override fallback paths for ..

.. MSBuildExtensionsPath.

For OSX, this essentially sets the fallback paths to:

`<property name="MSBuildExtensionsPath" value="$(MSBuildExtensionsPathFallbackPathsOverride);/Library/Frameworks/Mono.framework/External/xbuild/" />`

.. whichs allows the default fallback paths to be overridden by setting
a property - `$(MSBuildExtensionsPathFallbackPathsOverride)`.

One of the useful use cases for this is to be able to run tests with
in-tree builds of assemblies/targets meant for
`$(MSBuildExtensionsPath)/`. Simply changing `$(MSBuildExtensionsPath)` would
mean that the in-tree builds of the custom frameworks would work but the
default ones would not be found!

src/Build.UnitTests/BackEnd/TaskRegistry_Tests.cs

@@ -1900,8 +1900,8 @@ public static IEnumerable<object[]> TaskRegistryTranslationTestData
                     "dotv",
                     new Dictionary<string, ProjectImportPathMatch>
                     {
-                        {"a", new ProjectImportPathMatch("a", new List<string> {"b", "c"})},
-                        {"d", new ProjectImportPathMatch("d", new List<string> {"e", "f"})}
+                        {"a", new ProjectImportPathMatch("a", "b;c")},
+                        {"d", new ProjectImportPathMatch("d", "e;f")}
                     }
                 );
 

src/Build.UnitTests/Definition/ToolsetConfigurationReader_Tests.cs

@@ -526,7 +526,7 @@ public void ExtensionPathsTest_Basic1()
                             <property name=""MSBuildExtensionsPath64"" value=""c:\foo64;c:\bar64""/>
                          </searchPaths>
                          <searchPaths os=""osx"">
-                            <property name=""MSBuildExtensionsPath"" value=""/tmp/foo""/>
+                            <property name=""MSBuildExtensionsPath"" value=""/tmp/foo;$(FallbackPaths)""/>
                             <property name=""MSBuildExtensionsPath32"" value=""/tmp/foo32;/tmp/bar32""/>
                          </searchPaths>
                          <searchPaths os=""unix"">
@@ -557,7 +557,7 @@ public void ExtensionPathsTest_Basic1()
 
             Assert.Equal(allPaths.GetElement(1).OS, "osx");
             Assert.Equal(allPaths.GetElement(1).PropertyElements.Count, 2);
-            Assert.Equal(allPaths.GetElement(1).PropertyElements.GetElement("MSBuildExtensionsPath").Value, @"/tmp/foo");
+            Assert.Equal(allPaths.GetElement(1).PropertyElements.GetElement("MSBuildExtensionsPath").Value, @"/tmp/foo;$(FallbackPaths)");
             Assert.Equal(allPaths.GetElement(1).PropertyElements.GetElement("MSBuildExtensionsPath32").Value, @"/tmp/foo32;/tmp/bar32");
 
             Assert.Equal(allPaths.GetElement(2).OS, "unix");
@@ -578,7 +578,7 @@ public void ExtensionPathsTest_Basic1()
             }
             else if (NativeMethodsShared.IsOSX)
             {
-                CheckPathsTable(pathsTable, "MSBuildExtensionsPath", new string[] {"/tmp/foo"});
+                CheckPathsTable(pathsTable, "MSBuildExtensionsPath", new string[] {"/tmp/foo", "$(FallbackPaths)"});
                 CheckPathsTable(pathsTable, "MSBuildExtensionsPath32", new string[] {"/tmp/foo32", "/tmp/bar32"});
             }
             else
@@ -591,11 +591,16 @@ private void CheckPathsTable(Dictionary<string, ProjectImportPathMatch> pathsTab
         {
             Assert.True(pathsTable.ContainsKey(kind));
             var paths = pathsTable[kind];
-            Assert.Equal(paths.SearchPaths.Count, expectedPaths.Length);
 
-            for (int i = 0; i < paths.SearchPaths.Count; i ++)
+            // Property expansion would be done in the context of an
+            // actual project. So, without that, pathsTable would
+            // have the unexpanded strings.
+            var searchPaths = paths.GetExpandedSearchPaths(p => p);
+            Assert.Equal(searchPaths.Count, expectedPaths.Length);
+
+            for (int i = 0; i < searchPaths.Count; i ++)
             {
-                Assert.Equal(paths.SearchPaths[i], expectedPaths[i]);
+                Assert.Equal(searchPaths[i], expectedPaths[i]);
             }
         }
 

src/Build.UnitTests/Definition/Toolset_Tests.cs

@@ -117,7 +117,7 @@ public void ValidateToolsetTranslation()
             Toolset t = new Toolset("4.0", "c:\\bar", buildProperties, environmentProperties, globalProperties,
                 subToolsets, "c:\\foo", "4.0", new Dictionary<string, ProjectImportPathMatch>
                 {
-                    ["MSBuildExtensionsPath"] = new ProjectImportPathMatch("MSBuildExtensionsPath", new List<string> {@"c:\foo"})
+                    ["MSBuildExtensionsPath"] = new ProjectImportPathMatch("MSBuildExtensionsPath", @"c:\foo")
                 });
 
             ((INodePacketTranslatable)t).Translate(TranslationHelpers.GetWriteTranslator());
@@ -162,7 +162,7 @@ public void ValidateToolsetTranslation()
 
             Assert.NotNull(t2.ImportPropertySearchPathsTable);
             Assert.Equal(1, t2.ImportPropertySearchPathsTable.Count);
-            Assert.Equal(@"c:\foo", t2.ImportPropertySearchPathsTable["MSBuildExtensionsPath"].SearchPaths[0]);
+            Assert.Equal(@"c:\foo", t2.ImportPropertySearchPathsTable["MSBuildExtensionsPath"].GetExpandedSearchPaths(p => p)[0]);
         }
 
         [Fact]

src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs

@@ -515,16 +515,31 @@ public void ImportFromExtensionsPathAnd32And64()
             }
         }
 
-        // Fall-back path that has a property in it: $(FallbackExpandDir1)
-        [Fact]
-        public void ExpandExtensionsPathFallback()
+        [Theory]
+        [InlineData(
+                "/a/b;$(FallbackPaths);/x/y",
+                @"<IndirectRefProperty>$(FallbackExpandDir1);/a/b</IndirectRefProperty>
+                <FallbackPaths>/xyz/tmp;$(IndirectRefProperty)</FallbackPaths>", null, false)]
+        // has fallback paths, but won't point to the valid one
+        [InlineData(
+                "/a/b;$(NonExistantProperty)",
+                "",
+                typeof (InvalidProjectFileException), true)]
+        // no fallback paths after expansion, so should ignore fallback-paths code path
+        [InlineData(
+                "$(NonExistantProperty)",
+                @"", typeof(InvalidProjectFileException), false)]
+        // no fallback paths after expansion, so should ignore fallback-paths code path
+        [InlineData(
+                "",
+                @"", typeof(InvalidProjectFileException), false)]
+        public void ExpandExtensionsPathFallback(string fallbackPathsInAppConfig, string propertiesInMainProject, Type expectedException, bool exceptionHasFallbacks)
         {
             string extnTargetsFileContentTemplate = @"
                 <Project xmlns='http://schemas.microsoft.com/developer/msbuild/2003' >
                     <Target Name='FromExtn'>
                         <Message Text='Running FromExtn'/>
                     </Target>
-                    <Import Project='$(MSBuildExtensionsPath)\\foo\\extn.proj' Condition=""Exists('$(MSBuildExtensionsPath)\foo\extn.proj')"" />
                 </Project>";
 
             var configFileContents = @"
@@ -538,7 +553,7 @@ public void ExpandExtensionsPathFallback()
                        <property name=""MSBuildBinPath"" value="".""/>
                        <projectImportSearchPaths>
                          <searchPaths os=""" + NativeMethodsShared.GetOSNameForExtensionsPath() + @""">
-                           <property name=""MSBuildExtensionsPath"" value=""$(FallbackExpandDir1)"" />
+                           <property name=""MSBuildExtensionsPath"" value=""" + fallbackPathsInAppConfig + @""" />
                          </searchPaths>
                        </projectImportSearchPaths>
                       </toolset>
@@ -550,24 +565,57 @@ public void ExpandExtensionsPathFallback()
 
             try
             {
+                string mainTargetsFileContent = @"
+                    <Project xmlns='http://schemas.microsoft.com/developer/msbuild/2003' >
+                        <PropertyGroup>
+                            {0}
+                        </PropertyGroup>
+
+                        <Target Name='Main' DependsOnTargets='FromExtn'>
+                            <Message Text='PropertyFromExtn1: $(PropertyFromExtn1)'/>
+                        </Target>
+
+                        <Import Project='$({1})\foo\extn.proj'/>
+                    </Project>";
+
+                mainTargetsFileContent = string.Format(mainTargetsFileContent, propertiesInMainProject, "MSBuildExtensionsPath");
+                mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj", mainTargetsFileContent);
+
                 extnDir1 = GetNewExtensionsPathAndCreateFile("extensions1", Path.Combine("foo", "extn.proj"),
                     extnTargetsFileContentTemplate);
 
-                mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj",
-                    GetMainTargetFileContent());
-
                 ToolsetConfigurationReaderTestHelper.WriteConfigFile(configFileContents);
                 var reader = GetStandardConfigurationReader();
 
-                var projectCollection = new ProjectCollection(new Dictionary<string, string> {["FallbackExpandDir1"] = extnDir1});
-
-                projectCollection.ResetToolsetsForTests(reader);
                 var logger = new MockLogger();
-                projectCollection.RegisterLogger(logger);
+                try {
+                    var projectCollection = new ProjectCollection(new Dictionary<string, string> {["FallbackExpandDir1"] = extnDir1});
 
-                var project = projectCollection.LoadProject(mainProjectPath);
-                Assert.True(project.Build("Main"));
-                logger.AssertLogContains("Running FromExtn");
+                    projectCollection.ResetToolsetsForTests(reader);
+                    projectCollection.RegisterLogger(logger);
+
+                    var project = projectCollection.LoadProject(mainProjectPath);
+                    Assert.True(project.Build("Main"));
+                    logger.AssertLogContains("Running FromExtn");
+
+                    Assert.True(expectedException == null, $"Didn't get any exception. Expected: {expectedException}");
+                }
+                catch (Exception ex)
+                {
+                    if (expectedException == null)
+                        throw;
+
+                    Assert.Equal(ex.GetType(), expectedException);
+
+                    if (exceptionHasFallbacks)
+                    {
+                        logger.AssertLogContains("in the fallback search path");
+                    }
+                    else
+                    {
+                        logger.AssertLogDoesntContain("in the fallback search path");
+                    }
+                }
             }
             finally
             {

src/Build/Definition/ProjectImportPathMatch.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Shared;
 
@@ -13,19 +14,31 @@ namespace Microsoft.Build.Evaluation
     /// </summary>
     internal class ProjectImportPathMatch : INodePacketTranslatable
     {
+        /// <summary>
+        /// Character used to separate search paths specified for MSBuildExtensionsPath* in
+        /// the config file
+        /// </summary>
+        private static char s_separatorForExtensionsPathSearchPaths = ';';
+
         /// <summary>
         /// ProjectImportPathMatch instance representing no fall-back
         /// </summary>
-        public static readonly ProjectImportPathMatch None = new ProjectImportPathMatch(string.Empty, new List<string>());
+        public static readonly ProjectImportPathMatch None = new ProjectImportPathMatch(string.Empty, string.Empty);
 
-        internal ProjectImportPathMatch(string propertyName, List<string> searchPaths)
+        internal ProjectImportPathMatch(string propertyName, string propertyValue)
         {
             ErrorUtilities.VerifyThrowArgumentNull(propertyName, nameof(propertyName));
-            ErrorUtilities.VerifyThrowArgumentNull(searchPaths, nameof(searchPaths));
+            ErrorUtilities.VerifyThrowArgumentNull(propertyValue, nameof(propertyValue));
 
             PropertyName = propertyName;
-            SearchPaths = searchPaths;
+            PropertyValue = propertyValue;
             MsBuildPropertyFormat = $"$({PropertyName})";
+
+            // cache the result for @propertyValue="" case also
+            if (!ProjectImportPathMatch.HasPropertyReference(propertyValue) || string.IsNullOrEmpty(propertyValue))
+            {
+                SearchPathsWithNoExpansionRequired = ProjectImportPathMatch.SplitSearchPaths(propertyValue);
+            }
         }
 
         public ProjectImportPathMatch(INodePacketTranslator translator)
@@ -38,6 +51,11 @@ public ProjectImportPathMatch(INodePacketTranslator translator)
         /// </summary>
         public string PropertyName;
 
+        /// <summary>
+        /// String representation of the property value
+        /// </summary>
+        public string PropertyValue;
+
         /// <summary>
         /// Returns the corresponding property name - eg. "$(MSBuildExtensionsPath32)"
         /// </summary>
@@ -46,15 +64,44 @@ public ProjectImportPathMatch(INodePacketTranslator translator)
         /// <summary>
         /// Enumeration of the search paths for the property.
         /// </summary>
-        public List<string> SearchPaths;
+        private List<string> SearchPathsWithNoExpansionRequired;
 
         public void Translate(INodePacketTranslator translator)
         {
             translator.Translate(ref PropertyName);
+            translator.Translate(ref PropertyValue);
             translator.Translate(ref MsBuildPropertyFormat);
-            translator.Translate(ref SearchPaths);
+            translator.Translate(ref SearchPathsWithNoExpansionRequired);
         }
 
+        /// <summary>
+        /// Gets the list of search paths with any property references expanded using @expandPropertyReferences
+        /// <param name="expandPropertyReferences">Func that expands properties in a string</param>
+        /// <returns>List of expanded search paths</returns>
+        /// </summary>
+        public IList<string> GetExpandedSearchPaths(Func<string, string> expandPropertyReferences)
+        {
+            ErrorUtilities.VerifyThrowArgumentNull(expandPropertyReferences, nameof(expandPropertyReferences));
+
+            /// If SearchPathsWithNoExpansionRequired is not-null, then it means that the PropertyValue
+            /// did not have any property reference and so we just return the list we prepared during
+            /// construction.
+            return SearchPathsWithNoExpansionRequired ?? ProjectImportPathMatch.SplitSearchPaths(expandPropertyReferences(PropertyValue));
+        }
+
+        /// <summary>
+        /// Splits @fullString on @s_separatorForExtensionsPathSearchPaths
+        /// </summary>
+        /// FIXME: handle ; in path on Unix
+        private static List<string> SplitSearchPaths(string fullString) => fullString
+                                                                        .Split(new[] {s_separatorForExtensionsPathSearchPaths}, StringSplitOptions.RemoveEmptyEntries)
+                                                                        .Distinct().ToList();
+
+        /// <summary>
+        /// Returns true if @value might have a property reference
+        /// </summary>
+        private static bool HasPropertyReference(string value) => value.Contains("$(");
+
         /// <summary>
         /// Factory for serialization.
         /// </summary>
@@ -63,4 +110,4 @@ internal static ProjectImportPathMatch FactoryForDeserialization(INodePacketTran
             return new ProjectImportPathMatch(translator);
         }
     }
-}
+}

src/Build/Definition/ToolsetConfigurationReader.cs

@@ -40,12 +40,6 @@ internal class ToolsetConfigurationReader : ToolsetReader
         /// </summary>
         private bool _configurationReadAttempted = false;
 
-        /// <summary>
-        /// Character used to separate search paths specified for MSBuildExtensionsPath* in
-        /// the config file
-        /// </summary>
-        private char _separatorForExtensionsPathSearchPaths = ';';
-
         /// <summary>
         /// Cached values of tools version -> project import search paths table
         /// </summary>
@@ -239,13 +233,7 @@ private Dictionary<string, ProjectImportPathMatch> ComputeDistinctListOfSearchPa
                     continue;
                 }
 
-                //FIXME: handle ; in path on Unix
-                var paths = property.Value
-                    .Split(new[] {_separatorForExtensionsPathSearchPaths}, StringSplitOptions.RemoveEmptyEntries)
-                    .Distinct()
-                    .Where(path => !string.IsNullOrEmpty(path));
-
-                pathsTable.Add(property.Name, new ProjectImportPathMatch(property.Name, paths.ToList()));
+                pathsTable.Add(property.Name, new ProjectImportPathMatch(property.Name, property.Value));
             }
 
             return pathsTable;
@@ -271,4 +259,4 @@ private static Configuration ReadApplicationConfiguration()
             return ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None);
         }
     }
-}
+}