New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tools] Integrate NuGet #8414

Merged
merged 16 commits into from Jan 20, 2018

Conversation

Projects
None yet
7 participants
@wli3
Copy link
Collaborator

wli3 commented Jan 17, 2018

Please review this change by commit.

The CI won't pass since I am pointing to a private NuGet build

The only WIP is to insert correct NuGet version after NuGet/NuGet.Client#1900 is merged. I hope to get this PR approved and merge it once NuGet is available. So I didn't make separate PRs.

It has

  • Update NuGet version. Rely on NuGet to filter TFM. And use asset.json to find entrypoint

  • Update XML file to per TFM

  • Add extra property to the fake project according to nuget

  • Treat nuget fallback folder as offline cache for tool

  • Require -g to install global tool #8395

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 17, 2018

@wli3 wli3 requested a review from dotnet/dotnet-cli Jan 17, 2018

</trans-unit>
<trans-unit id="InstallToolCommandOnlySupportGlobal">
<source>Install tool only support install user wide (-g).</source>
<target state="new">Install tool only support install user wide (-g).</target>

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

@KathleenDollard this need your review. This is the message when the consumer forget to add -g to dotnet install tool my.tool. Do you think it is clear enough to guide them adding -g ?

This comment has been minimized.

@KathleenDollard

KathleenDollard Jan 17, 2018

We'll check this at standup. I anticipate this is temporary as I think we're still planning session tools after Preview 1.

The --global switch (-g) is currently required because only user wide tools supported.

@peterhuene can you look at that as well

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

@wli3's PR properly checks for the presence of the global flag and aborts if not present.

Agree with @KathleenDollard's suggestion of making it clear that this is "currently required" because only user-wide is supported. As a user encountering a message like that, I would think "oh okay, perhaps one day the -g option won't be always required".

I'd just amend the suggestion to: The --global switch (-g) is currently required because only user wide tools are supported. (added are).

This comment has been minimized.

@KathleenDollard

KathleenDollard Jan 17, 2018

Yes, agree with Peter

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed, change to above text

@wli3 wli3 referenced this pull request Jan 17, 2018

Closed

Update to global tools latest nupkg format #383

0 of 2 tasks complete

internal static bool MatchesFile(string pathInLockFile, string entryPoint)
{
_pathInLockFilePathInArray = SplitPathByDirectorySeparator(pathInLockFile);

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Why the use of static fields here instead of locals? This prevents this method from being reentrant.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Please see if the local function + strong type input + comments is good enough

return true;
}

private static bool EntryPointHasFileAtLeast()

This comment has been minimized.

@livarcocc

livarcocc Jan 17, 2018

Member

This method name sounds weird. EntryPointHasFile seems enough to me.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Please see if the local function + strong type input + comments is good enough

