From cfe5df466fdee0d399ada7607dc10c998048e87a Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Mon, 5 Jun 2023 18:09:12 +0800 Subject: [PATCH 1/5] Catch the illegal argument exception in Net framework --- .../ImportFromMSBuildExtensionsPath_Tests.cs | 42 +++++++++++++++++++ src/Build/Evaluation/Evaluator.cs | 11 ++++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs index 4d4e33ff79d..5414e49d593 100644 --- a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs @@ -10,6 +10,7 @@ using Microsoft.Build.Execution; using Microsoft.Build.Shared; using Xunit; +using Xunit.NetCore.Extensions; #nullable disable @@ -842,6 +843,47 @@ public void FallbackImportWithFileNotFoundWhenPropertyNotDefined() FileUtilities.DeleteDirectoryNoThrow(extnDir1, true); } } + /// + /// https://github.com/dotnet/msbuild/issues/8762 + /// + /// imported project value expression + [WindowsFullFrameworkOnlyTheory] + [InlineData("")] + [InlineData("|")] + public void FallbackImportWithInvalidProjectValue(string projectValue) + { + string mainTargetsFileContent = $""" + + + {projectValue} + + + + """; + + string extnDir1 = null; + string mainProjectPath = null; + + try + { + // The path to "extensions1" fallback should exist, but the file doesn't need to + extnDir1 = GetNewExtensionsPathAndCreateFile("extensions1", Path.Combine("file.props"), string.Empty); + + mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj", mainTargetsFileContent); + var projectCollection = GetProjectCollection(new Dictionary { ["FallbackExpandDir1"] = extnDir1 }); + projectCollection.ResetToolsetsForTests(WriteConfigFileAndGetReader("VSToolsPath", @"$(FallbackExpandDir1)\Microsoft\VisualStudio\v99")); + var logger = new MockLogger(); + projectCollection.RegisterLogger(logger); + + Assert.Throws(() => projectCollection.LoadProject(mainProjectPath)); + logger.AssertLogContains("MSB4020"); + } + finally + { + FileUtilities.DeleteNoThrow(mainProjectPath); + FileUtilities.DeleteDirectoryNoThrow(extnDir1, true); + } + } private void CreateAndBuildProjectForImportFromExtensionsPath(string extnPathPropertyName, Action action) { diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index a80109d4361..a3bf8be37e5 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -2547,7 +2547,16 @@ private void ThrowForImportedProjectWithSearchPathsNotFound(ProjectImportPathMat importElement.Project.Replace(searchPathMatch.MsBuildPropertyFormat, extensionsPathPropValue), ExpanderOptions.ExpandProperties, importElement.ProjectLocation); - relativeProjectPath = FileUtilities.MakeRelative(extensionsPathPropValue, importExpandedWithDefaultPath); + try + { + relativeProjectPath = FileUtilities.MakeRelative(extensionsPathPropValue, importExpandedWithDefaultPath); + } + catch (ArgumentException) + { + // https://github.com/dotnet/msbuild/issues/8762 In NET Framework, Path.* function wil throw exceptions if the path contains invalid characters. + ProjectErrorUtilities.ThrowInvalidProject(importElement.Location, "InvalidAttributeValue", importExpandedWithDefaultPath, XMakeAttributes.project, XMakeElements.import); + return; + } } else { From 657c63ba2dd8933c7a90206221457dfbc9bce542 Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Mon, 5 Jun 2023 18:23:59 +0800 Subject: [PATCH 2/5] Fix comment --- .../Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs | 2 +- src/Build/Evaluation/Evaluator.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs index 5414e49d593..c236c7b793e 100644 --- a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs @@ -844,7 +844,7 @@ public void FallbackImportWithFileNotFoundWhenPropertyNotDefined() } } /// - /// https://github.com/dotnet/msbuild/issues/8762 + /// Fall-back search path on a property that is not valid. https://github.com/dotnet/msbuild/issues/8762 /// /// imported project value expression [WindowsFullFrameworkOnlyTheory] diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index a3bf8be37e5..2c94edcf22b 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -2553,7 +2553,7 @@ private void ThrowForImportedProjectWithSearchPathsNotFound(ProjectImportPathMat } catch (ArgumentException) { - // https://github.com/dotnet/msbuild/issues/8762 In NET Framework, Path.* function wil throw exceptions if the path contains invalid characters. + // https://github.com/dotnet/msbuild/issues/8762 In NET Framework, Path.* function will throw exceptions if the path contains invalid characters. ProjectErrorUtilities.ThrowInvalidProject(importElement.Location, "InvalidAttributeValue", importExpandedWithDefaultPath, XMakeAttributes.project, XMakeElements.import); return; } From 3d012946e302f6f0b5eb65b2840abae33da8e799 Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Tue, 6 Jun 2023 14:59:26 +0800 Subject: [PATCH 3/5] Add exception details in ouput --- .../Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs | 2 +- src/Build/Evaluation/Evaluator.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs index c236c7b793e..c177548e5b1 100644 --- a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs @@ -876,7 +876,7 @@ public void FallbackImportWithInvalidProjectValue(string projectValue) projectCollection.RegisterLogger(logger); Assert.Throws(() => projectCollection.LoadProject(mainProjectPath)); - logger.AssertLogContains("MSB4020"); + logger.AssertLogContains("MSB4102"); } finally { diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index 2c94edcf22b..ee59ed0ace9 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -2551,10 +2551,10 @@ private void ThrowForImportedProjectWithSearchPathsNotFound(ProjectImportPathMat { relativeProjectPath = FileUtilities.MakeRelative(extensionsPathPropValue, importExpandedWithDefaultPath); } - catch (ArgumentException) + catch (ArgumentException ex) { // https://github.com/dotnet/msbuild/issues/8762 In NET Framework, Path.* function will throw exceptions if the path contains invalid characters. - ProjectErrorUtilities.ThrowInvalidProject(importElement.Location, "InvalidAttributeValue", importExpandedWithDefaultPath, XMakeAttributes.project, XMakeElements.import); + ProjectErrorUtilities.ThrowInvalidProject(importElement.Location, "InvalidAttributeValueWithException", importExpandedWithDefaultPath, XMakeAttributes.project, XMakeElements.import, ex.Message); return; } } From 022865970f38beae6113fc353f8b410030fc947d Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Tue, 6 Jun 2023 16:22:28 +0800 Subject: [PATCH 4/5] refactor test case --- .../Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs index c177548e5b1..050c6d4e466 100644 --- a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs @@ -861,17 +861,13 @@ public void FallbackImportWithInvalidProjectValue(string projectValue) """; - string extnDir1 = null; string mainProjectPath = null; try { - // The path to "extensions1" fallback should exist, but the file doesn't need to - extnDir1 = GetNewExtensionsPathAndCreateFile("extensions1", Path.Combine("file.props"), string.Empty); - mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj", mainTargetsFileContent); - var projectCollection = GetProjectCollection(new Dictionary { ["FallbackExpandDir1"] = extnDir1 }); - projectCollection.ResetToolsetsForTests(WriteConfigFileAndGetReader("VSToolsPath", @"$(FallbackExpandDir1)\Microsoft\VisualStudio\v99")); + var projectCollection = GetProjectCollection(); + projectCollection.ResetToolsetsForTests(WriteConfigFileAndGetReader("VSToolsPath", "temp")); var logger = new MockLogger(); projectCollection.RegisterLogger(logger); @@ -881,7 +877,6 @@ public void FallbackImportWithInvalidProjectValue(string projectValue) finally { FileUtilities.DeleteNoThrow(mainProjectPath); - FileUtilities.DeleteDirectoryNoThrow(extnDir1, true); } } From 1a764608129bb1b4c50eb41a6b6c698f4782ee05 Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Thu, 8 Jun 2023 15:43:20 +0800 Subject: [PATCH 5/5] Refactor the test case --- .../ImportFromMSBuildExtensionsPath_Tests.cs | 25 ++++++++++--------- src/Build/Evaluation/Evaluator.cs | 2 +- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs index 050c6d4e466..e21015b4c68 100644 --- a/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs +++ b/src/Build.UnitTests/Evaluation/ImportFromMSBuildExtensionsPath_Tests.cs @@ -847,7 +847,7 @@ public void FallbackImportWithFileNotFoundWhenPropertyNotDefined() /// Fall-back search path on a property that is not valid. https://github.com/dotnet/msbuild/issues/8762 /// /// imported project value expression - [WindowsFullFrameworkOnlyTheory] + [Theory] [InlineData("")] [InlineData("|")] public void FallbackImportWithInvalidProjectValue(string projectValue) @@ -861,22 +861,23 @@ public void FallbackImportWithInvalidProjectValue(string projectValue) """; - string mainProjectPath = null; + using TestEnvironment testEnvironment = TestEnvironment.Create(); + string mainProjectPath = testEnvironment.CreateTestProjectWithFiles("main.proj", mainTargetsFileContent).ProjectFile; + var projectCollection = GetProjectCollection(); + projectCollection.ResetToolsetsForTests(WriteConfigFileAndGetReader("VSToolsPath", "temp")); + var logger = new MockLogger(); + projectCollection.RegisterLogger(logger); + Assert.Throws(() => projectCollection.LoadProject(mainProjectPath)); - try + if (string.IsNullOrEmpty(projectValue)) { - mainProjectPath = ObjectModelHelpers.CreateFileInTempProjectDirectory("main.proj", mainTargetsFileContent); - var projectCollection = GetProjectCollection(); - projectCollection.ResetToolsetsForTests(WriteConfigFileAndGetReader("VSToolsPath", "temp")); - var logger = new MockLogger(); - projectCollection.RegisterLogger(logger); - - Assert.Throws(() => projectCollection.LoadProject(mainProjectPath)); logger.AssertLogContains("MSB4102"); } - finally + else { - FileUtilities.DeleteNoThrow(mainProjectPath); +#if NETFRAMEWORK + logger.AssertLogContains("MSB4102"); +#endif } } diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs index ee59ed0ace9..89318337c7f 100644 --- a/src/Build/Evaluation/Evaluator.cs +++ b/src/Build/Evaluation/Evaluator.cs @@ -2553,7 +2553,7 @@ private void ThrowForImportedProjectWithSearchPathsNotFound(ProjectImportPathMat } catch (ArgumentException ex) { - // https://github.com/dotnet/msbuild/issues/8762 In NET Framework, Path.* function will throw exceptions if the path contains invalid characters. + // https://github.com/dotnet/msbuild/issues/8762 .Catch the exceptions when extensionsPathPropValue is null or importExpandedWithDefaultPath is empty. In NET Framework, Path.* function also throws exceptions if the path contains invalid characters. ProjectErrorUtilities.ThrowInvalidProject(importElement.Location, "InvalidAttributeValueWithException", importExpandedWithDefaultPath, XMakeAttributes.project, XMakeElements.import, ex.Message); return; }