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

[Feature Request]: EnforceAssets attribute for PackageReference #12842

Closed
rainersigwald opened this issue Aug 24, 2023 · 8 comments
Closed

[Feature Request]: EnforceAssets attribute for PackageReference #12842

rainersigwald opened this issue Aug 24, 2023 · 8 comments
Labels
Functionality:Pack Functionality:Restore Resolution:Duplicate This issue appears to be a Duplicate of another issue Transferred issue This issue is transferred from VSFeedback or other github repo Type:Feature

Comments

@rainersigwald
Copy link


Issue moved from dotnet/msbuild#9163


From @EdLichtman on Thursday, August 24, 2023 1:04:14 PM

Summary

Sometimes a library may depend on another but not because you're using that library in the child.

This is to tell the parent that it needs to import a sibling.

Background and Motivation

I'm packaging up distributable json manifest files that add themselves to a project in the form of AdditionalFiles.

I'm also packaging a single analyzer that finds all the manifest additional files and generates code from them.

The problem I'm having is, if the json packages have explicit dependencies on the analyzer, then the analyzer runs multiple times and clashes with itself. If I set PrivateAssets=none and IncludeAssets=none or ExcludeAssets=all, then the dependency graph in nuget doesn't list the analyzer as a dependency.

An additional issue I can see this solving is, I would like a joint dependency, where the analyzer depends on the json being present and the json depends on the analyzer being present but neither of them are dependent upon each other and therefore don't cause a circular dependency.

Proposed Feature

I'm thinking EnforceAssets would be the reverse of PrivateAssets, but wouldn't interfere with include or exclude.

Therefore you can say:

EnforceAssets=build;buildTransitive;analyzers IncludeAssets=build

And the build would be the only thing the child uses but then tells the parent it must depend on those other assets.

Alternative Designs

I'm working on a snippet I found to add Pack=true called "AddPackDependencies" I found to make it easier to communicate to a parent that a dependency is required. Problem is it also compiles the dll into it to avoid using the targetmoniker syntax. I am going to see if I can alter the function to include this, but it's been slow going, understanding the whole pipeline.

@ghost ghost added Transferred issue This issue is transferred from VSFeedback or other github repo Triage:Untriaged labels Aug 24, 2023
@rainersigwald
Copy link
Author

if the json packages have explicit dependencies on the analyzer, then the analyzer runs multiple times and clashes with itself.

This doesn't sound right--@EdLichtman can you give an example of that happening?

@EdLichtman
Copy link

@rainersigwald

I'm using this target:
https://til.cazzulino.com/msbuild/how-to-include-package-reference-files-in-your-nuget-package

That causes the dll of the analyzer to be included with the package.

I'll work on getting an example for you that isn't proprietary, and am wondering if this pack target could be causing problems.

Regardless, I've also thought about this because I've been using Nuke.Common and there are build targets thay aren't passed to the child in nuke.common. But I don't know about those until runtime. So I spent ti.e tracking down why runtime nuke wasn't working, only to find out you need to explicitly add a dependency in the parent.

This is only 2 examples I can think of but I've had to creatively solve my way around this a number of times up to now because there hasn't been a way to mandate that there must be a full dependency for the parent.

@ghost
Copy link

ghost commented Aug 24, 2023

Issue is missing Type label, remember to add a Type label

@ghost ghost added the missing-required-type The required type label is missing. label Aug 24, 2023
@EdLichtman
Copy link

@rainersigwald
msftbot linked me to a 404 not found page, so I don't know what to do.

Meanwhile, I have spent all day working on some things just to better understand it, and I finally came up with a perfectly simple example of code for your review.

https://github.com/EdLichtman/UniformPackagesDemo/tree/master/DependencyHellExample

Take a look, starting in DependencyHell. You'll see I package up a target that throws an error if the parent implementer is missing a property.

Then, DependencyHellParent takes in DependencyHell and mandates to anything implementing it "you need to use this PackageReference. I'm doing it. You should do it too" but, I Exclude the BuildTransitive assets from the Parent. I set PrivateAssets to none though because I don't want to keep anything private, I want everything depending on Parent to have every asset type of DependencyHell.

Finally, if you take a look at DependencyHellRecipient, you'll see that it pulls in Parent. By definition, it should pull in BuildTransitive, which is something I've tested before and confirmed. Look in the picture below and you'll see DependencyHellParent has the Dependency of "DependencyHell"

image

And yet, when I build DependencyHellRecipient, I don't receive the error I'm expecting to see from the buildTransitive file

@EdLichtman
Copy link

@rainersigwald

I imagine there's a bit of a security consideration, as well as complexity of feature.

One other consideration is complexity. Another idea I have (as I was going to propose adding something like this to @matkoch's nuke repo) is an attribute:

EnforceAssetsWarningCode

Basically you would configure a warning code and if the consuming library requires that the child Enforce it, the warning will spit out a message like

"My.Tooling.Library requires a dependency on Nuke.Common assets that are not available with an implicit reference. To ensure compatibility with this library, make sure to reference Nuke.Common"

@zivkan zivkan added Type:Feature Functionality:Pack Functionality:Restore WaitingForCustomer Applied when a NuGet triage person needs more info from the OP and removed Triage:Untriaged missing-required-type The required type label is missing. labels Aug 28, 2023
@zivkan
Copy link
Member

zivkan commented Aug 28, 2023

Sometimes a library may depend on another but not because you're using that library in the child.

@EdLichtman It sounds to me like a different way to phrase the request (or a different way to think about it) is that you'd like to be able to do <PackageReference Include="SomePackage" Version="1.2.3" ExcludeAssets="all" />, but when the project is packed, the package has a dependency on SomePackage. Put another way, have a PackageReference whose assets are excluded from the current project, but are included transitively.

If so, there's a duplicate issue that you could upvote to help with prioritization:

Do you agree that that issue is a duplicate of this one? Or did I misunderstand the feature request in my quick read through the issue?

I believe that your more recent comments are going into other examples, and workarounds you're attempting due to the lack of your desired feature. However, in the context of defining the feature request, I think they're not contributing to the requirements of the feature

@ghost ghost added WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. and removed WaitingForCustomer Applied when a NuGet triage person needs more info from the OP labels Aug 28, 2023
@zivkan zivkan added WaitingForCustomer Applied when a NuGet triage person needs more info from the OP and removed WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. labels Aug 28, 2023
@EdLichtman
Copy link

My more recent examples/ workarounds are just that - workarounds, or an alternative in case you guys don't feel that this is ok because of some sort of security concern. I believe you got the gist though, that issue you linked is most definitely what I'm looking for.

@ghost ghost added WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. and removed WaitingForCustomer Applied when a NuGet triage person needs more info from the OP labels Aug 28, 2023
@zivkan zivkan added the Resolution:Duplicate This issue appears to be a Duplicate of another issue label Aug 28, 2023
@zivkan
Copy link
Member

zivkan commented Aug 28, 2023

Closing as a duplicate of #6938. Be sure to add a 👍 reaction to the first comment in that issue, so that it bubbles up the list of issues sorted by 👍

@zivkan zivkan closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2023
@ghost ghost removed the WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionality:Pack Functionality:Restore Resolution:Duplicate This issue appears to be a Duplicate of another issue Transferred issue This issue is transferred from VSFeedback or other github repo Type:Feature
Projects
None yet
Development

No branches or pull requests

3 participants