Adding support for dotnet add package command #1063

mishra14 · 2016-12-17T04:08:27Z

Fixes : NuGet/Home#3751

This PR adds a new xplat command to allow users to add package references though dotnet cli.

Objectives -

Add an add package command to allow users to add package reference into csproj files through dotnet cli as part of https://github.com/dotnet/cli/issues/4521
Edit build and test scripts to download a newer version of dotnet cli into root/cli_test to enable testing.
Add a new csproj file as a template for SimplePackageContext()
Add tests for add package command.

//cc: @emgarten @joelverhagen @rohit21agrawal @rrelyea @zhili1208 @jainaashish @alpaix @nkolev92

Nvm the number of commits, will squash and merge!

emgarten

Major changes needed:

Pass a delegate in for dotnet msbuild
Make methods async instead of calling .Result

emgarten · 2016-12-19T20:37:40Z

build/common.ps1

+
+        $installDotnet = Join-Path $CLIRootTest "dotnet-install.ps1"
+
+        wget 'https://raw.githubusercontent.com/dotnet/cli/rel/1.0.0/scripts/obtain/dotnet-install.ps1' -OutFile $installDotnet


use a url containing the commit hash, this script commonly changes

emgarten · 2016-12-19T20:42:48Z

...Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommand.cs

+                var sources = addpkg.Option(
+                    "-s|--sources",
+                    Strings.AddPkg_SourcesDescription,
+                    CommandOptionType.SingleValue);


This should match restore and take multiple values
https://github.com/dotnet/cli/blob/rel/1.0.0/src/dotnet/commands/dotnet-restore/Program.cs#L33-L36

emgarten · 2016-12-19T20:43:05Z

...Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommand.cs

+                var frameworks = addpkg.Option(
+                    "-f|--frameworks",
+                    Strings.AddPkg_FrameworksDescription,
+                    CommandOptionType.SingleValue);


Should this allow multiple?

And why is the name plural for a single value

Use the same framework parameter as build does

emgarten · 2016-12-19T20:44:43Z

...Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommand.cs

