Fix 4521: Add dotnet-add-p2p #4758

krwq · 2016-11-17T00:53:40Z

https://github.com/dotnet/cli/issues/4521

TheRealPiotrP

Great start! I see it's a WiP so much of the commentary is likely already planned to be addressed. Key points:

I think we can simplify the predicates approach
I don't see why we have another parser. We're not saving much [5%, maybe] on LoC and yet we're paying for YAP(tm) [Yet another parser].
Need more test coverage

TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/App/App.csproj

+  </PropertyGroup>
+
+  <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
+</Project>


src/dotnet/Program.cs

@@ -24,6 +24,7 @@
 using Microsoft.DotNet.Tools.Test;
 using Microsoft.DotNet.Tools.VSTest;
 using NuGet.Frameworks;
+using Microsoft.DotNet.Tools.Add;


src/dotnet/Program.cs

@@ -45,6 +46,7 @@ public class Program
            ["run"] = RunCommand.Run,
            ["test"] = TestCommand.Run,
            ["vstest"] = VSTestCommand.Run,
+            ["add"] = AddCommand.Run,


src/dotnet/commands/dotnet-add-p2p/Program.cs

+                FullName = ".NET Add Project to Project (p2p) reference Command",
+                Description = "Command to add project to project (p2p) reference"
+            };
+            app.HelpOption("-h|--help");


src/dotnet/commands/dotnet-add-p2p/Program.cs

+            app.HelpOption("-h|--help");
+
+            CommandArgument projectArgument = app.Argument("<PROJECT>",
+                "The MSBuild project file to modify. If a project file is not specified," +


test/dotnet-add-p2p.Tests/GivenDotnetAddP2P.cs

+            new AddP2PCommand()
+                    .WithWorkingDirectory(testRoot)
+                    .Execute($"{Lib2Path} -r {Lib4Path} -f net451")
+                .Should().Pass();


test/dotnet-add-p2p.Tests/GivenDotnetAddP2P.cs

+            new AddP2PCommand()
+                    .WithWorkingDirectory(testRoot)
+                    .Execute($"{Lib2Path} -r {Lib4Path} -f netcoreapp1.0")
+                .Should().Pass();


test/dotnet-add-p2p.Tests/GivenDotnetAddP2P.cs

+            string lib2FullPath = Path.Combine(testRoot, Lib2Path);
+            CountOccurrances(File.ReadAllText(lib2FullPath), "Lib4.csproj").Should().Be(2);
+
+            new RestoreCommand()


test/dotnet-add-p2p.Tests/GivenDotnetAddP2P.cs

+            return ret;
+        }
+    }
+}


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/App/Program.cs

+#endif
+        }
+    }
+}


TheRealPiotrP · 2016-11-18T19:44:34Z

Any updates on this one?

TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/EmptyItemGroup/EmptyItemGroup.csproj

+  <ItemGroup>
+    <Compile Include="**\*.cs" />
+    <EmbeddedResource Include="**\*.resx" />
+    <PackageReference Include="Microsoft.NET.Sdk">


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/Lib/Lib.csproj

+  <ItemGroup>
+    <Compile Include="**\*.cs" />
+    <EmbeddedResource Include="**\*.resx" />
+    <PackageReference Include="Microsoft.NET.Sdk">


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/MoreThanOne/SomeSourceFile.cs

@@ -0,0 +1,8 @@
+using System;
+
+namespace SomeNS


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/MoreThanOne/a.csproj

+  <ItemGroup>
+    <Compile Include="**\*.cs" />
+    <EmbeddedResource Include="**\*.resx" />
+    <PackageReference Include="Microsoft.NET.Sdk">


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/MoreThanOne/b.csproj

+  <ItemGroup>
+    <Compile Include="**\*.cs" />
+    <EmbeddedResource Include="**\*.resx" />
+    <PackageReference Include="Microsoft.NET.Sdk">


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/MoreThanOne/b.csproj

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/MoreThanOne/a.csproj

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/Lib/Lib.csproj

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/EmptyItemGroup/EmptyItemGroup.csproj

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/Broken/Broken.csproj

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/ValidRef/ValidRef.csproj

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/ValidRef/ValidRef.csproj

+  <ItemGroup>
+    <Compile Include="**\*.cs" />
+    <EmbeddedResource Include="**\*.resx" />
+    <PackageReference Include="Microsoft.NET.Sdk">


...TestProjects/DotnetAddP2PProjects/WithExistingRefCondOnItem/WithExistingRefCondOnItem.csproj

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="utf-8"?>


...ts/DotnetAddP2PProjects/WithExistingRefCondWhitespaces/WithExistingRefCondWhitespaces.csproj

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="utf-8"?>


...RestoredTestProjects/DotnetAddP2PProjects/WithRefCondNonUniform/WithRefCondNonUniform.csproj

@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="utf-8"?>


