Revert "Allow overriding fallback paths for MSBuildExtensionsPath via…

src/Build.UnitTests/BackEnd/TaskRegistry_Tests.cs

@@ -1897,8 +1897,8 @@ public static IEnumerable<object[]> TaskRegistryTranslationTestData
                     "dotv",
                     new Dictionary<string, ProjectImportPathMatch>
                     {
-                        {"a", new ProjectImportPathMatch("a", "b;c")},
-                        {"d", new ProjectImportPathMatch("d", "e;f")}
+                        {"a", new ProjectImportPathMatch("a", new List<string> {"b", "c"})},
+                        {"d", new ProjectImportPathMatch("d", new List<string> {"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;$(FallbackPaths)""/>
+                            <property name=""MSBuildExtensionsPath"" value=""/tmp/foo""/>
                             <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;$(FallbackPaths)");
+            Assert.Equal(allPaths.GetElement(1).PropertyElements.GetElement("MSBuildExtensionsPath").Value, @"/tmp/foo");
             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", "$(FallbackPaths)"});
+                CheckPathsTable(pathsTable, "MSBuildExtensionsPath", new string[] {"/tmp/foo"});
                 CheckPathsTable(pathsTable, "MSBuildExtensionsPath32", new string[] {"/tmp/foo32", "/tmp/bar32"});
             }
             else
@@ -591,16 +591,11 @@ private void CheckPathsTable(Dictionary<string, ProjectImportPathMatch> pathsTab
         {
             Assert.True(pathsTable.ContainsKey(kind));
             var paths = pathsTable[kind];
+            Assert.Equal(paths.SearchPaths.Count, expectedPaths.Length);
 
-            // 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 ++)
+            for (int i = 0; i < paths.SearchPaths.Count; i ++)
             {
-                Assert.Equal(searchPaths[i], expectedPaths[i]);
+                Assert.Equal(paths.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", @"c:\foo")
+                    ["MSBuildExtensionsPath"] = new ProjectImportPathMatch("MSBuildExtensionsPath", new List<string> {@"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"].GetExpandedSearchPaths(p => p)[0]);
+            Assert.Equal(@"c:\foo", t2.ImportPropertySearchPathsTable["MSBuildExtensionsPath"].SearchPaths[0]);
         }
 
         [Fact]

src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs

@@ -515,31 +515,16 @@ public void ImportFromExtensionsPathAnd32And64()
             }
         }
 
-        [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)
+        // Fall-back path that has a property in it: $(FallbackExpandDir1)
+        [Fact]
+        public void ExpandExtensionsPathFallback()
         {
             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 = @"
@@ -553,7 +538,7 @@ public void ExpandExtensionsPathFallback(string fallbackPathsInAppConfig, string
                        <property name=""MSBuildBinPath"" value="".""/>
                        <projectImportSearchPaths>
                          <searchPaths os=""" + NativeMethodsShared.GetOSNameForExtensionsPath() + @""">
-                           <property name=""MSBuildExtensionsPath"" value=""" + fallbackPathsInAppConfig + @""" />
+                           <property name=""MSBuildExtensionsPath"" value=""$(FallbackExpandDir1)"" />
                          </searchPaths>
                        </projectImportSearchPaths>
                       </toolset>
@@ -565,57 +550,24 @@ public void ExpandExtensionsPathFallback(string fallbackPathsInAppConfig, string
 
             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 logger = new MockLogger();
-                try {
-                    var projectCollection = new ProjectCollection(new Dictionary<string, string> {["FallbackExpandDir1"] = extnDir1});
-
-                    projectCollection.ResetToolsetsForTests(reader);
-                    projectCollection.RegisterLogger(logger);
-
-                    var project = projectCollection.LoadProject(mainProjectPath);
-                    Assert.True(project.Build("Main"));
-                    logger.AssertLogContains("Running FromExtn");
+                var projectCollection = new ProjectCollection(new Dictionary<string, string> {["FallbackExpandDir1"] = extnDir1});
 
-                    Assert.True(expectedException == null, $"Didn't get any exception. Expected: {expectedException}");
-                }
-                catch (Exception ex)
-                {
-                    if (expectedException == null)
-                        throw;
-
-                    Assert.Equal(ex.GetType(), expectedException);
+                projectCollection.ResetToolsetsForTests(reader);
+                var logger = new MockLogger();
+                projectCollection.RegisterLogger(logger);
 
-                    if (exceptionHasFallbacks)
-                    {
-                        logger.AssertLogContains("in the fallback search path");
-                    }
-                    else
-                    {
-                        logger.AssertLogDoesntContain("in the fallback search path");
-                    }
-                }
+                var project = projectCollection.LoadProject(mainProjectPath);
+                Assert.True(project.Build("Main"));
+                logger.AssertLogContains("Running FromExtn");
             }
             finally
             {

src/Build/Definition/ProjectImportPathMatch.cs

@@ -3,7 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Linq;
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Shared;
 
@@ -14,31 +13,19 @@ 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, string.Empty);
+        public static readonly ProjectImportPathMatch None = new ProjectImportPathMatch(string.Empty, new List<string>());
 
-        internal ProjectImportPathMatch(string propertyName, string propertyValue)
+        internal ProjectImportPathMatch(string propertyName, List<string> searchPaths)
         {
             ErrorUtilities.VerifyThrowArgumentNull(propertyName, nameof(propertyName));
-            ErrorUtilities.VerifyThrowArgumentNull(propertyValue, nameof(propertyValue));
+            ErrorUtilities.VerifyThrowArgumentNull(searchPaths, nameof(searchPaths));
 
             PropertyName = propertyName;
-            PropertyValue = propertyValue;
+            SearchPaths = searchPaths;
             MsBuildPropertyFormat = $"$({PropertyName})";
-
-            // cache the result for @propertyValue="" case also
-            if (!ProjectImportPathMatch.HasPropertyReference(propertyValue) || string.IsNullOrEmpty(propertyValue))
-            {
-                SearchPathsWithNoExpansionRequired = ProjectImportPathMatch.SplitSearchPaths(propertyValue);
-            }
         }
 
         public ProjectImportPathMatch(INodePacketTranslator translator)
@@ -51,11 +38,6 @@ 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>
@@ -64,44 +46,15 @@ public ProjectImportPathMatch(INodePacketTranslator translator)
         /// <summary>
         /// Enumeration of the search paths for the property.
         /// </summary>
-        private List<string> SearchPathsWithNoExpansionRequired;
+        public List<string> SearchPaths;
 
         public void Translate(INodePacketTranslator translator)
         {
             translator.Translate(ref PropertyName);
-            translator.Translate(ref PropertyValue);
             translator.Translate(ref MsBuildPropertyFormat);
-            translator.Translate(ref SearchPathsWithNoExpansionRequired);
+            translator.Translate(ref SearchPaths);
         }
 
-        /// <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>
@@ -110,4 +63,4 @@ internal static ProjectImportPathMatch FactoryForDeserialization(INodePacketTran
             return new ProjectImportPathMatch(translator);
         }
     }
-}
+}

src/Build/Definition/ToolsetConfigurationReader.cs

@@ -36,6 +36,12 @@ 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>
@@ -229,7 +235,13 @@ private Dictionary<string, ProjectImportPathMatch> ComputeDistinctListOfSearchPa
                     continue;
                 }
 
-                pathsTable.Add(property.Name, new ProjectImportPathMatch(property.Name, property.Value));
+                //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()));
             }
 
             return pathsTable;
@@ -255,4 +267,4 @@ private static Configuration ReadApplicationConfiguration()
             return ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None);
         }
     }
-}
+}