{
internal class LockFileMatcher
{
private static string[] _pathInLockFilePathInArray;

This comment has been minimized.

@livarcocc

livarcocc Jan 17, 2018

Member

Why is this whole class static?

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

@peterhuene including why static field. It simply look nicer in linq. Where(p => LockFileMatcher.MatchesFile(p, entry)). I want to test it separately, and it is not very related to ToolPackageObtainer.cs, so it is a separate class. I don't want to pass _pathInLockFilePathInArray and _entryPointPathInArray all over the place. So I used field.

What do you think making it a non static class? Where(p => new LockFileMatcher(p, entry).MatchesFile()) another minor downside is, there will be extra allocation.

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

I think static class is fine; I just don't like making the one public method non-reentrant for argument passing convenience (i.e. using static fields).

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

I agree that you shouldn't use static state to keep track of the internal execution state of a method.

What I would do is move the static fields inside the method, and move the private methods inside the internal method as local functions.

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

I see, I think make it non static class can satisfy all

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

To be clear, I do think this should remain a static class; making these instance fields does not address the reentrancy problem.

This comment has been minimized.

@livarcocc

livarcocc Jan 17, 2018

Member

It seems to me like both the fields could simply be passed as parameters to the private static methods, no?

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

This is my plan, it is good?

internal class LockFileMatcher
    {
        private readonly string[] _pathInLockFilePathInArray;
        private readonly string[] _entryPointPathInArray;

        public LockFileMatcher(LockFileItem lockFileItem, string targetRelativeFilePath)
        {
            _pathInLockFilePathInArray = SplitPathByDirectorySeparator(lockFileItem.Path);
            _entryPointPathInArray = SplitPathByDirectorySeparator(targetRelativeFilePath);
        }

        internal bool MatchesFile()
        {
            return EntryPointHasFileAtLeast()
                   && PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories()
                   && RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories();
        }

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Or as Livar suggested, make it static method, and passing pathInLockFilePathInArray entryPointPathInArray down to PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories and RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories.

This comment has been minimized.

@dsplaisted

dsplaisted Jan 18, 2018

Member

I would just do it as a static method with local functions. Something like this:

internal class LockFileMatcher
{
    internal static bool MatchesFile(string pathInLockFile, string entryPoint)
    {
        string[] pathInLockFilePathInArray = SplitPathByDirectorySeparator(pathInLockFile);
        string[] entryPointPathInArray = SplitPathByDirectorySeparator(entryPoint);

        return EntryPointHasFileAtLeast()
                && PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories()
                && RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories();

        //  Local functions
        bool RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories()
        {
            string[] pathAfterToolsTfmRid = pathInLockFilePathInArray.Skip(3).ToArray();
            return !pathAfterToolsTfmRid
                .Where((directoryOnEveryLevel, i) => directoryOnEveryLevel != entryPointPathInArray[i])
                .Any();
        }

        bool PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories()
        {
            if (pathInLockFilePathInArray.Length - entryPointPathInArray.Length != 3)
            {
                return false;
            }

            if (pathInLockFilePathInArray[0] != "tools")
            {
                return false;
            }

            return true;
        }

        bool EntryPointHasFileAtLeast()
        {
            return entryPointPathInArray.Length >= 1;
        }

        string[] SplitPathByDirectorySeparator(string path)
        {
            return path.Split('\\', '/');
        }
    }    
}
.Any();
}

private static bool PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories()

This comment has been minimized.

@livarcocc

livarcocc Jan 17, 2018

Member

This method name has a typo in it. Excatly instead of Exactly.

Also, it is not really doing what it claims to be doing. It says it will check for ToolsTfmRid and it only checks that it has three directories there and that it starts with tools.

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

TFM and RID is checked by NuGet already, since it is in asset.json. Maybe a good place to add comment

This comment has been minimized.

@livarcocc

livarcocc Jan 18, 2018

Member

If TFM and RID are already being checked, than don't mention it in this method's name? Seems more confusing to do so, then when someone looks at the implementation, than it does not match the method name.

&& RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories();
}

private static bool RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories()

This comment has been minimized.

@livarcocc

livarcocc Jan 17, 2018

Member

This is also a very hard name to follow.

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

I want to say"/tfm/rid/my/tool.dll" matches "my/tool.dll", what about

SubPathMatchesTargetFilePath ?

&& RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories();
}

