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.

Sign up for GitHub

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

Jump to bottom

[Proposal] Pack ProjectReferences #11708

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

[Proposal] Pack ProjectReferences #11708

Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2e31e25
Create pack-referenced-projects.md
maxkoshevoi Mar 28, 2022
6acbf91
Update pack-referenced-projects.md
maxkoshevoi Mar 28, 2022
3597234
Initial draft
maxkoshevoi Mar 30, 2022
ad4ceb2
Fix spelling
maxkoshevoi Mar 31, 2022
74f3948
ProjectReference.Pack
maxkoshevoi Apr 1, 2022
053f7d5
Update proposed/2022/pack-referenced-projects.md
maxkoshevoi Apr 7, 2022
c7a9656
Add a question
maxkoshevoi Apr 7, 2022
0028320
Update proposed/2022/pack-referenced-projects.md
maxkoshevoi Apr 9, 2022
ab2b92c
`PackagePath`
maxkoshevoi Apr 15, 2022
5f0313d
`PackagePath`
maxkoshevoi Apr 15, 2022
2e38238
Merge branch 'dev' into mk/pack-ref
maxkoshevoi Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions proposed/2022/pack-referenced-projects.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# Pack referenced projects

- Author Name [maxkoshevoi](https://github.com/maxkoshevoi)
- Start Date (2022-03-28)
- GitHub Issue [3891](https://github.com/NuGet/Home/issues/3891)
- GitHub PR (-)

## Summary

Allow referenced projects' artifacts to be packed inside the NuGet.

## Motivation
Copy link
Contributor

Choose a reason for hiding this comment

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

Great motivation. Thanks for writing it!


When SDK-style project is being packed as a NuGet, all projects that it references are considered as NuGet themselves that this NuGet depends on.
This is not always a desired behavior, sometimes user needs artifacts from referenced project(s) to be packed into the NuGet.
Copy link
Member

Choose a reason for hiding this comment

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

I think the motivation needs to capture the particular scenarios where this is helpful. We'd appreciate a bigger focus on the scenarios.

Currently there's no easy way to override this behavior except for ditching `csproj`, and going back to `nuspec`. In many cases that involves piping lots of information from the `MSBuild` context into the `nuspec` evaluation via `key=value` parameters.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's an unfair characterization to say the only way to pack more than assembly is by going with a nuspec, as None items can glob in an equivalent way the nuspec files elements can.

Furthermore the issue body does link to a comment that contains a workaround:
#3891 (comment), that can be implemented with the documented pack extensibility. https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#advanced-extension-points-to-create-customized-package

Chaining together dozens of arguments to achieve parity with metadata usually automagically introduced by the SDK becomes tedious, which eventually weakens the otherwise rich set of information attached to a modern NuGet package.
Copy link
Member

Choose a reason for hiding this comment

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

Someone brought up:
One of the primary use cases of bundling third party package content within your own package is custom build logic - analyzers, custom msbuild tasks, bundling satellite applications

I think this what the motivation should cover.

Copy link

@baronfel baronfel Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

One specific motivating example for this feature is MSBuild Tasks. Happy to see this brought up!


## Explanation

### Functional explanation
Copy link
Member

Choose a reason for hiding this comment

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

I think multiple asks are being conflated into 1 here.

  • Packaging multiple projects into 1 package.
  • Packaging assemblies from packages into a different package.

These are obviously related, but the scenarios vary and the potential for misuses of a feature that does things automatically varies.

It's worth nothing that IncludeReferenceProjects handled projects only, so I don't think it'd be unfair to suggest that these features are not all combined into one.


Proposed solution is to add new `Pack` and `PackagePath` properties to `ProjectReference` (`<ProjectReference Include="..." Pack="True" PackagePath="tools" />`) and `PackageReference`.
If `Pack` is `False` or not present - consider the project (or package) as a NuGet's dependency, if it's `True` - pack its artifacts in.
If `PackagePath` is not present - pack artifacts to the default location within the package. If it's present - pack them to the specified path.

For `PackageReference`s it's useful in case referenced package is from private feed, but NuGet that's being packed will be public.
Copy link
Member

Choose a reason for hiding this comment

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

nit: This needs to go into the motivation.

Copy link
Member

@nkolev92 nkolev92 Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Additionally an aside, packing assemblies from packages published on private feeds only can lead to a lot of issues.

If a package is in a private feed, it must have been done so for a reason. Redistributing assemblies for packages you don't own might lead into some licensing violations.

There's no guarantee that this package currently on a private feed (maybe because it's in previews) does not make it to NuGet.org. Redistributing the same assembly in multiple packages is something that's known to cause a lot of problems for the consumers.
The build and runtime work best when only 1 reference of the same assembly is used in an application. NuGet ensures that you cannot have multiple versions of the package in the project, but that guarantee cannot be transferred to the assemblies themselves. If an assembly is in multiple packages, it's very possible that someone ends up referencing both. The more dependency issues can be resolved at restore time the better.

The above concerns can be generalized to Never include the same assembly in multiple library packages.
Note the focus on library packages. In some scenarios, packages contain tools, and those often need the entry assembly + some of it's dependencies.


### Technical explanation

When NuGet is being created from `csproj` file:

1) If `ProjectReference` doesn't have `Pack` property, or has it set to `False` - preserve behavior that have existed before introduction of this feature (don't include artifacts of referenced projects into the NuGet, and consider them as NuGet-dependencies).
2) If `ProjectReference` has `Pack` property and it's set to `True` - include it's artifacts into the NuGet.
maxkoshevoi marked this conversation as resolved.
Show resolved Hide resolved
3) Do the same steps for `PackageReference`s
Copy link
Member

