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

Requirement for CLI tool authors to list Microsoft.NETCore.Platforms in nuspec #8418

Closed
natemcmaster opened this Issue Jan 17, 2018 · 19 comments

Comments

Projects
None yet
6 participants
@natemcmaster
Copy link
Member

natemcmaster commented Jan 17, 2018

According to current design spec, global CLI tool authors must define the <dependencies> section in spec as follows:

      <dependencies>
        <dependency id="Microsoft.NETCore.Platforms" version="2.0.1" />
      </dependencies>

Is this really necessary? From my initial tests, portable .NET Core app tools restore just fine without this in the nuspec.

IIUC this is only required for rid-specific tools, not portable ones. RID-restores require it because dotnet-install-tool is implemented as a dotnet-restore. Could dotnet-install-tool add Microsoft.NETCore.Platforms to the generated csproj it uses to call dotnet-restore?

cc @wli3 @livarcocc

cref aspnet/DotNetTools#383

@livarcocc livarcocc added this to the 2.2.0 milestone Jan 18, 2018

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 22, 2018

Try to list problems on the road: If we want to add Platforms dependency in temp project instead of nupkg. As in today's master. Temp project only allow 1 package reference (to prevent the state of restore multiple tool with one temp project) and package type need to be DotnetTool type (to prevent dotnet install tool -g a-library-package-id, although we don't have a good error message return back). This check is in nuget. If we allow platforms package to be referenced, we have to find another way to solve the previous 2 problems

The requirement of Microsoft.NETCore.Platforms also make --source painful:

the feed of --source need all the packages, the means if the producer want to try out the package using --source + a local folder, s/he also need to put the correct version of platforms in the source folder as well.

@livarcocc livarcocc changed the title Requirement for CLI tool authors to list Microsoft.NETCore.Platforms in nuspec [tool] Requirement for CLI tool authors to list Microsoft.NETCore.Platforms in nuspec Jan 22, 2018

@livarcocc livarcocc added the tool label Jan 22, 2018

@wli3 wli3 changed the title [tool] Requirement for CLI tool authors to list Microsoft.NETCore.Platforms in nuspec Requirement for CLI tool authors to list Microsoft.NETCore.Platforms in nuspec Jan 24, 2018

@natemcmaster

This comment has been minimized.

Copy link
Member

natemcmaster commented Jan 24, 2018

By the way, this is the error message users will see if they try to install a package that doesn't have M.NC.Platforms in the dependency graph.

Install failed. Failed to download package:
NuGet returned:

Failed to restore package.
WorkingDirectory:
Arguments: restore C:\Users\namc\AppData\Local\Temp\xxvuzjml.r2u\tsyuxbkj.kwm.csproj --source .\artifacts\build\ --runtime win10-x64 /p:BaseIntermediateOutputPath=\"C:\Users\namc\.dotnet\tools\dotnet-watch\2.1.0-preview1-t000\"
Output:   Restoring packages for C:\Users\namc\AppData\Local\Temp\xxvuzjml.r2u\tsyuxbkj.kwm.csproj...
  Installing dotnet-watch 2.1.0-preview1-t000.
C:\Users\namc\AppData\Local\Temp\xxvuzjml.r2u\tsyuxbkj.kwm.csproj : error NU1202: Package dotnet-watch 2.1.0-preview1-t000 is not compatible with netcoreapp2.1 (.NETCoreApp,Version=v2.1) / win10-x64. Package dotnet-watch 2.1.0-preview1-t000 supports: netcoreapp2.1 (.NETCoreApp,Version=v2.1) / any
  Restore failed in 263.85 ms for C:\Users\namc\AppData\Local\Temp\xxvuzjml.r2u\tsyuxbkj.kwm.csproj.
@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 25, 2018

let me see if directly adding runtime.json to temp project can work. Still sounds hacky.

need some help cc @nkolev92

@nkolev92

This comment has been minimized.

Copy link
Contributor

nkolev92 commented Jan 25, 2018

The platforms package is the only package that knows of the " win10-x64" => "any" mapping. As far as NuGet is concerned, we don't do anything fancy there,

@wli3, Adding it to the project won't work.
We don't pull in extras there. Just read package reference and project related info.

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 25, 2018

@nkolev92 hmm any way to squeeze this dependency in temp project restore time? So the producer don't have to add it all the time?

@nkolev92

This comment has been minimized.

Copy link
Contributor

nkolev92 commented Jan 25, 2018

@wli3
That would break all the other restrictions.

@nkolev92

This comment has been minimized.

Copy link
Contributor

nkolev92 commented Jan 25, 2018

You can always consider auto-including the platforms package during pack.

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 25, 2018

