Add design for packed project references#14875
Conversation
Document the explicit ProjectReference pack opt-in so the client implementation can be reviewed against agreed scenarios and redistribution risks.
Capture the branch's choices around project-reference closure, package path handling, symbols, and dependency promotion so reviewers can challenge the design directly.
dotnet-policy-service
Bot
added
the
Community
Point reviewers from the Home design to the NuGet.Client implementation PR that motivated the proposal.
zivkan
left a comment
zivkan
left a comment
There was a problem hiding this comment.
I've been busier than usual the last few weeks, so I haven't made time to read and think about this much until now. Some of it is still churning in my head, so there are parts I don't have an opinion on yet.
I'm generally supportive of trying to aim small, get the simple scenarios working, and later we'll see if we need to extend it. I just hope that it's not too limited on first release and people get frustrated that it's not usable. But for this feature, I think that being able to pack multiple simple class libraries will be a good first start.
|
|
||
| ## Motivation | ||
|
|
||
| `dotnet pack` and `msbuild /t:pack` currently treat project references as package dependencies. This is the right default for projects that are independently packaged and versioned, but it is painful for packages that intentionally expose one package while their implementation is split across multiple projects. |
There was a problem hiding this comment.
I think that describing it as painful is unhelpful opinionated commentary.
In the list of common scenarios below, the first two are personal preferences. So, I don't believe it's fair to say it's painful. Both scenarios would work just fine from a technical perspective with multiple nupkgs.
The third talks about needing a migration, but doesn't explain why it's difficult or painful. If the projects are simple classlibs, the only migration cost of nuget.exe pack -IncludeReferencedProjects to msbuild -t:pack is migrating from packages.config to PackageReference. But if the developer is willing to accept their single package becoming multiple packages, there is no additional migration cost. On the other hand, if they want to onboard onto the feature this spec is proposing, then there is additional migration cost.
The example in the list of common scenarios is a good scenario, and if we can make this easier for package authors, it will be a great win. I don't have experience creating packages with roslyn analyzers, so I don't know how painful it is, but if it's achievable with a <None Include="../../My.Analyzer/bin/$(Configuration)/netstandard2.0/My.Analyzer.dll" Pack="true" PackagePath="analyzers/dotnet/cs" />, then that doesn't feel very painful to me.
From a package consumer point of view, there's pretty much no difference between a package that has multiple assemblies, or a package with one assembly and one or more dependencies. NuGet will download all the packages and hook them into the build either way.
| - Symbols are included consistently with existing `IncludeSymbols` behavior. | ||
| - The default package location follows the existing pack convention for build output, for example `lib/<tfm>/Implementation.dll`. | ||
|
|
||
| If a bundled project references another project, the default behavior should be to include the project-reference closure under the opted-in project. This is required for runtime correctness in common "private implementation assembly" scenarios. The package author opts in at the first redistributed edge; NuGet then follows the restored project graph from that edge so the package contains the implementation assemblies needed by that bundled project. |
There was a problem hiding this comment.
Edge case: the child project has a ProjectReference to a project that the parent project also has a ProjectReference to, but the parent doesn't set Pack="true" on the transitive project.
I guess possible options are:
- Add the transitive assembly to the package, and suppress it as a package dependency
- Do not include the transitive assembly to the package, only add its package as a dependency
- Both add the transitive assembly to the package, and also add it as a package dependency. Consider this an authoring error that NuGet will not try to prevent/solve.
I don't know which is best.
|
|
||
| When `PackagePath` is specified, NuGet places the referenced project's outputs under that path instead of under `BuildOutputTargetFolder` and the target framework folder. The package author is responsible for choosing a path that is meaningful for the package type. For normal library assemblies, omitting `PackagePath` is preferred so outputs remain under the target framework's `lib` folder. | ||
|
|
||
| For multi-targeting projects, pack applies the behavior per target framework. A project reference that only applies to a subset of target frameworks is bundled only for those frameworks. The feature is also controlled by existing pack switches: if `IncludeBuildOutput=false`, bundled project build outputs are not added to the package. |
There was a problem hiding this comment.
I think this should be rephrased, and possibly an example given for clarity. I think I know what you're trying to say, but I don't know if I'm just guessing based on my own knowledge.
For example, something like:
NuGet will use normal restore and build asset selection to choose child project assemblies to copy into the package. For example, ProjectA targeting net10.0 and net481 with a
ProjectReferenceto ProjectB, which targets net8.0 and net472 will have ProjectA's net10.0 and ProjectB's net8.0 dlls inlib/net10.0/, and ProjectA's net481 and ProjectB's net472 dlls inlib/net481/. If ProjectB targets only netstandard2.0, then that assembly will be duplicated in the package'slib/net10.0/andlib/net481/directories.
|
|
||
| When `PackagePath` is specified, NuGet places the referenced project's outputs under that path instead of under `BuildOutputTargetFolder` and the target framework folder. The package author is responsible for choosing a path that is meaningful for the package type. For normal library assemblies, omitting `PackagePath` is preferred so outputs remain under the target framework's `lib` folder. | ||
|
|
||
| For multi-targeting projects, pack applies the behavior per target framework. A project reference that only applies to a subset of target frameworks is bundled only for those frameworks. The feature is also controlled by existing pack switches: if `IncludeBuildOutput=false`, bundled project build outputs are not added to the package. |
There was a problem hiding this comment.
Should there be a warning when IncludeBuildOutput is false, and one or more ProjectReference has Pack="true"?
On one hand, I don't like it when mutually exclusive features are configured at the same time, and there's no feedback about this probable configuration mistake. If I'm new to the technology, I don't yet know enough to debug why it's silently failing.
On the other hand, the scenario should be simple enough to debug, since the project's own assembly won't be making it into the package. Also trying to consider every permutation of incompatible features and emit warnings could be noisy, slow down the product from an explosion of validation checks, make docs more confusing with an overabundance of warning messages.
| PackagePath="analyzers/dotnet/cs" /> | ||
| ``` | ||
|
|
||
| When `PackagePath` is specified, NuGet places the referenced project's outputs under that path instead of under `BuildOutputTargetFolder` and the target framework folder. The package author is responsible for choosing a path that is meaningful for the package type. For normal library assemblies, omitting `PackagePath` is preferred so outputs remain under the target framework's `lib` folder. |
There was a problem hiding this comment.
I'm wondering if this could be problematic for multi-targeting projects.
Say I have project Contoso with a ProjectReference to Contoso.Analyzers, which I want to put in analyzers/dotnet/cs.
If both Contoso and Contoso.Analyzers multi-target, we can't have two files with the same name with different contents. If NuGet's instructed to put both copies in the same location, it needs to fail with a duplicate file error. I did a search, but wasn't able to quickly find what the NU error code is for this.
Only if Contoso.Analyzers is single targeting (probably netstandard2.0), then it can make sense to allow it. It becomes an implementation detail to de-duplicate, as long as the source path is the same for duplicate target paths. I don't believe NuGet does this at the moment.
edit: this appears to be called out already in unresolved questions.
| ## Unresolved Questions | ||
|
|
||
| - Should NuGet warn when an opted-in project reference points to a project that is itself packable or has a `PackageId`, since that may indicate the project should remain a package dependency? | ||
| - Should transitive bundled project outputs inherit the root opted-in reference's `PackagePath`, or should only the directly opted-in project's outputs use that override? Inheriting the path is simple, but may be wrong for mixed asset types. |
There was a problem hiding this comment.
There's also the question of diamond dependencies.
If I'm packing ProjectA, which have references to ProjectB and ProjectC, both of which depend on ProjectD, if I use a different PackagePath for ProjectB and ProjectC, where does ProjectD's output go?
| - Do not change `nuget.exe pack -IncludeReferencedProjects`. | ||
| - Do not change the default handling of project references. | ||
|
|
||
| The current implementation shape uses two assets-file reads during pack: one to collect all project references for the existing package-version lookup, and one to collect the subset of project references whose outputs should be packed. The packed project-reference items carry `TargetFramework`, `BuildProjectReferences=false`, and, when specified, the requested package path. |
There was a problem hiding this comment.
are you referring to the pull request you already opened, or are you referring to how pack is already implemented in the .NET SDK that ships today?
In either case, it sounds like an implementation detail not really relevant for a feature spec.
| At restore time, NuGet should preserve pack-specific project reference metadata in the restore graph and `project.assets.json`: | ||
|
|
||
| - `Pack` | ||
| - `PackagePath` |
There was a problem hiding this comment.
Please provide a json snippet of the part of the assets file that will change, and what it looks like. I highly recommend using a diff code block, so github can highlight the changes, making it easier for people reading the spec.
|
|
||
| The implementation should keep restore as the source of truth for the project graph. Pack should not rediscover an independent project graph or infer project reference metadata from only the entry project. | ||
|
|
||
| At restore time, NuGet should preserve pack-specific project reference metadata in the restore graph and `project.assets.json`: |
There was a problem hiding this comment.
Thinking out loud: restore is NuGet's primary use case. Far more people restore packages and never pack, than people that pack. Let alone people who will want to pack and use this spec's feature.
Adding more content to the assets file means that reading the file has more work to do, and therefore will be slower, even in scenarios (restore) where the additional metadata isn't used.
As long as the assets file is effectively unmodified in projects that don't use this spec's feature, then it should be fine, no measurable perf degradation. But within Microsoft we have some repos using 1000+ projects per restore, so even a few additional memory allocations can add up to additional garbage collections and slower restores.
If we can't avoid perf regressions for restore by adding this feature, while using the assets file, then I think it'll be better to make pack slower by needing to run additional msbuild targets, or whatever is needed, to collect the additional project information it needs.
2 participants
Links
Summary
ProjectReferenceoutputs.Test plan