-
Notifications
You must be signed in to change notification settings - Fork 252
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
Comments
This doesn't sound right--@EdLichtman can you give an example of that happening? |
I'm using this target: 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. |
Issue is missing Type label, remember to add a Type label |
@rainersigwald 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" And yet, when I build DependencyHellRecipient, I don't receive the error I'm expecting to see from the buildTransitive file |
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" |
@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 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 |
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. |
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 👍 |
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.
The text was updated successfully, but these errors were encountered: