Log an error if MSBuildProjectExtensionsPath is modified after it is …

…used (#3059)

* Log an error if MSBuildProjectExtensionsPath is modified after it is used
* Add warning for BaseIntermediateOutputPath if specified

src/Tasks.UnitTests/ProjectExtensionsImportTestBase.cs

@@ -3,6 +3,8 @@
 using Microsoft.Build.Evaluation;
 using System;
 using System.IO;
+using System.Linq;
+using Shouldly;
 using Xunit;
 
 namespace Microsoft.Build.UnitTests
@@ -57,10 +59,10 @@ public void DoesNotImportProjectIfNotExist()
 
             string projectExtensionsPath = project.GetPropertyValue("MSBuildProjectExtensionsPath");
 
-            Assert.True(!String.IsNullOrWhiteSpace(projectExtensionsPath), "The property 'MSBuildProjectExtensionsPath' should not be empty during project evaluation.");
-            Assert.True(!Directory.Exists(projectExtensionsPath), $"The project extension directory '{projectExtensionsPath}' should not exist.");
-            Assert.Equal("true", project.GetPropertyValue(PropertyNameToEnableImport), StringComparer.OrdinalIgnoreCase);
-            Assert.Equal(String.Empty, project.GetPropertyValue(PropertyNameToSignalImportSucceeded), StringComparer.OrdinalIgnoreCase);
+            projectExtensionsPath.ShouldNotBeNullOrWhiteSpace();
+            Directory.Exists(projectExtensionsPath).ShouldBeFalse();
+            project.GetPropertyValue(PropertyNameToEnableImport).ShouldBe("true");
+            project.GetPropertyValue(PropertyNameToSignalImportSucceeded).ShouldBeEmpty();
         }
 
         /// <summary>
@@ -92,10 +94,10 @@ public void DoesNotImportProjectWhenDisabled()
 
             string projectExtensionsDirectory = Path.Combine(ObjectModelHelpers.TempProjectDir, Path.GetDirectoryName(ImportProjectPath));
 
-            Assert.Equal("false", project.GetPropertyValue(PropertyNameToEnableImport), StringComparer.OrdinalIgnoreCase);
-            Assert.Equal(String.Empty, project.GetPropertyValue(PropertyNameToSignalImportSucceeded), StringComparer.OrdinalIgnoreCase);
-            Assert.True(Directory.Exists(projectExtensionsDirectory), $"The directory '{projectExtensionsDirectory}' should exist but doesn't.");
-            Assert.Equal($@"{projectExtensionsDirectory}{Path.DirectorySeparatorChar}", project.GetPropertyValue("MSBuildProjectExtensionsPath"));
+            project.GetPropertyValue(PropertyNameToEnableImport).ShouldBe("false");
+            project.GetPropertyValue(PropertyNameToSignalImportSucceeded).ShouldBeEmpty();
+            Directory.Exists(projectExtensionsDirectory).ShouldBeTrue();
+            project.GetPropertyValue("MSBuildProjectExtensionsPath").ShouldBe($@"{projectExtensionsDirectory}{Path.DirectorySeparatorChar}");
         }
 
         /// <summary>
@@ -121,8 +123,8 @@ public void ImportsProjectIfCustomPath()
                 </Project>
             "));
 
-            Assert.Equal("true", project.GetPropertyValue(PropertyNameToEnableImport), StringComparer.OrdinalIgnoreCase);
-            Assert.Equal("true", project.GetPropertyValue(PropertyNameToSignalImportSucceeded), StringComparer.OrdinalIgnoreCase);
+            project.GetPropertyValue(PropertyNameToEnableImport).ShouldBe("true");
+            project.GetPropertyValue(PropertyNameToSignalImportSucceeded).ShouldBe("true");
         }
 
         /// <summary>
@@ -145,8 +147,60 @@ public void ImportsProjectIfExists()
                 </Project>
             "));
 
