NuGet packaging #29408

tmat · 2018-08-20T23:50:42Z

Removes custom packaging scripts and most hand-written nuspecs with packages generated from csproj files that produce the assemblies contained in the packages.

This allows integration to Arcade build flow, proper dependency tracking and dotnet pack to work.
dotnet pack and msbuild /t:pack called on Roslyn.sln produces all packages. The same commands called on any project produces the package corresponding to the project.

Package kinds

Roslyn produces these kinds of packages:

Packages containing the output of a single project build (e.g. Microsoft.CodeAnalysis.Common, Microsoft.CodeAnalysis.CSharp, etc.)

These packages are trivial to build - the csproj just needs to set IsPackable and PackageDescription.
Analyzer packages (e.g. Microsoft.CodeAnalysis.CSharp.CodeStyle)

Analyzer packages are currently not supported directly by NuGet Pack target, but can be easily constructed using TargetsForTfmSpecificContentInPackage extensibility point, which allows to specify list of files to be included in the package.

Since the package contains both analyzer and fixer assemblies the package is built by the fixer project, which depends on the analyzer.
Source Packages (e.g. Microsoft.CodeAnalysis.Debugging)

These are created using hand-written nuspec. Arcade provides targets that automatically flow relevant msbuild properties to the nuspec variables to make it easier to generate packages using a hand-written nuspec.
Meta-Packages (e.g. Microsoft.CodeAnalysis, Microsoft.CodeAnalysis.Compilers, etc.)

Packages that do not carry any files. They only list dependencies on other packages. The projects generating these packages import build\Targets\PackageProject.targets, which overrides Build target to only resolve and build P2P references but skip build of the project itself. The Pack target produces the package.
Tools packages (Microsoft.Net.Compilers, Microsoft.NETCore.Compilers)

Tools package contains all binaries and other artifacts that are necessary to run a tool, e.g. csc. Like meta-packages the projects generating these packages do not themselves produce binaries, so they also import build\Targets\PackageProject.targets. The projects list all dependent projects that contribute to the tool package layout. It is thus possible to build the tools package by calling dotnet pack on the project in a clean repo. This is used to bootstrap the compilers. The bootstrapping script calls dotnet pack on Microsoft.Net.Compilers.Package.csproj, unzips the resulting package and points the main build to the tools directory.

Microsoft.NETCore.Compilers.Package.csproj triggers Publish target on all dependencies before building the package. dotnet pack on the project still works in a clean repo.
VS insertion packages
These are essentially tools packages that list all binaries that we insert into ExternalAPIs or Tools Razzle directories. I have removed logic from DevDivInsertionFiles that generated these packages. Instead these packages are generated from an explicit list of files.
Hacks (Microsoft.CodeAnalysis.Workspaces.Common)
This package does not follow the standard pattern of multi-targeted packages - it contains both Desktop and netstandard binaries in a single package, but does not multi-target. Instead it aggregates outputs of two different projects (Workspaces and Workspaces.Desktop). Projects that depend on Workspaces thus have implicit dependency on Workspaces.Desktop even when they are netstandard-only projects. To make this work a hand-written nuspec and a trick with setting PackageId were needed.

We should clean this up as we transition to netstandard 2.0: Cleanup Microsoft.CodeAnalysis.Workspaces packages #29292

Package and project interchangeability

In general, package and project references are designed to be interchangeable. That is, a project reference can be changed to a reference to a package produced by the project. This means that dependencies of a project correspond to dependencies of the package.

To avoid spilling dependencies on private VS packages to the dependency lists of our packages it was necessary to mark these dependencies with PrivateAssets="all" metadata. Due to the package-project correspondence this implied the need to add some missing package references.

Building multiple versions of packages (repacking)

Roslyn build produces 3 versions of packages: per-build pre-release, pre-release and release.
dotnet pack (or msbuild /t:pack) on Roslyn.sln now always produces per-build pre-release.
In CI (Jenkins or official build) a custom task RoslynTools.NuGetRepack is executed that takes a set of packages and updates their version: from per-build pre-release to pre-release or release. While doing so it validates consistency. When producing release packages it also generates a text file that lists all pre-release dependencies that were found in the given packages. This allows us to check whether we can push the release packages to NuGet.org (the file is empty).

Exact dependency versions

