Conversation
|
Seems like this issue is no longer relevant: dotnet/sdk#1159 |
|
@jaredpar @agocke @jasonmalinowski @dotnet/roslyn-infrastructure PTAL |
@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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
It is only needed for packages that include outputs of multiple projects.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Analyzer packages contain both analyzer and fixer assemblies. This project builds analyzer package.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Shouldn't this be net46? #Resolved
There was a problem hiding this comment.
It should not. We need to support net45 target, so that the sources can be imported to Mono.Cecil. #Resolved
There was a problem hiding this comment.
❔ Do we validate the net45 API compatibility during our PR build?
There was a problem hiding this comment.
Yes, the project is targeting <TargetFrameworks>netstandard1.3;net45</TargetFrameworks>
There was a problem hiding this comment.
Why do we need all of the extra package references here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Explained in the PR comment under Package and project interchangeability.
There was a problem hiding this comment.
TLDR: Using PrivateAssets=true means that the references do not flow transitively.
|
Finished review pass (Iteration 2) |
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
left a comment
There was a problem hiding this comment.
This pull request makes me realize how little I understand NuGet packaging. Specific questions in some comments.
There was a problem hiding this comment.
To Microsoft.CodeAnalysis.csproj.
There was a problem hiding this comment.
❔ Is this still needed, as opposed to just referencing $pack?
There was a problem hiding this comment.
❔ Why the mix of / and \? #Resolved
There was a problem hiding this comment.
Because NuGet. This is the preferred method for specifying output paths. #Resolved
There was a problem hiding this comment.
📝 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
❔ Why not just omit PrivateAssets here and include a comment saying it's intentionally kept as a transitive dependency?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
❔ Do we validate the net45 API compatibility during our PR build?
There was a problem hiding this comment.
❕ 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.
There was a problem hiding this comment.
PrivateAssets is required here. I'll add a comment.
There was a problem hiding this comment.
❓ What is PrivateAssets="true"?
There was a problem hiding this comment.
❓ Why are some <File> and some <_File>?
35af1ad to
0113080
Compare
|
@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. |
There was a problem hiding this comment.
Accidentally removed in a previous commit.
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? |
|
FYI @jcagme |
|
@jasonmalinowski Your concern about the exact versions has been addressed. |
|
Any more feedback? |
|
@jaredpar Ping |
|
Thanks! |
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 packto work.dotnet packandmsbuild /t:packcalled 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
IsPackableandPackageDescription.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
TargetsForTfmSpecificContentInPackageextensibility 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 overridesBuildtarget to only resolve and build P2P references but skip build of the project itself. ThePacktarget 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 callingdotnet packon the project in a clean repo. This is used to bootstrap the compilers. The bootstrapping script callsdotnet packonMicrosoft.Net.Compilers.Package.csproj, unzips the resulting package and points the main build to thetoolsdirectory.Microsoft.NETCore.Compilers.Package.csprojtriggersPublishtarget on all dependencies before building the package.dotnet packon 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
PackageIdwere 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(ormsbuild /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.