-            Assert.Equal("true", project.GetPropertyValue(PropertyNameToEnableImport), StringComparer.OrdinalIgnoreCase);
-            Assert.Equal("true", project.GetPropertyValue(PropertyNameToSignalImportSucceeded), StringComparer.OrdinalIgnoreCase);
+            project.GetPropertyValue(PropertyNameToEnableImport).ShouldBe("true");
+            project.GetPropertyValue(PropertyNameToSignalImportSucceeded).ShouldBe("true");
+        }
+
+        /// <summary>
+        /// Ensures that an error is logged if MSBuildProjectExtensionsPath is modified after it was set by Microsoft.Common.props.
+        /// </summary>
+        [Fact]
+        public void ErrorIfChangedInBodyOfProject()
+        {
+            Project project = ObjectModelHelpers.LoadProjectFileInTempProjectDirectory(ObjectModelHelpers.CreateFileInTempProjectDirectory(_projectRelativePath, @"
+                <Project DefaultTargets=`Build` ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`http://schemas.microsoft.com/developer/msbuild/2003`>
+                    <Import Project=`$(MSBuildBinPath)\Microsoft.Common.props` />
+
+                    <PropertyGroup>
+                        <MSBuildProjectExtensionsPath>foo</MSBuildProjectExtensionsPath>
+                    </PropertyGroup>
+
+                    <Import Project=`$(MSBuildBinPath)\Microsoft.CSharp.targets` />
+                </Project>
+            "));
+
+            MockLogger logger = new MockLogger();
+
+            project.Build("_CheckForInvalidConfigurationAndPlatform", new[] {logger}).ShouldBeFalse();
+
+            logger.Errors.Select(i => i.Code).FirstOrDefault().ShouldBe("MSB3540");
+        }
+
+        /// <summary>
+        /// Ensures that an error is logged if BaseIntermediateOutputPath is modified after it was set by Microsoft.Common.props and 
+        /// EnableBaseIntermediateOutputPathMismatchWarning is 'true'.
+        /// </summary>
+        [Fact]
+        public void WarningIfBaseIntermediateOutputPathIsChangedInBodyOfProject()
+        {
+            Project project = ObjectModelHelpers.LoadProjectFileInTempProjectDirectory(ObjectModelHelpers.CreateFileInTempProjectDirectory(_projectRelativePath, @"
+                <Project DefaultTargets=`Build` ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`http://schemas.microsoft.com/developer/msbuild/2003`>
+                    <Import Project=`$(MSBuildBinPath)\Microsoft.Common.props` />
+
+                    <PropertyGroup>
+                        <EnableBaseIntermediateOutputPathMismatchWarning>true</EnableBaseIntermediateOutputPathMismatchWarning>
+                        <BaseIntermediateOutputPath>foo</BaseIntermediateOutputPath>
+                    </PropertyGroup>
+
+                    <Import Project=`$(MSBuildBinPath)\Microsoft.CSharp.targets` />
+                </Project>
+            "));
+
+            MockLogger logger = new MockLogger();
+
+            project.Build("_CheckForInvalidConfigurationAndPlatform", new[] { logger }).ShouldBeTrue();
+
+            logger.Warnings.Select(i => i.Code).FirstOrDefault().ShouldBe("MSB3539");
         }
     }
-}
+}

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -794,6 +794,31 @@ Copyright (C) Microsoft Corporation. All rights reserved.
     <PropertyGroup Condition="'$(Prefer32Bit)' == 'true' and ('$(PlatformTarget)' == 'AnyCPU' or '$(PlatformTarget)' == '') and '$(PlatformTargetAsMSBuildArchitectureExplicitlySet)' != 'true'">
       <PlatformTargetAsMSBuildArchitecture>x86</PlatformTargetAsMSBuildArchitecture>
     </PropertyGroup>
+
+    <!--
+      Log an error if the user set MSBuildProjectExtensionsPath in the body of a project. In an SDK style project
+      if you set a value in the body, the value is not used by the top implicit import but is used by the bottom.
+      This can lead to confusing behavior and builds can fail for obscure reasons.
+    -->
+    <Error Condition=" '$(_InitialMSBuildProjectExtensionsPath)' != '' And '$(MSBuildProjectExtensionsPath)' != '$(_InitialMSBuildProjectExtensionsPath)' "
+           Code="MSB3540"
+           Text="The value of the property &quot;MSBuildProjectExtensionsPath&quot; was modified after it was used by MSBuild which can lead to unexpected build results.  To set this property, you must do so before Microsoft.Common.props is imported, for example by using Directory.Build.props.  For more information, please visit https://go.microsoft.com/fwlink/?linkid=869650"
+            />
+
+    <!--
+      Log a warning if:
+        1. $(EnableBaseIntermediateOutputPathMismatchWarning) is 'true'
+        2. $(BaseIntermediateOutputPath) was set in the body of a project after its default value was set in Microsoft.Common.props
+        3. $(BaseIntermediateOutputPath) is not the same as $(MSBuildProjectExtensionsPath)
+
+      Similar to the error above, there are cases when users set $(BaseIntermediateOutputPath) in the body of their project and things build but only by coincidence.
+      MSBuild does not know if $(BaseIntermediateOutputPath) changing would cause problems so tools like NuGet must set $(EnableBaseIntermediateOutputPathMismatchWarning)
+      to 'true'.
+    -->
+    <Warning Condition=" '$(EnableBaseIntermediateOutputPathMismatchWarning)' == 'true' And '$(_InitialBaseIntermediateOutputPath)' != '$(BaseIntermediateOutputPath)' And '$(BaseIntermediateOutputPath)' != '$(MSBuildProjectExtensionsPath)' "
+             Code="MSB3539"
+             Text="The value of the property &quot;BaseIntermediateOutputPath&quot; was modified after it was used by MSBuild which can lead to unexpected build results. Tools such as NuGet will write outputs to the path specified by the &quot;MSBuildProjectExtensionsPath&quot; instead. To set this property, you must do so before Microsoft.Common.props is imported, for example by using Directory.Build.props.  For more information, please visit https://go.microsoft.com/fwlink/?linkid=869650"
+             />
   </Target>
 
   <!--

src/Tasks/Microsoft.Common.props

@@ -49,6 +49,8 @@ Copyright (C) Microsoft Corporation. All rights reserved.
         -->
     <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)'=='' ">obj\</BaseIntermediateOutputPath>
     <BaseIntermediateOutputPath Condition="!HasTrailingSlash('$(BaseIntermediateOutputPath)')">$(BaseIntermediateOutputPath)\</BaseIntermediateOutputPath>