When generating packages from projects NuGet Pack task currently doesn't support generating exact versions to nuspec file (e.g. [2.11.0] instead of the default [2.11.0)) - filed NuGet/Home#7213. This is necessary when packages contain assemblies with InternalsVisibleTo relationship. To workaround I have updated RoslynTools.NuGetRepack to use exact versions when generating pre-release and release packages.

Package versions

Pre-release package versions now include commit SHA.

tmat · 2018-08-21T01:30:23Z

Seems like this issue is no longer relevant: dotnet/sdk#1159

tmat · 2018-08-21T02:06:05Z

@jaredpar @agocke @jasonmalinowski @dotnet/roslyn-infrastructure PTAL
/cc @jinujoseph

jasonmalinowski · 2018-08-21T16:45:58Z

To workaround I have updated RoslynTools.NuGetRepack to use exact versions when generating pre-release and release packages.

@tmat does this mean that the packages we get directly out of our build won't have the exact versions? That's stil somewhat worrisome, as we do have teams like VS for Mac consuming those packages. If nothing else, @KirillOsenkov, @brettfo and others should be aware that they're losing some protection if they aren't careful.

jaredpar · 2018-08-21T16:56:15Z

src/CodeStyle/CSharp/CodeFixes/Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.csproj

+      <_File Include="$(ArtifactsConfigurationDir)Dlls\Microsoft.CodeAnalysis.CSharp.CodeStyle\**\Microsoft.CodeAnalysis.CSharp.CodeStyle.resources.dll"/>
+      <_File Include="$(ArtifactsConfigurationDir)Dlls\Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes\**\Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.resources.dll"/>
+      <_File Include="$(ArtifactsConfigurationDir)Dlls\Microsoft.CodeAnalysis.CodeStyle\**\Microsoft.CodeAnalysis.CodeStyle.resources.dll"/>
+      <_File Include="$(ArtifactsConfigurationDir)Dlls\Microsoft.CodeAnalysis.CodeStyle.Fixes\**\Microsoft.CodeAnalysis.CodeStyle.Fixes.resources.dll"/>


This pattern worries me. Our porject files are increasing quite a bit and they're now highly dependent on our build output layout. Once we move to the standard layout every line here will need to be revisited.

Right, this will have to be updated. That's no different than having these paths in hand-written nuspecs, except here we can use msbuild properties.

We might be able to refine this a bit later to get the output paths from ProjectReference.

This seems to be present in only a few of the project files though. What is special about these?

In reply to: 211700636 [](ancestors = 211700636)

It is only needed for packages that include outputs of multiple projects.

Ah I see now. It's only when the NuPkg chooses to directly include the output of multiple projects vs. having one NuPkg per project and having the NuPkg reference structure match the project reference structure.

Why are we not doing the latter here?

In reply to: 212009717 [](ancestors = 212009717)

Analyzer packages contain both analyzer and fixer assemblies. This project builds analyzer package.

jaredpar · 2018-08-21T16:58:07Z

src/Compilers/Core/Portable/Microsoft.CodeAnalysis.csproj

+    -->
+    <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="$(MicrosoftCodeAnalysisAnalyzersVersion)" PrivateAssets="ContentFiles" />
+
+    <PackageReference Include="Microsoft.DiaSymReader.Native" Version="$(MicrosoftDiaSymReaderNativeVersion)" PrivateAssets="all" />


It seems like we are inconsistent with whose responsibility it is to mark a PackageReference as PrivateAssets=all to avoid it being included in the NuGet. All of the Visual Studio assemblies are covered with a single Update line in Imports.targets. Many other assemblies are special cased in the file. Should we just move to forcing everything to be in the project file?

We could. My thinking that the set of VS packages is a well-defined group for which this setting makes sense since Roslyn is a VS plugin. Essentially establishing a default and having to set PrivateAssets only on packages that are not in the default set.

jaredpar · 2018-08-21T16:58:41Z

src/Compilers/VisualBasic/Portable/Microsoft.CodeAnalysis.VisualBasic.vbproj

    <RootNamespace></RootNamespace>
+
+    <!-- NuGet -->


Nit: I'd remove this comment. I think we all know what this block means and it's just adding two lines to our project files.

jaredpar · 2018-08-21T16:59:00Z

src/Dependencies/CodeAnalysis.Debugging/Microsoft.CodeAnalysis.Debugging.nuspec