You can always consider auto-including the platforms package during pack.

That's already done tho.

I think there is problems on both side. But no blocking problem

The version of 'Microsoft.NETCore.Platforms` is hard. In a sense, it is fine with any version. But now we need to find the version that packaged with the tool. It is hard for offline scenario. dotnet-watch need to match the Platforms version in offline cache. Also if a package is packed with old runtime.json, it cannot run on newer RID, like osx-10.13.

During pack we had a hard time to find the right version of Platforms. It needs to be the version associated with Microsoft.NETCore.App or it will be downgrade error (note: this is producer, making a console app and SDK is adding the implicit Platforms package). But that version if very hard to compute, since SDK know what Microsoft.NETCore.App depends on after restore, but there is a packagereference need to be added before restore.

@natemcmaster

This comment has been minimized.

Copy link
Member

natemcmaster commented Jan 25, 2018

It seems like we're incurring unneeded complexity here. Could we alter NuGet to do a "portable" restore as a workaround for now and only look at tools/$txm/. The RID piece in tools/$txm/$rid/ only applies to support for "native" tools, such as a tool implemented in C++ or as a .NET Core standalone app. These aren't even supported in the current implementation -- only portable .NET Core apps are allowed.

If someone really needed a RID-specific tool, they could workaround by adding a portable .NET Core app that basically acts as a facade; the facade finds the right binary for the current OS and does Process.Start to forward arguments into the new process.

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 25, 2018

This issue will come again at next major version. And tools can only support one of them even the author take action. Or 2 different packageid

The producer in this case can multi targeting, change <TargetFramework>netcoreapp2.1<TargetFramework/> to <TargetFrameworks>netcoreapp2.1;netcoreapp9.0<TargetFrameworks/> and run pack, the filtering will find the right asset and use it.

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Jan 26, 2018

the facade finds the right binary for the current OS

We probably will end up doing the same thing as nuget doing today. Use platforms package and select the right asset.

@natemcmaster

This comment has been minimized.

Copy link
Member

natemcmaster commented Jan 26, 2018

The producer in this case can multi targeting,

A producer still could do this. For this particular issue, I'm not suggesting we change that. I'm only suggesting NuGet doesn't need to do a RID restore. Just use tools/$txm/. NuGet has built-in knowledge of txm's, not RIDs.

@natemcmaster

This comment has been minimized.

Copy link
Member

natemcmaster commented Jan 26, 2018

This issue will come again at next major version.

By the way, no matter what you decide on how to use NuGet, RIDs and TFMs, you will still run in the next major version problem. The only way to solve this one is to support (1) detecting required .NET Core runtimes, (2) installing them from dotnet-install, or (3) changing .NET Core 3+ to automatically roll-forward apps build for .NET Core < 3. Target frameworks don't solve (1) (TFM != runtime), and (2) isn't something you can solve with NuGet unless we started publishing the .NET Core runtime itself as a package.

@dasMulli

This comment has been minimized.

Copy link
Contributor

dasMulli commented Jan 26, 2018

You can always consider auto-including the platforms package during pack.

The hard part about that is that tools would need to be re-packed again for every new addition to the platforms package even if there is no change to the tool.
E.g. when osx.10.14 comes out and #8436 happens again.

@dasMulli

This comment has been minimized.

Copy link
Contributor

dasMulli commented Jan 26, 2018

Would it make sense to have a NuGet feature to provide a fallback RID graph as part of its installation?

That may be helpful for other scenarios as well, e.g. runtime assets in packages consumed by classic .net framework projects.

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Feb 13, 2018

Here is the plan to remove the dependency in tool package to Microsoft.NETCore.Platforms

  • Pass "any" as RID to nuget during restore of temp project, so there is no need for Microsoft.NETCore.Platforms. However that also blocked all RID specific scenarios
  • Remove implicit package reference during pack in SDK
  • Until NuGet/Home#6559 is finished
  • Add Microsoft.NETCore.Platforms to temp project during restore
@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Feb 13, 2018

@KathleenDollard

This comment has been minimized.

Copy link

KathleenDollard commented Feb 13, 2018

This is the plan.

There is a plan for RID specific packages in the future: Will, if you can maintain notes on what we plan on that it would be great, probably a separate issue, which will also call out that they are not supported in this first version.

For clarity, the user is not expected to be aware of the temp project, so no action will be needed on this issue by either consumer or producer (we removed the need for the producer to include Microsoft.NETCore.Platforms)

@wli3

This comment has been minimized.

Copy link
Collaborator

wli3 commented Feb 15, 2018

@KathleenDollard created #8617 for 2.1.3xx
#8618 is for future.

I am closing this issue in favor of 8617

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