Choose a reason for hiding this comment

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

I think the concerns about distributing the same assembly in multiple packages in a good case against PackageReference inclusion for libraries. In fact, it should never be done for libraries.

Copy link
Member

Choose a reason for hiding this comment

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

Does this design recommend only assembly built by projects are included?

What about the PackageReference side? There are different types of assemblies in a package, some for compile, some for runtime. We'd need to define the behavior for these further.

There might also be compatibility concerns due to the potential usage of AssetTargetFallback.


## Drawbacks
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth to add that one of the best practices in NuGet today is to have one assembly per NuGet package.

https://docs.microsoft.com/en-us/nuget/create-packages/creating-a-package#decide-which-assemblies-to-package

https://docs.microsoft.com/en-us/nuget/create-packages/select-assemblies-referenced-by-projects#recommended-one-assembly-per-package

https://docs.microsoft.com/en-us/nuget/create-packages/select-assemblies-referenced-by-projects#not-recommended-multiple-assemblies-in-one-package

Copy link
Member

Choose a reason for hiding this comment

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

I think some of the feedback around compatibiltiy and redistribution of packages should be listed in the drawbacks here.


None

## Rationale and alternatives

- Use `IsPackable` property from referenced project to determine it it's supposed to be packed or be a NuGet dependency.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a fine consideration.


It will be more automated, but won't fit some edge cases (also it will be a breaking change). And it isn't applicable to `PackageReferences`.

- Introduce new project-wide `PackReferencedProjects` property that would switch pack behavior for all `ProjectReference`s inside a single project.

This one is less flexible since it cannot switch on per-`ProjectReference` basis. And it isn't applicable to `PackageReferences`.

- Another solution is to introduce a `IncludeReferencedProjects` switch for `nuget` CLI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be for dotnet CLI instead?

Copy link
Member

Choose a reason for hiding this comment

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

If a Project is being included into a joined package and never gets packed itself, it's dependencies would need to be something that has to be represented in the joined package.
Example:

Project 1 -> Project 2 -> Package A.

Say, Project 1 includes the reference project, and it is intended to be redistributed as a library, it's extremely likely that Project 1 needs Package A as a dependency.
This is a reverse restore. Instead of given the dependency, give me a resolved graph, it starts with a resolved graph and it tries to generate the correct top level dependencies. Note that we'd need to account for package include/excludes as well.

Copy link
Member

Choose a reason for hiding this comment

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

Would this be for dotnet CLI instead?

Correct. It'd be for dotnet CLI and the msbuild pack target (which the dotnet pack is just a wrapper for)


Downsides are: Behavior of individual projects (if packing is performed in bulk) and `ProjectReference`s cannot be changed, and it's not obvious how to properly pack any specific project (I thought we are consolidating all the information in `csproj` now). And it isn't applicable to `PackageReferences`.

## Prior Art

This design flaw have been a pain for the community for more than 5 years now. It was such an issue that even third party solution was created - [nugetizer](https://github.com/devlooped/nugetizer).
Copy link
Member

Choose a reason for hiding this comment

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

I think it's unfair to characterize the feature ask as design flaw. It's a feature that hasn't been implemented.
This also suggests that nugetizer was created because of the lack of this feature, which isn't correct.


I haven't used it, so don't have much to comment here, but community clearly [expressed a desire for out of the box solution](https://github.com/NuGet/Home/issues/3891#issuecomment-1080044314).

## Unresolved Questions

- **How do we flow/assign TFM based `lib`/`content` items?** (Is `PackagePath` a desired parameter?)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I fully understand this example. Can you clarify better what the ask is?

Copy link

Choose a reason for hiding this comment

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

Referring to the conversation earlier - here: #11708 (comment)

Copy link
Member

@nkolev92 nkolev92 Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for that link. In that case, I think the compat side would probably need to not be configurable.

In other words, given a project reference today, the particular assembly chosen by NuGet/SDK is the best compatible framework one, so I think the switch would only really need to be copy for project, or don't.

The conflict resolution between package items and references is more complex as that happens in a part that NuGet doesn't understand. So we'd need NuGet and the SDK to work through that in some way.

Copy link
Member

Choose a reason for hiding this comment

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

I understand there is potentially a better framework for some of the dependent assemblies, but accounting for that would make things too complicated imo. NuGet today chooses a full lib folder, so we can't really pick and choose from different folders.

- **How is conflict resolution going to work with consumers taking in a dependency already present in the bundle?**
Copy link
Member

Choose a reason for hiding this comment

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

It unfortunately doesn't. The controls for assembly includes are on a package level. Furthermore, it's not something that NuGet tries to address today. The build itself does go through conflict resolution itself.

- Should license flow/validation be considered right from the start?
- **Should a `PackageReference` or `ProjectReference` tagged with `Pack="true"` be resolved transitively?**
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to consider the ProjectReference side of this problem with a broader team. I can include them.

Copy link
Member

Choose a reason for hiding this comment

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

This is a very good question, which is why I think the motivation isn't the correct one here. Understanding the scenarios would guide this answer (the answer would usually be yes.)

- What happens to NuGets that are referenced in `PackageReference` tagged with `Pack="true"`? Do they become dependencies of the resulting NuGet or being packed into it.
Copy link
Member

Choose a reason for hiding this comment

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

This a reverse restore cases similar to the above one.


## Future Possibilities

This proposal would allow users more flexibility in choosing what is being packed as part of a NuGet, and how it's dependencies look like.
Copy link
Member

Choose a reason for hiding this comment

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

nit: More of a motivation thing.