+  <files>
+    <file src="SourcePackages\Microsoft.CodeAnalysis.Debugging.Package\Microsoft.CodeAnalysis.Debugging.props" target="build" />
+    <file src="$ProjectDirectory$\**\*.cs" target="contentFiles/cs/netstandard1.3" />
+    <file src="$ProjectDirectory$\**\*.cs" target="contentFiles/cs/net45" />


Shouldn't this be net46? #Resolved

It should not. We need to support net45 target, so that the sources can be imported to Mono.Cecil. #Resolved

❔ Do we validate the net45 API compatibility during our PR build?

Yes, the project is targeting <TargetFrameworks>netstandard1.3;net45</TargetFrameworks>

jaredpar · 2018-08-21T17:01:01Z

src/VisualStudio/CSharp/Test/Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.csproj

    <PackageReference Include="Microsoft.VisualStudio.InteractiveWindow" Version="$(MicrosoftVisualStudioInteractiveWindowVersion)" />
+    <PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="$(MicrosoftVisualStudioShell150Version)" />


Why do we need all of the extra package references here?

Indeed: I've been trying to push us down to a minimal set -- having a bunch of extra references that would have otherwise been pulled in transitively has been a major source of pain for VS SDK upgrades.

Explained in the PR comment under Package and project interchangeability.

TLDR: Using PrivateAssets=true means that the references do not flow transitively.

jaredpar · 2018-08-21T17:01:28Z

Finished review pass (Iteration 2)

tmat · 2018-08-21T18:12:37Z

@jasonmalinowski

@tmat does this mean that the packages we get directly out of our build won't have the exact versions?

Yes, it does. If this becomes an issue we can update the NuGetRepack tool to repack the per-build packages as well. Or we can just sent PR to NuGet to add the support like I described in NuGet/Home#7213

sharwell

This pull request makes me realize how little I understand NuGet packaging. Specific questions in some comments.

sharwell · 2018-08-21T19:15:37Z

build/Targets/Settings.props

             Condition="'$(OS)' == 'Windows_NT'" />

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeQuality.Analyzers" Version="$(MicrosoftCodeQualityAnalyzersVersion)" PrivateAssets="all" />
-    <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="$(MicrosoftCodeAnalysisAnalyzersVersion)" PrivateAssets="all" />


❔ Where did this go?

To Microsoft.CodeAnalysis.csproj.

sharwell · 2018-08-21T19:16:24Z

build/scripts/build.ps1

@@ -124,8 +123,7 @@ function Process-Arguments() {
        exit 1
    }

-    $script:pack = $pack -or $packAll
-    $script:packAll = $packAll -or ($pack -and $official)
+    $script:pack = $pack


❔ Is this still needed, as opposed to just referencing $pack?

sharwell · 2018-08-21T19:19:49Z

src/CodeStyle/CSharp/CodeFixes/Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.csproj

+      <_File Include="$(ArtifactsConfigurationDir)Dlls\Microsoft.CodeAnalysis.CodeStyle\**\Microsoft.CodeAnalysis.CodeStyle.resources.dll"/>
+      <_File Include="$(ArtifactsConfigurationDir)Dlls\Microsoft.CodeAnalysis.CodeStyle.Fixes\**\Microsoft.CodeAnalysis.CodeStyle.Fixes.resources.dll"/>
+
+      <TfmSpecificPackageFile Include="@(_File)" PackagePath="analyzers/dotnet/cs/%(_File.RecursiveDir)%(_File.FileName)%(_File.Extension)" />


❔ Why the mix of / and \? #Resolved

Because NuGet. This is the preferred method for specifying output paths. #Resolved

sharwell · 2018-08-21T19:21:38Z

src/CodeStyle/VisualBasic/CodeFixes/Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.vbproj

+    <PackageDescription>
+      .NET Compiler Platform ("Roslyn") code style analyzers for Visual Basic.
+    </PackageDescription>
+    <TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificContentInPackage);_GetFilesToPackage</TargetsForTfmSpecificContentInPackage>


📝 This doesn't follow the normal xxxxxxDependsOn pattern for properties referencing targets by name. If this is a property defined somewhere in this pull request, it might make sense to update it.

This is NuGet extensibility point. It's not a list of dependent targets. Rather it's a list of targets that populate TfmSpecificPackageFile item group.

sharwell · 2018-08-21T19:25:14Z

src/Compilers/Core/Portable/Microsoft.CodeAnalysis.csproj

+      Microsoft.CodeAnalysis project or Microsoft.CodeAnalysis.Common package.
+
+      Note: PrivateAssets="ContentFiles" ensures that projects referencing Microsoft.CodeAnalysis.Common package 
+      will import everything but content files from Microsoft.CodeAnalysis.Analyzers, specifically, analyzers.


