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

Conversation

Projects
None yet
6 participants
@tmat
Member

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: #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 dotnet/roslyn-compiler as code owners Aug 20, 2018

@tmat

This comment has been minimized.

Member

tmat commented Aug 21, 2018

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

@tmat

This comment has been minimized.

@jasonmalinowski

This comment has been minimized.

Member

jasonmalinowski commented Aug 21, 2018

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"/>

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

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.

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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 comment has been minimized.

@jaredpar

jaredpar Aug 22, 2018

Member

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)

This comment has been minimized.

@tmat

tmat Aug 22, 2018

Member

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

This comment has been minimized.

@jaredpar

jaredpar Aug 22, 2018

Member

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)

This comment has been minimized.

@tmat

tmat Aug 22, 2018

Member

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" />

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

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?

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

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" />

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

Shouldn't this be net46? #Resolved

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

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

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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)" />

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

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

This comment has been minimized.

@jasonmalinowski

jasonmalinowski Aug 21, 2018

Member

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.

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

Explained in the PR comment under Package and project interchangeability.

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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

@jaredpar

This comment has been minimized.

Member

jaredpar commented Aug 21, 2018

Finished review pass (Iteration 2)

@tmat

This comment has been minimized.

Member

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

@sharwell

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" />

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

Where did this go?

This comment has been minimized.

@tmat

tmat Aug 23, 2018

Member

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

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

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)" />

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

Why the mix of / and \? #Resolved

This comment has been minimized.

@jaredpar

jaredpar Aug 21, 2018

Member

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>

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

📝 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 comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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.

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

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

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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" />

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

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" />

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

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.

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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" />

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

What is PrivateAssets="true"?

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

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" />

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

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

This comment has been minimized.

@tmat

tmat Aug 21, 2018

Member

Typo. Fixing.

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

This comment has been minimized.

@sharwell

sharwell Aug 21, 2018

Member

Why move this?

@tmat

This comment has been minimized.

Member

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>

This comment has been minimized.

@tmat

tmat Aug 22, 2018

Member

Accidentally removed in a previous commit.

@jasonmalinowski

This comment has been minimized.

Member

jasonmalinowski commented Aug 22, 2018

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

This comment has been minimized.

Member

maririos commented Aug 22, 2018

FYI @jcagme

@tmat

This comment has been minimized.

Member

tmat commented Aug 23, 2018

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

@tmat

This comment has been minimized.

Member

tmat commented Aug 23, 2018

Any more feedback?

@tmat

This comment has been minimized.

Member

tmat commented Aug 27, 2018

@jaredpar Ping

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

14 of 16 checks passed

windows_debug_vs-integration_prtest Build finished.
Details
windows_release_vs-integration_prtest Build finished.
Details
WIP ready for review
Details
license/cla All CLA requirements met.
Details
microbuild_prtest Build finished.
Details
ubuntu_16_debug_prtest Build finished.
Details
ubuntu_16_mono_debug_prtest Build finished.
Details
windows_build_correctness_prtest Build finished.
Details
windows_coreclr_debug_prtest Build finished.
Details
windows_coreclr_release_prtest Build finished.
Details
windows_debug_spanish_unit32_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_determinism_prtest Build finished.
Details
windows_release_unit32_prtest Build finished.
Details
windows_release_unit64_prtest Build finished.
Details
@tmat

This comment has been minimized.

Member

tmat commented Aug 27, 2018

Thanks!

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