Skip to content
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

NuGet packaging #29408

Merged
merged 9 commits into from
Aug 27, 2018
Merged

NuGet packaging #29408

merged 9 commits into from
Aug 27, 2018

Conversation

tmat
Copy link
Member

@tmat tmat commented Aug 20, 2018

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 tmat requested review from a team as code owners August 20, 2018 23:50
@tmat
Copy link
Member Author

tmat commented Aug 21, 2018

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

@tmat
Copy link
Member Author

tmat commented Aug 21, 2018

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

@jasonmalinowski
Copy link
Member

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.

<_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"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@tmat tmat Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

-->
<PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="$(MicrosoftCodeAnalysisAnalyzersVersion)" PrivateAssets="ContentFiles" />

<PackageReference Include="Microsoft.DiaSymReader.Native" Version="$(MicrosoftDiaSymReaderNativeVersion)" PrivateAssets="all" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<RootNamespace></RootNamespace>

<!-- NuGet -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<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" />
Copy link
Member

@jaredpar jaredpar Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be net46? #Resolved

Copy link
Member Author

@tmat tmat Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained in the PR comment under Package and project interchangeability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jaredpar
Copy link
Member

Finished review pass (Iteration 2)

@tmat
Copy link
Member Author

tmat commented Aug 21, 2018

@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

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Where did this go?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To Microsoft.CodeAnalysis.csproj.

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

$script:pack = $pack -or $packAll
$script:packAll = $packAll -or ($pack -and $official)
$script:pack = $pack
Copy link
Member

@sharwell sharwell Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<_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)" />
Copy link
Member

@sharwell sharwell Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Why the mix of / and \? #Resolved

Copy link
Member

@jaredpar jaredpar Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❕ 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ What is PrivateAssets="true"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo, should be all.

<_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" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo. Fixing.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Why move this?

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

tmat commented Aug 22, 2018

@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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally removed in a previous commit.

@jasonmalinowski
Copy link
Member

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?

@sharwell sharwell dismissed their stale review August 22, 2018 02:09

questions were answered

@maririos
Copy link
Member

FYI @jcagme

@tmat
Copy link
Member Author

tmat commented Aug 23, 2018

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

@tmat
Copy link
Member Author

tmat commented Aug 23, 2018

Any more feedback?

@tmat
Copy link
Member Author

tmat commented Aug 27, 2018

@jaredpar Ping

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@tmat tmat merged commit 8a34dfb into dotnet:master Aug 27, 2018
@tmat
Copy link
Member Author

tmat commented Aug 27, 2018

Thanks!

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

Successfully merging this pull request may close these issues.

6 participants