❔ Why not just omit PrivateAssets here and include a comment saying it's intentionally kept as a transitive dependency?

By default, when PrivateAssets is unspecified, NuGet excludes Analyzers when it flows dependencies. This makes sure that Analyzers are imported to projects that reference Microsoft.CodeAnalysis.Common package.

sharwell · 2018-08-21T19:27:44Z

src/Dependencies/CodeAnalysis.Debugging/Microsoft.CodeAnalysis.Debugging.nuspec

+  <files>
+    <file src="SourcePackages\Microsoft.CodeAnalysis.Debugging.Package\Microsoft.CodeAnalysis.Debugging.props" target="build" />
+    <file src="$ProjectDirectory$\**\*.cs" target="contentFiles/cs/netstandard1.3" />
+    <file src="$ProjectDirectory$\**\*.cs" target="contentFiles/cs/net45" />


❔ Do we validate the net45 API compatibility during our PR build?

sharwell · 2018-08-21T19:30:30Z

src/EditorFeatures/Core.Wpf/Microsoft.CodeAnalysis.EditorFeatures.Wpf.csproj

  </PropertyGroup>
  <ItemGroup Label="Project References">
    <ProjectReference Include="..\..\Compilers\Core\Portable\Microsoft.CodeAnalysis.csproj" />
-    <ProjectReference Include="..\..\Workspaces\Core\Desktop\Microsoft.CodeAnalysis.Workspaces.Desktop.csproj" />
+    <ProjectReference Include="..\..\Workspaces\Core\Desktop\Microsoft.CodeAnalysis.Workspaces.Desktop.csproj" PrivateAssets="all" />


❕ This looks like a bug to a reviewer. It should like to a bug describing the steps to remove the need to be different, or explain why it can't be changed for consistency. Applies also to other references to the same project.

PrivateAssets is required here. I'll add a comment.

sharwell · 2018-08-21T19:31:34Z

src/Features/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Features.csproj

@@ -59,7 +66,7 @@
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" />
-    <PackageReference Include="Humanizer.Core" Version="$(HumanizerCoreVersion)" />
+    <PackageReference Include="Humanizer.Core" Version="$(HumanizerCoreVersion)" PrivateAssets="true" />


❓ What is PrivateAssets="true"?

Typo, should be all.

sharwell · 2018-08-21T19:34:47Z

src/NuGet/Microsoft.NETCore.Compilers/Microsoft.NETCore.Compilers.Package.csproj

+      <_File Include="$(MSBuildProjectDirectory)\tools\**\*.*" TargetDir="tools" />
+
+      <!-- Targets and task files -->
+      <File Include="$(_Dlls)Microsoft.Build.Tasks.CodeAnalysis\netcoreapp2.0\Microsoft.Build.Tasks.CodeAnalysis.dll" TargetDir="tools" />


❓ Why are some <File> and some <_File>?

Typo. Fixing.

sharwell · 2018-08-21T19:35:31Z

src/NuGet/Microsoft.NETCore.Compilers/tools/bincore/.gitattributes

@@ -0,0 +1,9 @@
+# RunCsc/RunVbc are shell scripts and should always have unix line endings


❓ Why move this?

tmat · 2018-08-22T02:01:13Z

@jasonmalinowski Looking into non-flowing package references issue - some of our test projects actually already have unnecessary package references. They repeat references that flow from test utilities.

It looks like the ideal state would be having one utility test project per layer (workspaces, features, editorfeatures, VS) that all test projects on that layer reference and that lists the dependencies needed on that layer. The rest of the test projects would then have no additional package references.

tmat · 2018-08-22T02:03:43Z

src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.csproj

@@ -9,6 +9,7 @@
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <TargetFramework>netstandard1.3</TargetFramework>
    <CodeAnalysisRuleSet>..\CSharpCodeAnalysisRules.ruleset</CodeAnalysisRuleSet>
+    <GenerateMicrosoftCodeAnalysisCommitHashAttribute>true</GenerateMicrosoftCodeAnalysisCommitHashAttribute>


Accidentally removed in a previous commit.

jasonmalinowski · 2018-08-22T02:06:38Z

If this becomes an issue we can update the NuGetRepack tool to repack the per-build packages as well.

