Dotnet Tool impementation by nkolev92 · Pull Request #1900 · NuGet/NuGet.Client

nkolev92 · 2017-12-22T21:41:43Z

Bug

Fixes: NuGet/Home#6199, NuGet/Home#6198, NuGet/Home#6197, NuGet/Home#6200

Regression: No
If Regression then when did it last work:
If Regression then how are we preventing it in future:

Fix

Details:
Here's the spec for the global tools implementation.
https://github.com/NuGet/Home/wiki/Global-Tools---NuGet-Implementation

The brief is, we introduce a new package type for dotnet tools, with compatible asset as "tools/tfm/rid/any.extension.

That package is only compatible with a special project type that the CLI will create, DotnetToolReference.
1 package per project, that package has to be a dotnettool.
DotnetTooReference works almost exactly the same way as Package Reference with the above restrictions in place.
Notable: Tool Restore does not generate Msbuild props and targets, for failures
No no-op artifacts for tool restore.

The core logic is in:
CompatibilityChecker
CompatibilityIssue
RestoreCommand
LockFileUtils
LockFileFormat
LockFileTargetLibrary
ManagedCodeConventions

Most relevant tests:
DotnetToolTests
ContentModelToolsTests

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation done: Manual, by Will from CLI team, automation

PS. This will go in dev when it's shifted to 15.7
//cc @wli3 @rrelyea

nkolev92 · 2018-01-03T20:40:54Z

src/NuGet.Clients/NuGet.SolutionRestoreManager/SolutionRestoreJob.cs

            }
        }

-        private bool IsProjectBuildIntegrated(PackageSpec packageSpec)


This wasn't used at all.

emgarten · 2018-01-05T23:04:12Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

      <_RestoreProjectName>$(MSBuildProjectName)</_RestoreProjectName>
-      <_RestoreProjectName Condition=" '$(RestoreProjectStyle)' == 'PackageReference' AND '$(AssemblyName)' != '' ">$(AssemblyName)</_RestoreProjectName>
-      <_RestoreProjectName Condition=" '$(RestoreProjectStyle)' == 'PackageReference' AND '$(PackageId)' != '' ">$(PackageId)</_RestoreProjectName>
+      <_RestoreProjectName Condition=" ('$(RestoreProjectStyle)' == 'PackageReference' OR '$(RestoreProjectStyle)' == 'DotnetToolReference') AND '$(AssemblyName)' != '' ">$(AssemblyName)</_RestoreProjectName>


This is used a lot, I suggest storing the value of ('$(RestoreProjectStyle)' == 'PackageReference' OR '$(RestoreProjectStyle)' == 'DotnetToolReference') as a property and re-using it for these conditions. It can be done local to this target.

emgarten · 2018-01-05T23:13:52Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+            contentItems.Load(compatibilityData.Files);
+
+
+            if (compatibilityData.TargetLibrary.PackageType.Any(e => e.Equals(PackageType.DotnetTool)))


.Contains(PackageType.DotnetTool) might be a better fit than Any

emgarten · 2018-01-05T23:16:31Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+                    new PackageIdentity(node.Key.Name, node.Key.Version));
+
+                issues.Add(issue);
+                await _log.LogAsync(GetErrorMessage(NuGetLogCode.NU1204, issue, graph));//


remove comment

emgarten · 2018-01-05T23:21:19Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+            }
+
+            if (containsDotnetToolPackageType &&
+                    !(HasCompatibleAssets(compatibilityData.TargetLibrary) || !compatibilityData.Files.Any(p => p.StartsWith("tools/", StringComparison.OrdinalIgnoreCase))))


Why allow non-tools compatible assets? Shouldn't this only look at the tools folder for a package of that type?

I'm not sure I understand this method, should there be one path for tool projects and another for everything else?

