-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add SDK package for generating global CLI tool shims #660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I don't see any obvious structural problems but I also don't know the global tools stuff very well so you might want to make sure someone else reviews too.
…lTools.Sdk is used for
a0abc70
to
a1ff4a7
Compare
@pranavkm can you take a look at this too? |
</BundledPackageRestorerContent> | ||
</PropertyGroup> | ||
|
||
<WriteLinesToFile File="$(PublishDir)BundledPackageRestorer.csproj" Lines="$(BundledPackageRestorerContent)" Overwrite="true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sad 👏
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<TargetFramework>netcoreapp2.0</TargetFramework> | ||
<DisableImplicitFrameworkReferences>true</DisableImplicitFrameworkReferences> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need RestoreSources
or does it pick the one via Directory.Build.props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's set in the PreinstallBundledPackages target (see below). We can't set it here since the path will be machine-specific.
<copyright>$copyright$</copyright> | ||
<packageTypes> | ||
<!-- This project needs to be MSBuildSdk because its targets need to influence the way /t:Restore works --> | ||
<packageType name="MSBuildSdk" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Razor Sdk need one of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends. Do you expect users to reference razor as a PackageReference or only as an Sdk?
If you add this, UI tooling in VS for PackageReference will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably both. We use it as a PackageReference in other repos in aspnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If NuGet UI tooling wasn't a concern, I would say you should add this to your .nuspec
<packageTypes>
<packageType name="Dependency" /> <!-- PackageReference usage -->
<packageType name="MSBuildSdk" /> <!-- Sdk usage -->
</packageTypes>
But that might be dangerous because the NuGet team is thinking of breaking packages with multiple types.
See NuGet/Home#6298 and NuGet/Home#6484 for context.
Infrastructure work required to enable aspnet/Universe#1126.
This is a generalization of the work added by @prafullbhosale and @seancpeters in dotnet/Scaffolding#757. It pre-generates the apphost shim for global CLI tools for win-x86, win-x64, and osx-64.
It is eventually meant to move to https://github.com/dotnet/arcade so other teams can use it.
Usage
Projects that need to bundle and sign the global CLI tool shim should add this to their .csproj file. This will include files in the .nupkg
cc @wli3