...oredTestProjects/DotnetAddP2PProjects/WithRefNoCondNonUniform/WithRefNoCondNonUniform.csproj

@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/_remove_me_/AppE2E/App.csproj

@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/_remove_me_/AppE2E/App.csproj

+  <ItemGroup Condition="'$(TargetFramework)' == 'net451'">
+    <!-- This is intentionally empty - do not remove -->
+  </ItemGroup>
+  <PropertyGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/_remove_me_/AppE2E/App.csproj

+  <PropertyGroup Condition=" '$(TargetFramework)' == 'netcoreapp1.0' ">
+    <DefineConstants>$(DefineConstants);NETCOREAPP1_0</DefineConstants>
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(TargetFramework)' == 'net451' ">


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/_remove_me_/Lib1/Lib1.csproj

@@ -0,0 +1,28 @@
+<?xml version="1.0" encoding="utf-8"?>


TestAssets/NonRestoredTestProjects/DotnetAddP2PProjects/_remove_me_/Lib4/Lib4.csproj

@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="utf-8"?>


src/Microsoft.DotNet.ProjectJsonMigration/MSBuildExtensions.cs

@@ -27,14 +27,18 @@ public static bool IsEquivalentTo(this ProjectItemElement item, ProjectItemEleme
            // Different includes
            if (item.IntersectIncludes(otherItem).Count() != item.Includes().Count())
            {
+#if !DISABLE_TRACE


src/Microsoft.DotNet.ProjectJsonMigration/MSBuildExtensions.cs

                return false;
            }

            // Different Excludes
            if (item.IntersectExcludes(otherItem).Count() != item.Excludes().Count())
            {
+#if !DISABLE_TRACE


src/dotnet/commands/dotnet-add-p2p/Program.cs

+
+            if (files.Length > 1)
+            {
+                throw new GracefulException("Found more than one MSBuild project in the current directory. Please specify which one to use.");


test/dotnet-add-p2p.Tests/GivenDotnetAddP2P.cs

+        }
+
+        [Fact]
+


livarcocc · 2016-11-23T18:39:23Z

@dotnet-bot Test this please

added a lot of changes, need to be reviewed again

krwq · 2016-11-23T19:42:57Z

Seems like I got some non-windows issues, I'll investigate this

src/dotnet/Strings.cs

+
+namespace Microsoft.DotNet.Tools
+{
+    internal static class Strings


src/dotnet/Strings.cs

+{
+    internal static class Strings
+    {
+        // Arguments parsing


src/dotnet/commands/dotnet-add-p2p/Program.cs

+                CommandOptionType.NoValue);
+
+            app.OnExecute(() => {
+                if (string.IsNullOrEmpty(projectArgument.Value))


src/dotnet/commands/dotnet-add-p2p/Program.cs

+        }
+
+        // There is ProjectRootElement.TryOpen but it does not work as expected
+        // I.e. it returns null for some valid projects


test/Microsoft.DotNet.Tools.Tests.Utilities/Commands/AddP2PCommand.cs

+        public AddP2PCommand()
+            : base("dotnet")
+        {
+


test/dotnet-add-p2p.Tests/Extensions.cs

+
+namespace Microsoft.DotNet.Cli.Add.P2P.Tests
+{
+    internal static class Extensions


test/dotnet-add-p2p.Tests/Extensions.cs

+            return root.ItemGroups.Count((ig) => string.IsNullOrEmpty(ig.Condition));
+        }
+
+        public static IEnumerable<ProjectElement> ItemsWithIncludeAndConditionContaining(this ProjectRootElement root, string itemType, string includePattern, string patternInCondition)


test/dotnet-add-p2p.Tests/Extensions.cs

+            });
+        }
+
+        public static int NumberOfProjectReferencesWithIncludeAndConditionContaining(this ProjectRootElement root, string includePattern, string patternInCondition)


test/dotnet-add-p2p.Tests/Extensions.cs

+            return root.ItemsWithIncludeAndConditionContaining("ProjectReference", includePattern, patternInCondition).Count();
+        }
+
+        public static IEnumerable<ProjectElement> ItemsWithIncludeContaining(this ProjectRootElement root, string itemType, string includePattern)


…, fix long lines, remove empty line

TheRealPiotrP · 2016-11-28T21:28:07Z

TheRealPiotrP · 2016-11-28T21:28:17Z

@srivatsn @MattGertz please approve

MattGertz · 2016-11-28T21:46:45Z

Your wish is my command... :-)

dnfclas added the cla-already-signed label Nov 17, 2016

TheRealPiotrP previously requested changes Nov 17, 2016

View reviewed changes

rainersigwald mentioned this pull request Nov 18, 2016

AST for conditions in the OM dotnet/msbuild#1356

Closed

krwq force-pushed the i4521 branch from 98db1f4 to ba6bf01 Compare November 18, 2016 23:23