From 4b7cb9ed2cad8d2834b86e6d8edd47960a0498fd Mon Sep 17 00:00:00 2001 From: cartermp Date: Mon, 7 Dec 2020 15:53:02 -0800 Subject: [PATCH 1/2] Remove known troublesome imports and update tests --- src/MSBuild.Abstractions/BaselineProject.cs | 2 +- .../MSBuildConversionWorkspace.cs | 40 +++++++++++++++--- src/MSBuild.Abstractions/MSBuildHelpers.cs | 12 +++--- src/MSBuild.Abstractions/ProjectExtensions.cs | 4 +- .../AssemblyInfo.fs | 0 .../Program.fs | 0 ...okeTests.FSharpConsoleCoreBaseline.fsproj} | 5 +-- .../AssemblyInfo.fs | 41 +++++++++++++++++++ .../Program.fs | 8 ++++ ...okeTests.FSharpConsoleNet5Baseline.fsproj} | 2 +- .../Smoke.Tests/BasicConversions.cs | 10 ++--- 11 files changed, 101 insertions(+), 23 deletions(-) rename tests/TestData/{SmokeTests.FSharpConsoleBaseline => SmokeTests.FSharpConsoleCoreBaseline}/AssemblyInfo.fs (100%) rename tests/TestData/{SmokeTests.FSharpConsoleBaseline => SmokeTests.FSharpConsoleCoreBaseline}/Program.fs (100%) rename tests/TestData/{SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaseline.fsproj => SmokeTests.FSharpConsoleCoreBaseline/SmokeTests.FSharpConsoleCoreBaseline.fsproj} (73%) create mode 100644 tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/AssemblyInfo.fs create mode 100644 tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/Program.fs rename tests/TestData/{SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaselineNetCoreApp31.fsproj => SmokeTests.FSharpConsoleNet5Baseline/SmokeTests.FSharpConsoleNet5Baseline.fsproj} (92%) diff --git a/src/MSBuild.Abstractions/BaselineProject.cs b/src/MSBuild.Abstractions/BaselineProject.cs index 5998ca61d..d4ee22972 100644 --- a/src/MSBuild.Abstractions/BaselineProject.cs +++ b/src/MSBuild.Abstractions/BaselineProject.cs @@ -31,7 +31,7 @@ private static string AdjustTargetTFM(ProjectStyle projectStyle, ProjectOutputTy return MSBuildFacts.Net5Windows; } - if (projectStyle is not ProjectStyle.MSTest && outputType is ProjectOutputType.Library) + if (projectStyle is not ProjectStyle.MSTest && projectStyle is not ProjectStyle.Web && outputType is ProjectOutputType.Library) { return MSBuildFacts.NetStandard20; } diff --git a/src/MSBuild.Abstractions/MSBuildConversionWorkspace.cs b/src/MSBuild.Abstractions/MSBuildConversionWorkspace.cs index d421b2d74..74b9af619 100644 --- a/src/MSBuild.Abstractions/MSBuildConversionWorkspace.cs +++ b/src/MSBuild.Abstractions/MSBuildConversionWorkspace.cs @@ -34,6 +34,14 @@ public MSBuildConversionWorkspace(ImmutableArray paths, bool noBackup, s continue; } + // This is a hack, but the only way to handle this is to re-architect try-convert along these lines: + // 1. Launch a .NET Framework process using the VS-deployed MSBuild to evaluate a project + // 2. Serialize the evaluation model + // 3. Use the .NET Core process to load MSBuild, but this time from .NET SDK + // 4. Deserialize the evaluation model + // 5. Do conversions + RemoveTargetsNotLoadableByNETSDKMSBuild(path); + var root = new MSBuildProjectRootElement(ProjectRootElement.Open(path, collection, preserveFormatting: true)); if (IsSupportedProjectType(root, forceWeb)) { @@ -89,6 +97,25 @@ public ImmutableDictionary> Determin return builder.ToImmutable(); } + private void RemoveTargetsNotLoadableByNETSDKMSBuild(string path) + { + var projectFile = File.ReadAllText(path); + if (projectFile is { Length:>0 }) + { + var replacement = + projectFile + // Legacy web project specify these two targets paths. They aren't loadable by the .NET SDK msbuild process, so we just remove them. + // They aren't actually useful as an end result when converting web projects. When people conver to .NET Core they will just use the Web SDK attribute. + .Replace("", "") + .Replace("", "") + + // Legacy F# projects specify this import. It's not loadable by the .NET SDK MSBuild, and .NET Core-based F# projects don't use it. So we just remove it. + .Replace("", ""); + + File.WriteAllText(path, replacement); + } + } + /// /// Clear out the project's construction model and add a simple SDK-based project to get a baseline. /// We need to use the same name as the original csproj and same path so that all the default that derive @@ -97,7 +124,7 @@ public ImmutableDictionary> Determin private bool TryCreateSdkBaselineProject(string projectFilePath, IProject project, IProjectRootElement root, ImmutableDictionary> configurations, string tfm, bool keepCurrentTFMs, [NotNullWhen(true)] out BaselineProject? baselineProject) { var projectStyle = GetProjectStyle(root); - var outputType = GetProjectOutputType(root); + var outputType = GetProjectOutputType(root, projectStyle); var rootElement = ProjectRootElement.Open(projectFilePath); rootElement.RemoveAllChildren(); @@ -194,12 +221,15 @@ private bool IsSupportedOutputType(ProjectOutputType type) => _ => false }; - private ProjectOutputType GetProjectOutputType(IProjectRootElement root) + private ProjectOutputType GetProjectOutputType(IProjectRootElement root) => + GetProjectOutputType(root, GetProjectStyle(root)); + + private ProjectOutputType GetProjectOutputType(IProjectRootElement root, ProjectStyle projectStyle) { - if (root.Imports.Any(i => i.Project.Contains(WebFacts.WebApplicationTargets, StringComparison.OrdinalIgnoreCase))) + if (projectStyle == ProjectStyle.Web) { - // ASP.NET Core apps use an EXE output type even though ASP.NET apps use Library - // Note that this specifically looks for WebApplicationTargets (rather than a System.Web reference) since + // ASP.NET Core apps use an EXE output type even though legacy ASP.NET apps use Library + // Note that this specifically checks the project style only (rather than a System.Web reference) since // ASP.NET libraries may reference System.Web and should still use a Library output types. Only ASP.NET // apps should convert with Exe output type. return ProjectOutputType.Exe; diff --git a/src/MSBuild.Abstractions/MSBuildHelpers.cs b/src/MSBuild.Abstractions/MSBuildHelpers.cs index 2e9e1f5a0..a9d4a2fad 100644 --- a/src/MSBuild.Abstractions/MSBuildHelpers.cs +++ b/src/MSBuild.Abstractions/MSBuildHelpers.cs @@ -139,13 +139,11 @@ public static bool ConditionToDimensionValues(string condition, out ImmutableDic /// /// Given a TFM string, determines if that TFM has an explicit System.ValueTuple reference. /// - public static bool FrameworkHasAValueTuple(string tfm) - { - return !tfm.ContainsIgnoreCase(MSBuildFacts.NetstandardPrelude) - && !tfm.ContainsIgnoreCase(MSBuildFacts.NetcoreappPrelude) - && (tfm.StartsWith("net", StringComparison.OrdinalIgnoreCase) - && tfm.StartsWith(MSBuildFacts.LowestFrameworkVersionWithSystemValueTuple)); - } + public static bool FrameworkHasAValueTuple(string tfm) => + tfm.StartsWith(MSBuildFacts.NetstandardPrelude, StringComparison.OrdinalIgnoreCase) + || tfm.StartsWith(MSBuildFacts.NetcoreappPrelude, StringComparison.OrdinalIgnoreCase) + || tfm.StartsWith(MSBuildFacts.Net5, StringComparison.OrdinalIgnoreCase) + || tfm.StartsWith(MSBuildFacts.LowestFrameworkVersionWithSystemValueTuple, StringComparison.OrdinalIgnoreCase); /// /// Gets all Reference items from a given item group. diff --git a/src/MSBuild.Abstractions/ProjectExtensions.cs b/src/MSBuild.Abstractions/ProjectExtensions.cs index 5197cc82c..1f3af0ef3 100644 --- a/src/MSBuild.Abstractions/ProjectExtensions.cs +++ b/src/MSBuild.Abstractions/ProjectExtensions.cs @@ -32,7 +32,9 @@ public static string GetTargetFramework(this IProject project) var tfi = project.GetPropertyValue(MSBuildFacts.LegacyTargetFrameworkPropertyNodeName); if (string.IsNullOrWhiteSpace(tfi)) { - throw new InvalidOperationException($"{MSBuildFacts.LegacyTargetFrameworkPropertyNodeName} is not set!"); + // For some befuddling reason, legacy F# projects don't actually have a TFI. + // We just assume it's .NET Framework though, because like, that's just what it's gonna be anyways. + tfi = ".NETFramework"; } var tfv = project.GetPropertyValue(MSBuildFacts.LegacyTargetFrameworkVersionNodeName); diff --git a/tests/TestData/SmokeTests.FSharpConsoleBaseline/AssemblyInfo.fs b/tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/AssemblyInfo.fs similarity index 100% rename from tests/TestData/SmokeTests.FSharpConsoleBaseline/AssemblyInfo.fs rename to tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/AssemblyInfo.fs diff --git a/tests/TestData/SmokeTests.FSharpConsoleBaseline/Program.fs b/tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/Program.fs similarity index 100% rename from tests/TestData/SmokeTests.FSharpConsoleBaseline/Program.fs rename to tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/Program.fs diff --git a/tests/TestData/SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaseline.fsproj b/tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/SmokeTests.FSharpConsoleCoreBaseline.fsproj similarity index 73% rename from tests/TestData/SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaseline.fsproj rename to tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/SmokeTests.FSharpConsoleCoreBaseline.fsproj index ee7aaa8be..84e23e801 100644 --- a/tests/TestData/SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaseline.fsproj +++ b/tests/TestData/SmokeTests.FSharpConsoleCoreBaseline/SmokeTests.FSharpConsoleCoreBaseline.fsproj @@ -1,7 +1,6 @@ - - + - net472 + netcoreapp3.1 Exe false diff --git a/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/AssemblyInfo.fs b/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/AssemblyInfo.fs new file mode 100644 index 000000000..ebf75cf86 --- /dev/null +++ b/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/AssemblyInfo.fs @@ -0,0 +1,41 @@ +namespace SmokeTests.LegacyFSharpConsole.AssemblyInfo + +open System.Reflection +open System.Runtime.CompilerServices +open System.Runtime.InteropServices + +// General Information about an assembly is controlled through the following +// set of attributes. Change these attribute values to modify the information +// associated with an assembly. +[] +[] +[] +[] +[] +[] +[] +[] + +// Setting ComVisible to false makes the types in this assembly not visible +// to COM components. If you need to access a type in this assembly from +// COM, set the ComVisible attribute to true on that type. +[] + +// The following GUID is for the ID of the typelib if this project is exposed to COM +[] + +// Version information for an assembly consists of the following four values: +// +// Major Version +// Minor Version +// Build Number +// Revision +// +// You can specify all the values or you can default the Build and Revision Numbers +// by using the '*' as shown below: +// [] +[] +[] + +do + () \ No newline at end of file diff --git a/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/Program.fs b/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/Program.fs new file mode 100644 index 000000000..a7458f522 --- /dev/null +++ b/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/Program.fs @@ -0,0 +1,8 @@ +// Learn more about F# at http://fsharp.org + +open System + +[] +let main argv = + printfn "Hello World from F#!" + 0 // return an integer exit code diff --git a/tests/TestData/SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaselineNetCoreApp31.fsproj b/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/SmokeTests.FSharpConsoleNet5Baseline.fsproj similarity index 92% rename from tests/TestData/SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaselineNetCoreApp31.fsproj rename to tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/SmokeTests.FSharpConsoleNet5Baseline.fsproj index f88a79e94..1a429f3d8 100644 --- a/tests/TestData/SmokeTests.FSharpConsoleBaseline/SmokeTests.FSharpConsoleBaselineNetCoreApp31.fsproj +++ b/tests/TestData/SmokeTests.FSharpConsoleNet5Baseline/SmokeTests.FSharpConsoleNet5Baseline.fsproj @@ -1,6 +1,6 @@  - net472 + net5.0 Exe false diff --git a/tests/end-to-end/Smoke.Tests/BasicConversions.cs b/tests/end-to-end/Smoke.Tests/BasicConversions.cs index 0e2563067..f695cd98a 100644 --- a/tests/end-to-end/Smoke.Tests/BasicConversions.cs +++ b/tests/end-to-end/Smoke.Tests/BasicConversions.cs @@ -26,20 +26,20 @@ public BasicSmokeTests(SolutionPathFixture solutionPathFixture, MSBuildFixture m solutionPathFixture.SetCurrentDirectory(); } - [Fact(Skip = "Legacy F# support is not installed on any build machines")] + [Fact] public void ConvertsLegacyFSharpConsoleToNetCoreApp31() { var projectToConvertPath = GetFSharpProjectPath("SmokeTests.LegacyFSharpConsole"); - var projectBaselinePath = GetFSharpProjectPath("SmokeTests.FSharpConsoleBaseline"); + var projectBaselinePath = GetFSharpProjectPath("SmokeTests.FSharpConsoleCoreBaseline"); AssertConversionWorks(projectToConvertPath, projectBaselinePath, "netcoreapp3.1"); } - [Fact(Skip = "Legacy F# support is not installed on any build machines")] + [Fact] public void ConvertsLegacyFSharpConsoleToNet50() { var projectToConvertPath = GetFSharpProjectPath("SmokeTests.LegacyFSharpConsole"); - var projectBaselinePath = GetFSharpProjectPath("SmokeTests.FSharpConsoleBaseline"); - AssertConversionWorks(projectToConvertPath, projectBaselinePath, "netcoreapp3.1"); + var projectBaselinePath = GetFSharpProjectPath("SmokeTests.FSharpConsoleNet5Baseline"); + AssertConversionWorks(projectToConvertPath, projectBaselinePath, "net5.0"); } [Fact] From a37a2404b612c8bc14547f9bf201a67b99c1ad8e Mon Sep 17 00:00:00 2001 From: cartermp Date: Mon, 7 Dec 2020 16:07:30 -0800 Subject: [PATCH 2/2] Remove test cases that don't really make sense anymore (relative to this change) --- tests/unit/ProjectConversion.Tests/ProjectExtensionsTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit/ProjectConversion.Tests/ProjectExtensionsTests.cs b/tests/unit/ProjectConversion.Tests/ProjectExtensionsTests.cs index 3eb2a1fde..98b6a8af7 100644 --- a/tests/unit/ProjectConversion.Tests/ProjectExtensionsTests.cs +++ b/tests/unit/ProjectConversion.Tests/ProjectExtensionsTests.cs @@ -34,9 +34,7 @@ public void GetTargetFramework(string targetFramework, string targetFrameworkIde } [Theory] - [InlineData(null, null, "v4.6")] [InlineData(null, ".NETCoreApp", null)] - [InlineData(null, "Unknown", null)] public void GetTargetFramework_Throws(string targetFramework, string targetFrameworkIdentifier, string targetFrameworkVersion) { var properties = new Dictionary();