+    <_InitialBaseIntermediateOutputPath>$(BaseIntermediateOutputPath)</_InitialBaseIntermediateOutputPath>
+
     <MSBuildProjectExtensionsPath Condition="'$(MSBuildProjectExtensionsPath)' == '' ">$(BaseIntermediateOutputPath)</MSBuildProjectExtensionsPath>
     <!--
         Import paths that are relative default to be relative to the importing file.  However, since MSBuildExtensionsPath
@@ -58,6 +60,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
     <MSBuildProjectExtensionsPath Condition="'$([System.IO.Path]::IsPathRooted($(MSBuildProjectExtensionsPath)))' == 'false'">$([System.IO.Path]::Combine('$(MSBuildProjectDirectory)', '$(MSBuildProjectExtensionsPath)'))</MSBuildProjectExtensionsPath>
     <MSBuildProjectExtensionsPath Condition="!HasTrailingSlash('$(MSBuildProjectExtensionsPath)')">$(MSBuildProjectExtensionsPath)\</MSBuildProjectExtensionsPath>
     <ImportProjectExtensionProps Condition="'$(ImportProjectExtensionProps)' == ''">true</ImportProjectExtensionProps>
+    <_InitialMSBuildProjectExtensionsPath Condition=" '$(ImportProjectExtensionProps)' == 'true' ">$(MSBuildProjectExtensionsPath)</_InitialMSBuildProjectExtensionsPath>
   </PropertyGroup>
 
   <Import Project="$(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props" Condition="'$(ImportProjectExtensionProps)' == 'true' and exists('$(MSBuildProjectExtensionsPath)')" />

src/Tasks/Resources/Strings.resx

@@ -2681,7 +2681,7 @@
             MSB3501 - MSB3510   Task: ReadLinesFromFile
             MSB3511 - MSB3520   Task: Message
             MSB3521 - MSB3530   Task: Warning
-            MSB3531 - MSB3540   Task: Error
+            MSB3531 - MSB3540   Task: Error    // MSB3539, MSB3540 is used in Microsoft.Common.CurrentVersion.targets, do not reuse
             MSB3541 - MSB3550   Task: FindUnderPath
             MSB3551 - MSB3580   Task: GenerateResource     // MSB3561, MSB3571 not used but already shipped, do not reuse
             MSB3581 - MSB3600   Task: ResolveNonMSBuildProjectOutput