I think that's an issue now, since it means the packages we use on a regular basis won't have the same behavior of the ones we ship. In other words, this becomes an issue once we're about to ship packages we broke, didn't realize it, and have a major problem. 😄

How hard is this to just fix, or work around?

questions were answered

maririos · 2018-08-22T23:15:59Z

FYI @jcagme

tmat · 2018-08-23T22:46:43Z

@jasonmalinowski Your concern about the exact versions has been addressed.

tmat · 2018-08-23T22:52:40Z

Any more feedback?

tmat · 2018-08-27T21:01:34Z

@jaredpar Ping

jaredpar

tmat · 2018-08-27T22:17:49Z

Thanks!

tmat requested review from a team as code owners August 20, 2018 23:50

tmat force-pushed the NuGet branch from a34bd24 to 90cc43d Compare August 21, 2018 00:12

tmat force-pushed the NuGet branch from 9273c42 to 309a5e8 Compare August 21, 2018 02:05

jaredpar reviewed Aug 21, 2018

View reviewed changes

tmat force-pushed the NuGet branch from 309a5e8 to 4cb37dc Compare August 21, 2018 17:33

jinujoseph added the Area-Infrastructure label Aug 21, 2018

sharwell previously requested changes Aug 21, 2018

View reviewed changes

tmat force-pushed the NuGet branch 2 times, most recently from 35af1ad to 0113080 Compare August 21, 2018 22:45

tmat commented Aug 22, 2018

View reviewed changes

tmat added 4 commits August 23, 2018 14:31

NuGet packaging

6e41082

Add missing VS package references

01d6bd5

Fix scripts

ceedcd7

Add comment on Workspace.Desktop references with PrivateAssets

1ce1525

tmat added 5 commits August 23, 2018 14:31

Fix Microsoft.NETCore.Compilers package

116fb48

Fix up references

965f7f2

Build fixes

ef370df

Correct OutputPath for package projects

43a7ffc

Use NuGetRepack to force exact versions in per-build packages

4c3201a

tmat force-pushed the NuGet branch from 9dede12 to 4c3201a Compare August 23, 2018 21:32

tmat mentioned this pull request Aug 24, 2018

[Packaging] Define the way/ways that we want to create the nuget packages dotnet/arcade#383

Closed

jaredpar approved these changes Aug 27, 2018

View reviewed changes

tmat merged commit 8a34dfb into dotnet:master Aug 27, 2018

This was referenced Aug 28, 2018

Fix and clean up NuGet package publishing #29562

Merged

Fix Microbuild parameters #29588

Merged

		<PackageReference Include="Microsoft.VisualStudio.InteractiveWindow" Version="$(MicrosoftVisualStudioInteractiveWindowVersion)" />
		<PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="$(MicrosoftVisualStudioShell150Version)" />

		@@ -0,0 +1,9 @@
		# RunCsc/RunVbc are shell scripts and should always have unix line endings

NuGet packaging #29408

NuGet packaging #29408

Conversation

tmat commented Aug 20, 2018 • edited Loading

Package kinds

Package and project interchangeability

Building multiple versions of packages (repacking)

Exact dependency versions

Package versions

tmat commented Aug 21, 2018

tmat commented Aug 21, 2018

jasonmalinowski commented Aug 21, 2018

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

tmat Aug 22, 2018 • edited Loading

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

jaredpar Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

tmat Aug 21, 2018 • edited by jaredpar Loading

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

jaredpar commented Aug 21, 2018

tmat commented Aug 21, 2018

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell Aug 21, 2018 • edited by jaredpar Loading

Choose a reason for hiding this comment

jaredpar Aug 21, 2018 • edited Loading

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

tmat commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski commented Aug 22, 2018

maririos commented Aug 22, 2018

tmat commented Aug 23, 2018

tmat commented Aug 23, 2018

tmat commented Aug 27, 2018

jaredpar left a comment

Choose a reason for hiding this comment

tmat commented Aug 27, 2018

tmat commented Aug 20, 2018 •

edited

Loading

tmat Aug 22, 2018 •

edited

Loading

jaredpar Aug 21, 2018 •

edited

Loading

tmat Aug 21, 2018 •

edited by jaredpar

Loading

sharwell Aug 21, 2018 •

edited

Loading

sharwell Aug 21, 2018 •

edited by jaredpar

Loading

jaredpar Aug 21, 2018 •

edited

Loading

tmat commented Aug 22, 2018 •

edited

Loading