+                    CommandOptionType.SingleValue);
+
+                var packageDirectory = addpkg.Option(
+                    "--package-directory",


Use --packages
This command should match the other commands as close as possible
https://github.com/dotnet/cli/blob/rel/1.0.0/src/dotnet/commands/dotnet-restore/Program.cs#L38-L41

Thats misleading. --packages implies you want to add reference to multiple packages!

emgarten · 2016-12-19T20:45:38Z

...Core/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommand.cs

+                    var packageRefArgs = new PackageReferenceArgs(dotnetPath.Value(), projectPath.Value(), packageDependency, logger)
+                    {
+                        Frameworks = StringUtility.Split(frameworks.Value()),
+                        Sources = StringUtility.Split(sources.Value()),


Just allow multiple values instead of splitting this yourself, it is inconsistent with the other commands

emgarten · 2016-12-19T21:19:17Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

@@ -337,4 +337,80 @@ For more information, visit http://docs.nuget.org/docs/reference/command-line-re
 usage: NuGet locals &lt;all | http-cache | global-packages | temp&gt; [--clear | -c | --list | -l]
 For more information, visit http://docs.nuget.org/docs/reference/command-line-reference</value>
  </data>
+  <data name="Error_MSBuildUnableToOpenProject" xml:space="preserve">
+    <value>MSBuild was unable to open Project '{0}'.</value>


nit: lowecase p

emgarten · 2016-12-19T21:19:35Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

+    <value>Failed to locate dotnet in dotnet add pkg command.</value>
+  </data>
+  <data name="Error_FailedToCreateRandomFile" xml:space="preserve">
+    <value>Failed to create random file for dotnet add pkg command.</value>


I would not know what to do with this message as a user. Write out the path instead.

emgarten · 2016-12-19T21:21:50Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

+            var projectRootElement = TryOpenProjectRootElement(projectCSProjPath);
+            if (projectCSProjPath == null)
+            {
+                throw new Exception(string.Format(CultureInfo.CurrentCulture, Strings.Error_MSBuildUnableToOpenProject));


is the format needed here?

emgarten · 2016-12-19T21:22:18Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs

+            var projectRootElement = TryOpenProjectRootElement(projectCSProjPath);
+            if (projectCSProjPath == null)
+            {
+                throw new Exception(string.Format(CultureInfo.CurrentCulture, Strings.Error_MSBuildUnableToOpenProject));


remove format

emgarten · 2016-12-19T21:26:58Z

src/NuGet.Core/NuGet.Common/TempUtility.cs

+
+namespace NuGet.Common
+{
+    public class TempFile : IDisposable


This needs clean up, copying classes from test to production code doesn't work. This shouldn't use any sub folders under /tmp for linux to avoid permission issues

Is this used anywhere now?

rohit21agrawal · 2016-12-20T18:28:07Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

+                    .Where(t => t.Success)
+                    .Select(t => t.Graph.Framework));
+
+                if (packageReferenceArgs.Frameworks?.Any() == true)


no need for == true

packageReferenceArgs.Frameworks?.Any() is a bool? and if(bool?) is not a valid statement.

…mmand

emgarten

Overall great ✨

This needs a few small fixes and then

Check GetAllUnresolved() and handle errors
Remove unused loc strings/helper methods
TempFile doesn't look like it is used now, it might be better to avoid that change for now.

emgarten · 2016-12-21T06:55:46Z

src/NuGet.Core/NuGet.Test.Utility/SimpleTestSetup/SimpleTestProjectContext.cs

@@ -164,6 +178,41 @@ public LockFile AssetsFile
            }
        }

+        public PackageSpec PackageSpec


emgarten · 2016-12-21T06:57:49Z

src/NuGet.Core/NuGet.Common/TempUtility.cs

+
+namespace NuGet.Common
+{
+    public class TempFile : IDisposable


Is this used anywhere now?

emgarten · 2016-12-21T06:58:34Z

src/NuGet.Core/NuGet.Common/TempUtility.cs

+        {
+            if (File.Exists(_filePath))
+            {
+                File.Delete(_filePath);


This doesn't clean up the folder

emgarten · 2016-12-21T06:58:53Z

src/NuGet.Core/NuGet.Common/TempUtility.cs

+            var tempDirectory = Path.GetTempPath();
+            var randomFolderName = Guid.NewGuid().ToString();
+            var randomFileName = Guid.NewGuid().ToString();
+            Directory.CreateDirectory(Path.Combine(tempDirectory, randomFolderName));


I don't see a need for a sub directory here. Just create it in the temp root.

emgarten · 2016-12-21T07:13:55Z

src/NuGet.Core/NuGet.Common/StringUtility.cs

+
+namespace NuGet.Common
+{
+    public static class StringUtility


MSBuild helpers should not be in Common

emgarten · 2016-12-21T07:16:07Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

@@ -337,4 +337,81 @@ For more information, visit http://docs.nuget.org/docs/reference/command-line-re
 usage: NuGet locals &lt;all | http-cache | global-packages | temp&gt; [--clear | -c | --list | -l]
 For more information, visit http://docs.nuget.org/docs/reference/command-line-reference</value>
  </data>
+  <data name="Error_MSBuildUnableToOpenProject" xml:space="preserve">
+    <value>msbuild was unable to open Project '{0}'.</value>


nit: lowercase project

emgarten · 2016-12-21T07:16:29Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Strings.resx

+    <value>msbuild was unable to open Project '{0}'.</value>
+    <comment>{0} - Project/csproj path</comment>
+  </data>
+  <data name="Error_DotnetNotFound" xml:space="preserve">


Remove unused strings I see quite a few in here that are no longer used.

emgarten · 2016-12-21T08:03:14Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

+            packageReferenceArgs.Logger.LogDebug("Project Dependency Graph Read");
+
+            var projectName = dgSpec.Restore.FirstOrDefault();
+            var originalPackageSpec = dgSpec.GetProjectSpec(projectName);


this looks like it could crash if the dg spec wasn't passed in and an empty one was returned, maybe throw a helpful error message instead

emgarten · 2016-12-21T08:08:17Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

+            }
+        }
+
+        private static void LogQueue(ConcurrentQueue<string> outputs, ILogger logger)


I don't see these helper methods being used anywhere now

emgarten · 2016-12-21T08:21:44Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

+
+            // 2. Run Restore Preview
+            packageReferenceArgs.Logger.LogDebug("Running Restore preview");
+            var restorePreviewResult = await PreviewAddPackageReference(packageReferenceArgs,


checking Success wont work here, but you should verify GetAllUnresolved() == 0

It is possible the packages were missing or couldn't be downloaded. If that happens they won't show in the compat check since there was nothing to verify. Later it will null ref when they are missing from the graph.

mishra14 · 2016-12-21T22:23:06Z

merged : dd0f0aa

Ankit Mishra added 30 commits December 8, 2016 12:17

Added msbuild apis and resolved restore errors

ecba454

using localizable strings

a3a5a18

Restore preview functional; msbuild apis is breaking

a259d84

Adding logs through the add ref process

0e0cdb4

Improving msbuild dependency

d570946

Adding fx logic - not complete yet

f0b3892

Added logic for conditional tfm

e149089

Fixed the bug for unconditional adds

871be16

Adding comments to public methods in msbuildutility

a401047

Changed AddPackageReference to Add or Update

cecbe75

1. Added --no-restore; 2. Added --frameworks;

93f117d

Added string resources

2deb5b2

Added some test setup

ca3a544

Adding code to fetch cli test

bbf6345

Adding a test for arg parsing

3021312

Added comprehensive arg testing

c53afb1

refactored temp file

1f794ad

Refactored code to use PackageDependency, Sources and packagedirectory

2618056

Fixed arg parsing tests

5067c6f

Added test code and infrastructure for testing functional logic

44be353

Fixing dotnet install on shell script

63a65fd

Fixing build.sh to download cli_test for nix

1c66b02

Fixing shell script for installing cli test into the right directory

25465a2

Fixing msbuild version

722a3c7

Fixing moq dependency

95e21de

checking for ci status on tests

787e3cf

Removing arg parsing test

0ab6b27

1. CLosed project in msbuild; 2. Added some more tests

c61ae57

Added tests upto add as update

f3b8450

Added subsequent add tests

9e3771e

Ankit Mishra added 7 commits December 16, 2016 15:40

Refactored tests

b6d475d

Fixing commandline.utils dependency

178d9a9

Cleaned up strings

ebb75ba

Added code to handle when user does not pass version

35c133c

Added arg parsing tests again

1ca6e1f

Improved arg absence exception and added test case for package directory

6f8b2cd

PR clean up

284fe47

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

mishra14 closed this Dec 17, 2016

mishra14 reopened this Dec 17, 2016

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

Fixed casing in package directory assert

2e3d607

emgarten suggested changes Dec 19, 2016

View reviewed changes

Ankit Mishra added 3 commits December 19, 2016 19:02

Partially addressing PR comments

9880749

Fixed the dotnet cli download link

7fda14d

refactored tempfile class out of msbuildutility

cc017ea

rohit21agrawal reviewed Dec 20, 2016

View reviewed changes

Ankit Mishra added 3 commits December 20, 2016 16:06

Removed dgspec generation, to be moved into dotnet cli add package co…

4442c88

…mmand

Fixing tests to allow passing a dg file to add package command runner

70d6d10

Removinf string.format

7933108

emgarten approved these changes Dec 21, 2016

View reviewed changes

Final cleanup berfore merging based on PR

9eff579

mishra14 mentioned this pull request Dec 21, 2016

Adding dotnet add package sub command dotnet/cli#5122

Merged

mishra14 closed this Dec 21, 2016

mishra14 deleted the dev-anmishr-ref branch December 21, 2016 23:07

mishra14 restored the dev-anmishr-ref branch December 22, 2016 20:40

mishra14 deleted the dev-anmishr-ref branch January 5, 2017 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for dotnet add package command #1063

Adding support for dotnet add package command #1063

mishra14 commented Dec 17, 2016 •

edited

Loading

emgarten left a comment

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

mishra14 Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 19, 2016

emgarten Dec 21, 2016

rohit21agrawal Dec 20, 2016

mishra14 Dec 20, 2016 •

edited

Loading

emgarten left a comment

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

emgarten Dec 21, 2016

mishra14 commented Dec 21, 2016


		$installDotnet = Join-Path $CLIRootTest "dotnet-install.ps1"

		wget 'https://raw.githubusercontent.com/dotnet/cli/rel/1.0.0/scripts/obtain/dotnet-install.ps1' -OutFile $installDotnet

Adding support for dotnet add package command #1063

Adding support for dotnet add package command #1063

Conversation

mishra14 commented Dec 17, 2016 • edited Loading

emgarten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishra14 Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

emgarten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishra14 commented Dec 21, 2016

mishra14 commented Dec 17, 2016 •

edited

Loading

mishra14 Dec 20, 2016 •

edited

Loading