From ad88058266825e454268e79d9a8a4dfbc5e684fe Mon Sep 17 00:00:00 2001 From: Livar Cunha Date: Wed, 28 Dec 2016 14:48:38 -0800 Subject: [PATCH] Stopped doing hoisting of transitive project dependencies and references. We also don't validate if a dependency exist, since migration strictly speaking does not need it. We move the project.json and the csproj will fail to build just like the project.json would. --- .../MigrateAppWithMissingDep/.noautobuild | 0 .../MyApp/.noautobuild | 0 .../MigrateAppWithMissingDep/MyApp/Program.cs | 10 --- .../MyApp/project.json | 20 ------ .../MyLib/NotUsedButDoNotRemove.cs | 5 -- .../ProjectDependency.cs | 4 +- .../ProjectDependencyFinder.cs | 28 ++------ .../Rules/MigrateProjectDependenciesRule.cs | 69 +------------------ ...enThatIWantToMigrateProjectDependencies.cs | 57 +++++---------- .../GivenThatIWantToMigrateTestApps.cs | 15 +--- 10 files changed, 30 insertions(+), 178 deletions(-) delete mode 100644 TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/.noautobuild delete mode 100644 TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/.noautobuild delete mode 100644 TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/Program.cs delete mode 100644 TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/project.json delete mode 100644 TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyLib/NotUsedButDoNotRemove.cs diff --git a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/.noautobuild b/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/.noautobuild deleted file mode 100644 index e69de29bb..000000000 diff --git a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/.noautobuild b/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/.noautobuild deleted file mode 100644 index e69de29bb..000000000 diff --git a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/Program.cs b/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/Program.cs deleted file mode 100644 index a552b21e7..000000000 --- a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/Program.cs +++ /dev/null @@ -1,10 +0,0 @@ -namespace ConsoleApplication -{ - public class Program - { - public static void Main(string[] args) - { - } - } -} - diff --git a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/project.json b/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/project.json deleted file mode 100644 index e10324be5..000000000 --- a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyApp/project.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "version": "1.0.0-*", - "buildOptions": { - "emitEntryPoint": true - }, - - "dependencies": { - "Microsoft.NETCore.App": { - "type": "platform", - "version": "1.0.1" - }, - "MyLib": "1.0.0-*" - }, - - "frameworks": { - "netcoreapp1.0": { - "imports": "dnxcore50" - } - } -} diff --git a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyLib/NotUsedButDoNotRemove.cs b/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyLib/NotUsedButDoNotRemove.cs deleted file mode 100644 index d16e6e2d3..000000000 --- a/TestAssets/NonRestoredTestProjects/MigrateAppWithMissingDep/MyLib/NotUsedButDoNotRemove.cs +++ /dev/null @@ -1,5 +0,0 @@ -// This file needs to be here as this error does not repro if the MyLib folder doesn't exist -// Since git does not keep track of folders and files only, this folder needs to contain any file. -namespace MyLib -{ -} diff --git a/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependency.cs b/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependency.cs index afea501a7..d830182b8 100644 --- a/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependency.cs +++ b/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependency.cs @@ -7,13 +7,11 @@ namespace Microsoft.DotNet.ProjectJsonMigration { public string Name { get; } public string ProjectFilePath { get; } - public bool Hoisted { get; } - public ProjectDependency(string name, string projectFilePath, bool hoisted) + public ProjectDependency(string name, string projectFilePath) { Name = name; ProjectFilePath = System.IO.Path.GetFullPath(projectFilePath); - Hoisted = hoisted; } public override bool Equals(object obj) diff --git a/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependencyFinder.cs b/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependencyFinder.cs index dc449b60d..825c4695a 100644 --- a/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependencyFinder.cs +++ b/src/Microsoft.DotNet.ProjectJsonMigration/ProjectDependencyFinder.cs @@ -80,34 +80,23 @@ namespace Microsoft.DotNet.ProjectJsonMigration projectContext.ProjectFile, framework, preResolvedProjects, - solutionFile, - HoistDependenciesThatAreNotDirectDependencies(projectToResolve, project) + solutionFile ); - projects.AddRange(dependencies); allDependencies.UnionWith(dependencies); } return allDependencies; } - private bool HoistDependenciesThatAreNotDirectDependencies( - ProjectDependency originalProject, - ProjectDependency dependenciesOwner) - { - return originalProject != dependenciesOwner; - } - public IEnumerable ResolveDirectProjectDependenciesForFramework( Project project, NuGetFramework framework, IEnumerable preResolvedProjects=null, - SlnFile solutionFile = null, - bool hoistedDependencies = false) + SlnFile solutionFile = null) { preResolvedProjects = preResolvedProjects ?? new HashSet(); - var possibleProjectDependencies = - FindPossibleProjectDependencies(solutionFile, project.ProjectFilePath, hoistedDependencies); + var possibleProjectDependencies = FindPossibleProjectDependencies(solutionFile, project.ProjectFilePath); var projectDependencies = new List(); @@ -233,8 +222,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration private Dictionary FindPossibleProjectDependencies( SlnFile slnFile, - string projectJsonFilePath, - bool hoistedDependencies = false) + string projectJsonFilePath) { var projectRootDirectory = GetRootFromProjectJson(projectJsonFilePath); @@ -249,7 +237,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration var projects = new Dictionary(StringComparer.Ordinal); - foreach (var project in GetPotentialProjects(projectSearchPaths, hoistedDependencies)) + foreach (var project in GetPotentialProjects(projectSearchPaths)) { if (projects.ContainsKey(project.Name)) { @@ -311,8 +299,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration /// Create the list of potential projects from the search paths. /// private static List GetPotentialProjects( - IEnumerable searchPaths, - bool hoistedDependencies = false) + IEnumerable searchPaths) { var projects = new List(); @@ -338,8 +325,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration // Check if we've already added this, just in case it was pre-loaded into the cache var project = new ProjectDependency( projectDirectory.Name, - projectFilePath, - hoistedDependencies); + projectFilePath); projects.Add(project); } diff --git a/src/Microsoft.DotNet.ProjectJsonMigration/Rules/MigrateProjectDependenciesRule.cs b/src/Microsoft.DotNet.ProjectJsonMigration/Rules/MigrateProjectDependenciesRule.cs index a1f0d0fb6..f697a6f46 100644 --- a/src/Microsoft.DotNet.ProjectJsonMigration/Rules/MigrateProjectDependenciesRule.cs +++ b/src/Microsoft.DotNet.ProjectJsonMigration/Rules/MigrateProjectDependenciesRule.cs @@ -113,16 +113,13 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Rules ProjectRootElement outputMSBuildProject) { var projectDependencies = _projectDependencyFinder.ResolveAllProjectDependenciesForFramework( - new ProjectDependency(project.Name, project.ProjectFilePath, false), + new ProjectDependency(project.Name, project.ProjectFilePath), framework, migratedXProjDependencyNames, solutionFile); var projectDependencyTransformResults = - projectDependencies.Select(p => - p.Hoisted ? - HoistedDependencyTransform.Transform(p) : - ProjectDependencyTransform.Transform(p)); + projectDependencies.Select(p => ProjectDependencyTransform.Transform(p)); if (projectDependencyTransformResults.Any()) { @@ -131,56 +128,6 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Rules projectDependencyTransformResults, framework); } - - HoistFrameworkAssembliesForProjectDependencies(projectDependencies, outputMSBuildProject); - } - - private void HoistFrameworkAssembliesForProjectDependencies( - IEnumerable projectDependencies, - ProjectRootElement outputMSBuildProject) - { - foreach (var projectDependency in projectDependencies) - { - HoistFrameworkAssembliesForDesktopFrameworks(projectDependency, outputMSBuildProject); - } - } - - private void HoistFrameworkAssembliesForDesktopFrameworks( - ProjectDependency projectDependency, - ProjectRootElement outputMSBuildProject) - { - var targetFrameworks = ProjectReader - .GetProject(projectDependency.ProjectFilePath) - .GetTargetFrameworks().Where(p => !p.FrameworkName.IsPackageBased); - - foreach (var targetFramework in targetFrameworks) - { - HoistFrameworkAssemblies(targetFramework, outputMSBuildProject); - } - } - - private void HoistFrameworkAssemblies( - TargetFrameworkInformation targetFramework, - ProjectRootElement outputMSBuildProject) - { - var frameworkAssemblies = targetFramework.Dependencies.Where(d => - d.LibraryRange.TypeConstraint == LibraryDependencyTarget.Reference); - if(frameworkAssemblies.Any()) - { - var condition = targetFramework.FrameworkName.GetMSBuildCondition(); - var itemGroup = - outputMSBuildProject.ItemGroups.FirstOrDefault(i => i.Condition == condition) ?? - outputMSBuildProject.AddItemGroup(); - itemGroup.Condition = condition; - - foreach (var frameworkAssembly in frameworkAssemblies) - { - _transformApplicator.Execute( - FrameworkDependencyTransform.Transform(frameworkAssembly), - itemGroup, - mergeExisting: true); - } - } } private void AddProjectDependenciesToNewItemGroup( @@ -202,10 +149,6 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Rules private AddItemTransform ProjectDependencyTransform => GetProjectDependencyTransfrom(); - private AddItemTransform HoistedDependencyTransform => - GetProjectDependencyTransfrom() - .WithMetadata("FromP2P", "true"); - private Func> GetProjectDependencyTransfrom => () => new AddItemTransform( "ProjectReference", @@ -225,13 +168,5 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Rules path => path, path => "", path => true); - - private AddItemTransform FrameworkDependencyTransform => - new AddItemTransform( - "Reference", - dep => dep.Name, - dep => "", - dep => true) - .WithMetadata("FromP2P", "true"); } } diff --git a/test/Microsoft.DotNet.ProjectJsonMigration.Tests/Rules/GivenThatIWantToMigrateProjectDependencies.cs b/test/Microsoft.DotNet.ProjectJsonMigration.Tests/Rules/GivenThatIWantToMigrateProjectDependencies.cs index 52900923a..028bda3a8 100644 --- a/test/Microsoft.DotNet.ProjectJsonMigration.Tests/Rules/GivenThatIWantToMigrateProjectDependencies.cs +++ b/test/Microsoft.DotNet.ProjectJsonMigration.Tests/Rules/GivenThatIWantToMigrateProjectDependencies.cs @@ -18,7 +18,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests public class GivenThatIWantToMigrateProjectDependencies : TestBase { [Fact] - public void Project_dependencies_are_migrated_to_ProjectReference() + public void ProjectDependenciesAreMigratedToProjectReference() { var solutionDirectory = TestAssetsManager.CreateTestInstance("TestAppWithLibrary", callingMethod: "p").Path; @@ -41,7 +41,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void It_does_not_migrate_a_dependency_with_target_package_that_has_a_matching_project_as_a_ProjectReference() + public void ItDoesNotMigrateADependencyWithTargetPackageThatHasAMatchingProjectAsAProjectReference() { var testAssetsManager = GetTestGroupTestAssetsManager("NonRestoredTestProjects"); var solutionDirectory = @@ -62,7 +62,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void TFM_specific_Project_dependencies_are_migrated_to_ProjectReference_under_condition_ItemGroup() + public void TFMSpecificProjectDependenciesAreMigratedToProjectReferenceUnderConditionItemGroup() { var solutionDirectory = TestAssetsManager.CreateTestInstance("TestAppWithLibraryUnderTFM", callingMethod: "p").Path; @@ -85,7 +85,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void It_throws_when_project_dependency_is_unresolved() + public void ItThrowsWhenProjectDependencyIsUnresolved() { // No Lock file => unresolved var solutionDirectory = @@ -108,7 +108,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests [Theory] [InlineData(@"some/path/to.cSproj", new [] { @"some/path/to.cSproj" })] [InlineData(@"to.CSPROJ",new [] { @"to.CSPROJ" })] - public void It_migrates_csproj_ProjectReference_in_xproj(string projectReference, string[] expectedMigratedReferences) + public void ItMigratesCsprojProjectReferenceInXproj(string projectReference, string[] expectedMigratedReferences) { var xproj = ProjectRootElement.Create(); xproj.AddItem("ProjectReference", projectReference); @@ -137,7 +137,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void It_migrates_csproj_ProjectReference_in_xproj_including_condition_on_ProjectReference() + public void ItMigratesCsprojProjectReferenceInXprojIncludingConditionOnProjectReference() { var projectReference = "some/to.csproj"; var xproj = ProjectRootElement.Create(); @@ -171,7 +171,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void It_migrates_csproj_ProjectReference_in_xproj_including_condition_on_ProjectReference_parent() + public void ItMigratesCsprojProjectReferenceInXprojIncludingConditionOnProjectReferenceParent() { var projectReference = "some/to.csproj"; var xproj = ProjectRootElement.Create(); @@ -205,7 +205,7 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void It_migrates_csproj_ProjectReference_in_xproj_including_condition_on_ProjectReference_parent_and_item() + public void ItMigratesCsprojProjectReferenceInXprojIncludingConditionOnProjectReferenceParentAndItem() { var projectReference = "some/to.csproj"; var xproj = ProjectRootElement.Create(); @@ -240,17 +240,17 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests } [Fact] - public void It_promotes_P2P_references_up_in_the_dependency_chain() + public void ItDoesNotPromoteP2PReferencesUpInTheDependencyChain() { var mockProj = MigrateProject("TestAppDependencyGraph", "ProjectA"); var projectReferences = mockProj.Items.Where( item => item.ItemType.Equals("ProjectReference", StringComparison.Ordinal)); - projectReferences.Count().Should().Be(7); + projectReferences.Count().Should().Be(2); } [Fact] - public void It_promotes_FrameworkAssemblies_from_P2P_references_up_in_the_dependency_chain() + public void ItDoesNotPromoteFrameworkAssembliesFromP2PReferencesUpInTheDependencyChain() { var solutionDirectory = TestAssets.Get(TestAssetKinds.DesktopTestProjects, "TestAppWithFrameworkAssemblies") .CreateInstance() @@ -269,32 +269,11 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests item => item.ItemType == "Reference" && item.Include == "System.ComponentModel.DataAnnotations" && item.Parent.Condition == " '$(TargetFramework)' == 'net451' "); - frameworkAssemblyReferences.Count().Should().Be(1); + frameworkAssemblyReferences.Count().Should().Be(0); } [Fact] - public void All_promoted_P2P_references_are_marked_with_a_FromP2P_attribute() - { - var expectedHoistedProjectReferences = new [] { - Path.Combine("..", "ProjectD", "ProjectD.csproj"), - Path.Combine("..", "ProjectE", "ProjectE.csproj"), - Path.Combine("..", "CsprojLibrary1", "CsprojLibrary1.csproj"), - Path.Combine("..", "CsprojLibrary2", "CsprojLibrary2.csproj"), - Path.Combine("..", "CsprojLibrary3", "CsprojLibrary3.csproj") - }; - - var mockProj = MigrateProject("TestAppDependencyGraph", "ProjectA"); - - var projectReferences = mockProj.Items - .Where(item => - item.ItemType == "ProjectReference" && item.GetMetadataWithName("FromP2P") != null) - .Select(i => i.Include); - - projectReferences.Should().BeEquivalentTo(expectedHoistedProjectReferences); - } - - [Fact] - public void All_non_promoted_P2P_references_are_not_marked_with_a_FromP2P_attribute() + public void NoP2PReferenceIsMarkedWithAFromP2PAttribute() { var expectedNonHoistedProjectReferences = new [] { Path.Combine("..", "ProjectB", "ProjectB.csproj"), @@ -305,14 +284,16 @@ namespace Microsoft.DotNet.ProjectJsonMigration.Tests var projectReferences = mockProj.Items .Where(item => - item.ItemType == "ProjectReference" && item.GetMetadataWithName("FromP2P") == null) - .Select(i => i.Include); + item.ItemType == "ProjectReference"); - projectReferences.Should().BeEquivalentTo(expectedNonHoistedProjectReferences); + projectReferences.Should().HaveCount(c => c == 2) + .And.OnlyContain(item => item.GetMetadataWithName("FromP2P") == null); + + projectReferences.Select(i => i.Include).Should().BeEquivalentTo(expectedNonHoistedProjectReferences); } [Fact] - public void It_migrates_unqualified_dependencies_as_ProjectReference_when_a_matching_project_is_found() + public void ItMigratesUnqualifiedDependenciesAsProjectReferenceWhenAMatchingProjectIsFound() { var mockProj = MigrateProject("TestAppWithUnqualifiedDependencies", "ProjectA"); var projectReferenceInclude = Path.Combine("..", "ProjectB", "ProjectB.csproj"); diff --git a/test/dotnet-migrate.Tests/GivenThatIWantToMigrateTestApps.cs b/test/dotnet-migrate.Tests/GivenThatIWantToMigrateTestApps.cs index 30cb3cc22..98325b076 100644 --- a/test/dotnet-migrate.Tests/GivenThatIWantToMigrateTestApps.cs +++ b/test/dotnet-migrate.Tests/GivenThatIWantToMigrateTestApps.cs @@ -484,20 +484,7 @@ namespace Microsoft.DotNet.Migration.Tests Restore(projectDirectory, projectName); BuildMSBuild(projectDirectory, projectName); } - - [Fact] - public void ItFailsGracefullyWhenMigratingAppWithMissingDependency() - { - string projectName = "MigrateAppWithMissingDep"; - var projectDirectory = Path.Combine(GetTestGroupTestAssetsManager("NonRestoredTestProjects").CreateTestInstance(projectName).Path, "MyApp"); - - string migrationOutputFile = Path.Combine(projectDirectory, "migration-output.json"); - File.Exists(migrationOutputFile).Should().BeFalse(); - MigrateCommand.Run(new string[] { projectDirectory, "-r", migrationOutputFile, "--format-report-file-json" }).Should().NotBe(0); - File.Exists(migrationOutputFile).Should().BeTrue(); - File.ReadAllText(migrationOutputFile).Should().Contain("MIGRATE1018"); - } - + private void VerifyAutoInjectedDesktopReferences(string projectDirectory, string projectName, bool shouldBePresent) { if (projectName != null)