emgarten · 2018-01-05T23:25:17Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+
+            if (ProjectStyle.DotnetToolReference == compatibilityData.PackageSpec.RestoreMetadata?.ProjectStyle)
+            {
+                if (compatibilityData.PackageSpec.GetAllPackageDependencies().Any(e => e.Name.Equals(compatibilityData.TargetLibrary.Name)))


Use ignore case here

emgarten · 2018-01-05T23:27:11Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

                targetLibrary.Build.Count > 0 ||                                      // Build
-                targetLibrary.BuildMultiTargeting.Count > 0;                          // Cross targeting build
+                targetLibrary.BuildMultiTargeting.Count > 0 ||                        // Cross targeting build
+                targetLibrary.ToolsAssemblies.Count > 0;                              // Tools assemblies - This makes a package not backwards compatible potentially, but do tools assets matter


This should be separate check, a regular project should not see tool packages as compatible. I am guessing that this wouldn't be populated if it wasn't a tools project, but it would be more clear if there were a different method to check tool assets.

nkolev92 · 2018-01-09T01:33:48Z

🔔 @emgarten

// cc
@jainaashish @rohit21agrawal @mishra14

emgarten · 2018-01-09T03:53:57Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

+    </PropertyGroup>
    <!-- Determine the restore output path -->
-    <PropertyGroup Condition=" '$(RestoreProjectStyle)' == 'PackageReference' OR '$(RestoreProjectStyle)' == 'ProjectJson' ">
+    <PropertyGroup Condition=" '$(PackageReferenceCompatibleProjectStyle)' == 'true' OR '$(RestoreProjectStyle)' == 'ProjectJson' ">


emgarten · 2018-01-09T04:26:46Z

src/NuGet.Core/NuGet.ProjectModel/ProjectStyle.cs

+        /// DotnetToolReference project
        /// </summary>
-        DotnetCliTool = 3,
+        DotnetToolReference = 3,


It shouldn't matter since I don't think anyone is using this enum where there would be a mix of NuGet.*.dll versions, but I'd typically expect this to be added at the end and not change the numbers of existing values.

emgarten · 2018-01-09T07:53:23Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ManagedCodeConventions.cs

+                        },
+                    pathPatterns: new PatternDefinition[]
+                        {
+                            new PatternDefinition("tools/{tfm}/{rid}/{any}", table: DotnetAnyTable),


Use DefaultTfmAny instead of DotnetAnyTable for both. At this point dotnet is deprecated so it would cause problems to use that for netstandard.

You also want the real Any like build/ uses.

You'll need to update the tests around this also.

emgarten · 2018-01-09T07:56:17Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetToolTests.cs

+        [PlatformTheory(Platform.Windows)]
+        [InlineData("net461")]
+        [InlineData("netcoreapp1.0")]
+        public void DotnetToolTests_NoPackageReferenceToolRestore_ThrowsError(string tfm)


🏆 great tests

emgarten · 2018-01-09T07:56:31Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/MsbuilldIntegrationTestFixture.cs

 using NuGet.Packaging.PackageExtraction;
 using NuGet.Protocol;
 using Xunit;
+using NuGet.Packaging.Core;


jainaashish · 2018-01-09T18:49:58Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

     RestorePackagesPath="$(RestorePackagesPath)"
     RestoreFallbackFolders="$(RestoreFallbackFolders)"
     RestoreConfigFile="$(RestoreConfigFile)"
+     RestoreRootConfigDirectory="$(RestoreRootConfigDirectory)"


do we wanna support this RestoreRootConfigDirectory via commandLine restore as well?

What do you mean by commandline?
People can set the MSBuild property if they want to, because we can't really hide it.
It's not called in the normal nuget.exe/dotnet.exe workflows.

jainaashish · 2018-01-09T22:30:37Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

                {
-                    await logger.LogAsync(LogLevel.Verbose, string.Format(CultureInfo.CurrentCulture, Strings.Log_CheckingCompatibility, graph.Name));
+                    if (!(ProjectStyle.DotnetToolReference == project.RestoreMetadata?.ProjectStyle && string.IsNullOrEmpty(graph.RuntimeIdentifier)))
+                    { // Don't do compat checks for the ridless graph of DotnetTooReference restore. Everything relevant will be caught in the graph with the rid


nit: next line

jainaashish

Looks good to me.

nkolev92 · 2018-01-11T06:33:58Z

@emgarten
🔔

I'll do the change for nested folders separately.

Don't want to overextend this PR.

jainaashish · 2018-01-11T06:50:56Z

Just out of curiosity, how this handles local tool installation?

nkolev92 · 2018-01-11T07:00:16Z

@jainaashish

From a NuGet perspective, there is no design/implementation work on global vs local (not even sure what/how they're doing that, it's still a WIP).

There's 2 difference between global and local tools...how the tools can be invoked and where they are installed.

The global tools, are as the name suggests, global, can be invoked from anywhere, so the tool location, which is at a global location, is added to the global path.
Each global tool will be installed in a self contained location, which is controlled by what the global packages folder for the restore will be.

The local tools are just meant to be invokable in a certain context, and they'll be installed to the global packages folder of the user.

So the only thing that changes for us, is the global packages folder, which is a core scenario of ours anyways.

rohit21agrawal · 2018-01-11T21:27:51Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

  -->
  <Target Name="_GetRestoreSettingsAllFrameworks"
-    Condition=" '$(TargetFrameworks)' != '' AND '$(RestoreProjectStyle)' == 'PackageReference' "
+    Condition=" '$(TargetFrameworks)' != '' AND ( '$(RestoreProjectStyle)' == 'PackageReference' OR '$(RestoreProjectStyle)' == 'DotnetToolReference' ) "


why not use PackageReferenceCompatibleProjectStyle here?

rohit21agrawal · 2018-01-11T21:27:57Z

src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets

  -->
  <Target Name="_GetRestoreSettingsCurrentProject"
-    Condition=" '$(TargetFrameworks)' == '' AND '$(RestoreProjectStyle)' == 'PackageReference' "
+    Condition=" '$(TargetFrameworks)' == '' AND ( '$(RestoreProjectStyle)' == 'PackageReference' OR '$(RestoreProjectStyle)' == 'DotnetToolReference' ) "


PackageReferenceCompatibleProjectStyle

The scope of that property is local.

as long as the target in which it is defined is executed before this one, that property can be used. it's much easier to maintain if there is only one place to add a future PackageReference compatible project style

Refactored it.

I was more comfortable to just add it to the _GetRestoreProjectStyle returns list.

rohit21agrawal · 2018-01-11T21:29:56Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs


+            if (packageSpec.RestoreMetadata?.ProjectStyle == ProjectStyle.DotnetToolReference)
+            {
+                if (packageSpec.GetAllPackageDependencies().Count() != 1)


can this return a null list for zero package dependencies?

No, it'll return an empty set.

rohit21agrawal · 2018-01-11T21:33:31Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

                graph.Conventions.Patterns.CompileRefAssemblies,
                graph.Conventions.Patterns.RuntimeAssemblies,
-                graph.Conventions.Patterns.ContentFiles
+                graph.Conventions.Patterns.ContentFiles,


why the , in the end?

Stuff was added, then removed in a later commit, but the , got left over.

rohit21agrawal · 2018-01-11T21:37:09Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+                    group.Properties.TryGetValue(ManagedCodeConventions.PropertyNames.RuntimeIdentifier, out var ridObj);
+                    group.Properties.TryGetValue(ManagedCodeConventions.PropertyNames.TargetFrameworkMoniker, out var tfmObj);
+
+                    var tfm = tfmObj as NuGetFramework;


you are assumbing TryGetValue returned the right value for both of these, is that a safe assumption?

What do you mean by the right value?
Nulls are ok, won't cause any problems, but that'll be caught way earlier.
This is mostly for tests.
We already do the same thing on L269

rohit21agrawal · 2018-01-11T21:40:03Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+            }
+
+            if (containsDotnetToolPackageType &&
+                    !(HasCompatibleToolsAssets(compatibilityData.TargetLibrary) || !compatibilityData.Files.Any(p => p.StartsWith("tools/", StringComparison.OrdinalIgnoreCase))))


since you do a check for files fairly often, can we abstract out functions that would tell us if there are any lib files or tool files like this:

compatibilityData.HasLibFiles
compatibilityData.HasToolsFiles etc etc.

HasCompatibleToolsAssetts is a static used once in the compat checker, and once in the lock file builder.
It can't be bound to the CompatibilityData model.

rohit21agrawal · 2018-01-11T21:40:20Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

                !compatibilityData.Files.Any(p =>
                    p.StartsWith("ref/", StringComparison.OrdinalIgnoreCase)
-                    || p.StartsWith("lib/", StringComparison.OrdinalIgnoreCase));                       // No assemblies at all (for any TxM)
+                    || p.StartsWith("lib/", StringComparison.OrdinalIgnoreCase)); // No assemblies at all (for any TxM)


see : https://github.com/NuGet/NuGet.Client/pull/1900/files#r161083060

I'd rather not refactor code that doesn't need changing.
Same as above, can't tie to the model.

rohit21agrawal · 2018-01-11T21:43:12Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/CompatibilityChecker.cs

+            {
+                if (compatibilityData.PackageSpec.GetAllPackageDependencies().Any(e => e.Name.Equals(compatibilityData.TargetLibrary.Name, StringComparison.OrdinalIgnoreCase)))
+                {
+                    if (!containsDotnetToolPackageType)


change this to :
if(!containsDotnetToolPackageType && compatibilityData.PackageSpec.GetAllPackageDependencies().Any(e => e.Name.Equals(compatibilityData.TargetLibrary.Name, StringComparison.OrdinalIgnoreCase)))

this way it will not have to evaluate the more expensive check if containsDotnetToolPackageType is false.

Good catch. Not sure how I missed that simplification.

…s check for tools vs dependency packages

…otnet tool reference

emgarten

Minor comments. This looks very complete and with lots of tests great! 👏

emgarten · 2018-01-18T19:49:42Z

src/NuGet.Core/NuGet.Commands/Strings.resx

    <comment>{0} is the certificate store name</comment>
  </data>
+  <data name="Error_ProjectWithIncorrectDependenciesCount" xml:space="preserve">
+    <value>Project {0} must have exactly {1} package reference(s).</value>


nit: this could just say a single package reference, I'm not sure that writing in the number is needed.

emgarten · 2018-01-18T19:52:57Z

src/NuGet.Core/NuGet.Commands/Strings.resx

+    <value>Invalid project-package combination for {0} {1}. DotnetToolReference project style can only contain references of the DotnetTool type</value>
+  </data>
+  <data name="Error_ToolsPackageWithExtraPackageTypes" xml:space="preserve">
+    <value>Invalid tools package {0} {1}. Tools packages cannot contain more than 1 PackageType.</value>


nit: as a general rule numbers smaller than 10 should be written out as the word, not as the numeral. So: more than one PackageType here

emgarten · 2018-01-18T19:53:40Z

src/NuGet.Core/NuGet.Packaging.Core/PackageType.cs

+
+            var res = StringComparer.OrdinalIgnoreCase.Compare(Name, other.Name);
+
+            if(res != 0)


nit: spacing

emgarten · 2018-01-18T19:55:15Z

src/NuGet.Core/NuGet.Packaging.Core/PackageType.cs

+                return res;
+            }
+
+            return Version.CompareTo(other.Version);


Looks like Version could be null here and throw. Maybe the parser prevents this, but this class seems to allow it.

There's a null version check, so version can not be null.

emgarten · 2018-01-18T19:59:05Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/MsbuilldIntegrationTestFixture.cs

+                }
+                try
+                { 
+                File.Delete(targetFilePath);


nit: format the file

emgarten · 2018-01-18T20:00:04Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/ContentModelTests/ContentModelToolsTests.cs

+
+            // Assert
+            Assert.Equal(1, groups.Count);
+            Assert.Equal(NuGetFramework.AnyFramework, (NuGetFramework)groups.First().Properties["tfm"]);


🥇 good test

nkolev92 force-pushed the dev-nkolev92-globalTools branch 2 times, most recently from 1e1109e to 5284a9e Compare December 28, 2017 10:18

nkolev92 force-pushed the dev-nkolev92-globalTools branch from 333cded to 5e4a630 Compare January 3, 2018 20:30

nkolev92 commented Jan 3, 2018

View reviewed changes

nkolev92 changed the title ~~[WIP] Global tools implementation~~ Dotnet Tool impementation Jan 3, 2018

nkolev92 requested review from PatoBeltran, emgarten, jainaashish, mishra14, rohit21agrawal and zhili1208 January 3, 2018 22:53

nkolev92 force-pushed the dev-nkolev92-globalTools branch from 9fb6b9d to c0a3152 Compare January 5, 2018 22:41

emgarten suggested changes Jan 5, 2018

View reviewed changes

emgarten suggested changes Jan 9, 2018

View reviewed changes

jainaashish reviewed Jan 9, 2018

View reviewed changes

jainaashish approved these changes Jan 9, 2018

View reviewed changes

nkolev92 force-pushed the dev-nkolev92-globalTools branch from 5c3e98e to ade7829 Compare January 11, 2018 21:23

rohit21agrawal reviewed Jan 11, 2018

View reviewed changes

nkolev92 added 21 commits January 17, 2018 02:49

add an option to specify the probing path, tests

5746d79

update the compat checker to allow for more than 1 package

ed7e9f5

add tests for the platforms package

795295e

fix isNullOrEmpty condition

fb935f0

add more tests, rework compat checker

8668149

fix the error message tests

e15626c

Add a test for a package with multiple package types

c29e9bc

localize all error messages

09048b0

fix up rebase

221f20a

remove line

f1a8c11

any framework rather than dotnet for tools, separate compatible asset…

5ad6322

…s check for tools vs dependency packages

address feedback

1ba1225

refactor out the targets to do less of the if package reference and d…

6e92121

…otnet tool reference

don't run the tests for recursive

a57b72e

cleanup

ca7ebf2

address feedback

5c4e277

Don't add package type to the equals and hash code checks

6f8f664

fix compile error

08f18b7

Add file nesting

a0a8163

fix tests, add more nested values tests

c235d00

refactor the msbuild integration test fixture

90dbfd5

nkolev92 force-pushed the dev-nkolev92-globalTools branch from 6a581f6 to 90dbfd5 Compare January 17, 2018 10:57

emgarten approved these changes Jan 18, 2018

View reviewed changes

Address feedback

9af71f7

nkolev92 merged commit b82a727 into dev Jan 18, 2018

natemcmaster mentioned this pull request Jan 25, 2018

Building LZMA with DotnetTool packages fails aspnet/Universe#803

Closed

nkolev92 deleted the dev-nkolev92-globalTools branch April 3, 2018 23:24

		contentItems.Load(compatibilityData.Files);


		if (compatibilityData.TargetLibrary.PackageType.Any(e => e.Equals(PackageType.DotnetTool)))


		var res = StringComparer.OrdinalIgnoreCase.Compare(Name, other.Name);

		if(res != 0)

Conversation

nkolev92 commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Fix

Testing/Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jainaashish left a comment

Choose a reason for hiding this comment

Uh oh!

nkolev92 commented Jan 11, 2018

Uh oh!

jainaashish commented Jan 11, 2018

Uh oh!

nkolev92 commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 commented Dec 22, 2017 •

edited

Loading

nkolev92 Jan 11, 2018 •

edited

Loading