private static bool RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories()

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Nit: I think these method names are a bit too descriptive (I know they're private, but still); they read like test names. I'd hate to have to type these out without IntelliSense.

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

nit: this will be a philosophical debate. But I think method name length should be low priority here. Extreme case:
objective c naming. And most editor has auto complete on words (by simply scan the existing doc and find words). Most "starter-pack" of vim and emacs will include such function as well.

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Agreed that naming can be a complete bikeshedding exercise (not to mention that good naming is difficult). I simply prefer concise names that convey enough meaning; they don't need to be fully self-describing as that's what either documentation or the code itself is for.

This comment has been minimized.

@dsplaisted

dsplaisted Jan 18, 2018

Member

To me even the long name doesn't really help me understand what the method does. So more than changing the name I would add comments to explain it.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Please see if the local function + strong type input + comments is good enough


private static bool EntryPointHasFileAtLeast()
{
return _entryPointPathInArray.Length >= 1;

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Do we really need a method for checking if _entryPointPathInArray is empty?

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator
return EntryPointHasFileAtLeast()
                  && PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories()
                   && RestMatchsOtherThanPathInLockFileHasToolsTfmRidAsParentDirectories();

If we do not check it there, PathInLockFileHasExcatlyToolsTfmRidAsParentDirectories will throw. This 3 check is more and more specific

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Sorry, I didn't mean to imply that the check itself is unneeded, only refactoring the check out into its own method when it is only checked from exactly one place. When reading the code, when a method call like this one is introduced I now have to stop and jump to the method's definition to see what it does (granted, this is a very tiny source file, but imagine a lot more code in between these methods). It would be far more concise to simply do a check for empty where needed.

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

I think Peter is not saying don't check it, he's just saying it doesn't seem worth it to factor the length check out into a separate method.

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

will in line this method

@@ -5,17 +5,18 @@

namespace Microsoft.DotNet.ToolPackage
{
internal class ToolConfigurationAndExecutableDirectory
internal class ToolConfigurationAndExecutablePath

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Nit: I wonder if this type could simply be replaced with a value tuple in the future (note: not important for this PR).


if (dotnetToolSettings == null)
{
throw new PackageObtainException("Cannot find DotnetToolSettings");

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Will it be possible to install a tool with a missing settings file (despite a package type check)? If so, I think this message should be localized and a little bit friendlier, something along the lines of Package '{packageId}' is missing tool settings file DotnetToolSettings.xml.

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

Will it be possible to install a tool with a missing settings file (despite a package type check)?

Yes, NuGet will not check the setting file.

And yes, I am missing loc. And your error message is better

@@ -172,4 +172,10 @@ The error was:
<data name="InstallToolVersionDefinition" xml:space="preserve">
<value>Version of the package in NuGet</value>
</data>
<data name="GlobalOptionDescription" xml:space="preserve">
<value>Install user wide.</value>

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

Unless I was a user coming from node.js, I'm going to find this option perplexing. Installing "globally" means to install "user wide"? Given that we have no "local" install to default to, a required boolean option strikes me as bad UX. I get that we're trying to future-proof a default of local install (is that something that even makes sense for tools? it does for npm which also manages runtime package dependencies), but this seems premature to me.

This comment has been minimized.

@wli3

This comment has been minimized.

@KathleenDollard

KathleenDollard Jan 17, 2018

@peterhuene

Yes, --user would be more correct than --global, but...

It's not coming from node.js, it's coming from npm experience, which happens to be deployed by node. I make that distinction because everybody (even us - Azure CLI as example) has the install node to run npm experience. So paralleling that is important.

We plan to have a "session" based tool that will be the default. I anticipate that in this release, but I don't think Preview 1. That will install into the user's session.

"Local" doesn't make sense to me. There is my whole experience (global), this session (planned) and this repo (in future thinking, not designed, not this version).

If global was default, we could not change later.

So, yes, this is odd, but I think correct.

This comment has been minimized.

@peterhuene

peterhuene Jan 17, 2018

Member

I totally get what you're saying, but I don't think I share the sentiment that we need to be consistent with a package manager from another ecosystem, even if we've been instructing users to use it for our own tools prior to this feature. If it truly makes sense for dotnet for the global option to be non-default, then I'm all for this.

@peterhuene

This comment has been minimized.

Copy link
Member

peterhuene commented Jan 17, 2018

Other than my comments above, the rest of the changes are 👍

@dsplaisted
Copy link
Member

dsplaisted left a comment

I've reviewed the first commit.

ExtraTestFeedPackages/microsoft.netcore.platforms.2.0.1.nupkg

Is there a way to avoid checking in a .nupkg here?

@@ -131,6 +131,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<BundledNETCoreAppPackageVersion>$(_NETCoreAppPackageVersion)</BundledNETCoreAppPackageVersion>
<BundledNETStandardTargetFrameworkVersion>$(_NETStandardTargetFrameworkVersion)</BundledNETStandardTargetFrameworkVersion>
<BundledNETStandardPackageVersion>$(_NETStandardPackageVersion)</BundledNETStandardPackageVersion>
<NETCorePlatformsImplicitPackageVersion>$(MicrosoftNETCoreAppPackageVersion)</NETCorePlatformsImplicitPackageVersion>

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

I don't think the Platforms package will always have the same version number as the NETCoreApp package. Did you verify whether this will be the case with the runtime team?

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

I did test that and it works. But this property is the version number of runtime right?

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

This is a problem. I think we should remove this property in CLI. And we should get this property in SDK derived from Microsoft.NETCore.App's dependency. During Pack, Microsoft.NETCore.App is required. If not (in some case), default fo 2.0.3
image

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

PackageDefinitions won't work, This need to be added before PackageDefinitions is computed

This comment has been minimized.

@nguerrera

nguerrera Jan 18, 2018

Member

We are already getting the NETStandard.Library 2.0.1 from PackageDefinitions here in this file. Why does this need to be computed before PackageDefinitions in this file?

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

"computed before PackageDefinitions" in during pack, ideally this should computed according to consumer project's runtime. Instead of predefined in CLI. the problem is if the user set RuntimeFrameworkVersion, and we will add the package version according to the runtime CLI carries. If RuntimeFrameworkVersion > CLI's runtime, there will be a package downgrade.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Talked to Nick. For preview 1, we should get the version from PackageDefinitions and set it in build/MSBuildExtensions.targets.

After that, we need to design whether we want to keep Microsoft.NETCore.Platforms on Pack side or install side (add the reference in fake project). Or maybe we can ask NuGet for pack target "add extra dependency" feature.

Also, the property name need to change. This require change on both SDK and CLI, I will send PR once this is merged. #8421

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Changed as discussed above

{
internal class LockFileMatcher
{
private static string[] _pathInLockFilePathInArray;

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

I agree that you shouldn't use static state to keep track of the internal execution state of a method.

What I would do is move the static fields inside the method, and move the private methods inside the internal method as local functions.


private static bool EntryPointHasFileAtLeast()
{
return _entryPointPathInArray.Length >= 1;

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

I think Peter is not saying don't check it, he's just saying it doesn't seem worth it to factor the length check out into a separate method.

private static string[] _pathInLockFilePathInArray;
private static string[] _entryPointPathInArray;

internal static bool MatchesFile(string pathInLockFile, string entryPoint)

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

From just reading the code without knowing more context, this class seems confusing. MatchesFile is a pretty generic name and doesn't help me understand the code.

I would recommend adding comments describing what the MatchesFile method does, and the context in which it does it.

.WithFile(entryPointFromLockFile.Path));
}

private static LockFileItem FindAssetInLockFile(LockFile lockFile, string toolConfigurationToolAssemblyEntryPoint, string packageId)

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

toolConfigurationToolAssemblyEntryPoint seems like the wrong name here since you're passing "DotnetToolSettings.xml" as the argument value in one instance.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed

ToolConfiguration toolConfiguration =
ToolConfigurationDeserializer.Deserialize(toolConfigurationPath.Value);

var entryPointFromLockFile = FindAssetInLockFile(lockFile, toolConfiguration.ToolAssemblyEntryPoint, packageId);

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

Is there a reason we need to specifically look for the entry point in the lockfile? Could we just combine the path of the directory the settings file is in with the relative path to the entry point in that settings file?

This comment has been minimized.

@wli3

wli3 Jan 17, 2018

Collaborator

what in asset json is "tools/tfm/rid/my/tools.dll" what we need is "tools/tfm/rid" in your case. So, we need to match tfm and rid pattern to get that, it is harder when there is subfolder. I think it is easier to match entrypoint directly.


if (entryPointFromLockFile == null)
{
throw new PackageObtainException("Cannot find tool entry point the package");

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

Error messages need to be localized and more user friendly (ie specify that this is probably a package authoring error, and that the entry point {0} specified in {toolsettingspath} could not be found).

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed

{

[Fact]
public void MatchesEntryPointTests()

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

I'd suggest making this a Theory instead of testing multiple things in one Fact. As it stands, if the first assertion fails, you'll get no information about the subsequent ones.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed, to theory


var assetJsonPath = toolConfigurationAndExecutablePath
.Executable
.GetDirectoryPath()
.GetParentPath()

This comment has been minimized.

@dsplaisted

dsplaisted Jan 17, 2018

Member

If you're going five levels up the directory tree, I'd suggest adding comments noting what each level is.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed, added comment

@@ -154,8 +176,15 @@ IProjectRestorer projectRestorer
new XElement("PropertyGroup",
new XElement("TargetFramework", targetframework),
new XElement("RestorePackagesPath", individualToolVersion.Value),
new XElement("RestoreSolutionDirectory", Directory.GetCurrentDirectory()), // https://github.com/NuGet/Home/issues/6199
new XElement("DisableImplicitFrameworkReferences", "true")
new XElement("RestoreProjectStyle", "DotnetToolReference"),

This comment has been minimized.

@nkolev92

nkolev92 Jan 17, 2018

Contributor

It was looked over in the design doc you've been usuing as a reference.
How does the runtimeidentifier get set?

shellShimMaker.EnsureCommandNameUniqueness(commandName);

shellShimMaker.CreateShim(
executable.Value,
toolConfigurationAndExecutablePath.Executable.Value,

This comment has been minimized.

@livarcocc

livarcocc Jan 18, 2018

Member

NIT: Should ToolConfigurationAndExecutablePath have a ExecutablePath property, instead of this ugly Executable.Value? I think it would read better.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

CreateShim should take in a strong type FilePath. However this will conflict with Nate's work. So I will change it later.

@@ -18,9 +18,12 @@

<NugetConfigCLIFeeds>
<![CDATA[
<add key="localtestfeed" value="C:\work\testfeed" />

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Will revert the whole file before merge

@dsplaisted
Copy link
Member

dsplaisted left a comment

I've finished reviewing the rest of the commits.

@@ -172,7 +174,11 @@ private static LockFileItem FindAssetInLockFile(LockFile lockFile, string toolCo
new XElement("RestorePackagesPath", individualToolVersion.Value),
new XElement("RestoreProjectStyle", "DotnetToolReference"),
new XElement("RestoreRootConfigDirectory", Directory.GetCurrentDirectory()),
new XElement("DisableImplicitFrameworkReferences", "true")
new XElement("DisableImplicitFrameworkReferences", "true"),

This comment has been minimized.

@dsplaisted

dsplaisted Jan 18, 2018

Member

Can you add comments explaining why these properties are set to these values?

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed, added comment

@@ -52,5 +57,7 @@
</MSBuild>
<MSBuild BuildInParallel="False" Projects="SampleGlobalTool/consoledemo.csproj" Targets="pack" Properties="Configuration=Release;NuspecFile=includepublish.nuspec;NuspecBasePath=$(testAssetSourceRoot);PackageOutputPath=$(OutputPath)/TestAssetLocalNugetFeed">
</MSBuild>
<Copy SourceFiles="ExtraTestFeedPackages/microsoft.netcore.platforms.2.0.1.nupkg" DestinationFolder="$(OutputPath)/TestAssetLocalNugetFeed" />

This comment has been minimized.

@dsplaisted

dsplaisted Jan 18, 2018

Member

Instead of explicitly copying this here, I'd suggest adding the file to the Content item.

This comment has been minimized.

@wli3

wli3 Jan 18, 2018

Collaborator

Fixed, use Content

@dsplaisted
Copy link
Member

dsplaisted left a comment

Is there any way we can avoid checking in a .nupkg file (microsoft.netcore.platforms.2.0.1.nupkg)?

@@ -99,6 +99,13 @@ public void GivenNugetConfigAndPackageNameAndVersionAndTargetFrameworkWhenCallIt
nugetconfig: nugetConfigPath,
targetframework: _testTargetframework);

/*
From mytool.dll to project.assets.json
.dotnet/.tools/packageid/version/packageid/version/mytool.dll

This comment has been minimized.

@dsplaisted

dsplaisted Jan 18, 2018

Member

Shouldn't the path also include tools/tfm/rid?

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 18, 2018

@dsplaisted

Is there any way we can avoid checking in a .nupkg file (microsoft.netcore.platforms.2.0.1.nupkg)?

when testing --source, the local feed need to have all the packages. Do you think it is ok to download this package from myget during build? Since some of tests have myget as feed and it eventually downloads it from the internet

@dsplaisted

This comment has been minimized.

Copy link
Member

dsplaisted commented Jan 18, 2018

@wli3 Yes, I would try to download the package as part of the build or test before it tests the --source option.

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 18, 2018

@dsplaisted changed to download package during build

@wli3 wli3 force-pushed the wli3:integrate-nuget-ask-rebase branch from 8c57613 to 935ccb9 Jan 19, 2018

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 19, 2018

Rebased including shim change. Find a bug in manual test, DisableImplicitNuGetFallbackFolder need to set in temp project.

@wli3 wli3 force-pushed the wli3:integrate-nuget-ask-rebase branch from 935ccb9 to c795235 Jan 19, 2018

@wli3 wli3 changed the title WIP [tools] Integrate NuGet [tools] Integrate NuGet Jan 19, 2018

@@ -4,6 +4,7 @@
using Microsoft.DotNet.Cli.CommandLine;
using LocalizableStrings = Microsoft.DotNet.Tools.Install.LocalizableStrings;


This comment has been minimized.

@peterhuene

peterhuene Jan 19, 2018

Member

Looks like the change to InstallCommandParser.cs can be reverted as it's just (inadvertent?) addition of whitespace.

@peterhuene
Copy link
Member

peterhuene left a comment

Other than the comment about reverting the whitespace (newline only) change to a InstallCommandParser.cs, LGTM.

@peterhuene

This comment has been minimized.

Copy link
Member

peterhuene commented Jan 19, 2018

Should we squash the PR?

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 19, 2018

Should we squash the PR?

Yes, I retry to make it nicer use rebase -i, but there is too much xlf merge conflict.

@wli3 wli3 force-pushed the wli3:integrate-nuget-ask-rebase branch 2 times, most recently from cc0ccd4 to 77f99c6 Jan 19, 2018

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 19, 2018

Updated test asset's platforms package version to a preview version that has alpine support

@wli3 wli3 force-pushed the wli3:integrate-nuget-ask-rebase branch from 77f99c6 to 46da4bc Jan 19, 2018

@wli3 wli3 merged commit 02a98d4 into dotnet:master Jan 20, 2018

15 checks passed

Alpine3.6 x64 Debug Build Build finished.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Debug Build Build finished.
Details
Debian8.2 x64 Debug Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
OSX10.12 x64 Release Build Build finished.
Details
RHEL6 x64 Debug Build Build finished.
Details
RHEL7.2 x64 Release Build Build finished.
Details
Ubuntu x64 Release Build Build finished.
Details
Ubuntu16.04 x64 Debug Build Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Release Build Build finished.
Details
Windows_NT x86 Debug Build Build finished.
Details
Windows_NT_ES x64 Debug Build Build finished.
Details
license/cla All CLA requirements met.
Details

@wli3 wli3 deleted the wli3:integrate-nuget-ask-rebase branch Jan 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment