cyclonedx-dotnet icon indicating copy to clipboard operation
cyclonedx-dotnet copied to clipboard

#Issue 847 - Fix runtime dependencies if developer dependencies are excluded

Open gotztibor opened this issue 1 year ago • 5 comments

Please see #Issue847

gotztibor avatar Feb 06 '24 15:02 gotztibor

HasRuntimeParts looks good as far as it actually works (didn't test it).

But I have a problem with ResolvedRuntimeDependencies. I actually just removed the excludeDevDependencies flag from those two services, with the goal of making the process of gathering the dependencies independent of the arguments.

If I understand correctly, you remove all dependencies, that are only dependent on dev-dependencies. Which was actually requested here.

However, at the step of gathering the dependencies the reasonable action should be, to just mark those transitive dev-dependencies as dev-dependencies.

How to handle them is still open - there seem to be different opinions ranging from, they actually should be part of the SBOM (as component or as formulation), to they should be marked as scope.excluded (which seem objectively wrong) to they should not be in the SBOM. Long story short, it is not the right position to delete them.

mtsfoni avatar Feb 06 '24 15:02 mtsfoni

Dear Michael, finally I applied the proposal You made, and I am now marking only dependencies to be dev dependencies instead of forcibly removing them. Actually I think, my solution also covers the problem of the mentioned case here.

The serious case, because of I started to jump on the code and check how developer dependencies are handled was, that in case of we have more fine-graned control on the package assets with (ExcludeAssets,PrivateAssets,IncludeAssets) I experienced the following:

  • some of the references libraries were falling out (into dev dependencies), which were actually hard runtime dependencies (available in the bin, publish folder) eg. PrivateAssets="analyzers;build",
  • some of them were additionally falling in, which were really not runtime dependencies at all

Please would You be so kind to check again the current solution proposal? The way now is working:

  • end of the dependency resolution it is recursively traversing through the package dependencies (starting from the direct one's as roots) collecting the "real-hard-runtime" dependencies which will be present in the bin,publish folders
  • all of the dependencies excluding the above will be marked as developer dependencies
  • with this solution also the direct and indirect dependencies (even if the same package) is handled

Cases:

  • D is a developer dependency, as that is reachable only throughout a dev dependency
  B (dev) - D
 /
A
  • D is not a dev dependency as that will be part of the hard-runtime dependencies through the A -C -D path
  B (dev) - D
 /
A -  C  - D
  • D is a dev dependency, because there is no path from root to it without visiting a dev dependency
  B (dev) - D
 /
A -  C (dev) - D

Really-really thanks a lot in advance!

gotztibor avatar Feb 07 '24 01:02 gotztibor

At the first glance it looks good. I will test the code in the weekend.

I will have a few nitpicky change request regarding style.

Also, I'll need you to sign-off your contribution for legal reason. Here it describes pretty good how you can do it retrospectively for your commits. However, you can just do this when everything else is done.

I will try to get your code released quickly. Should be in the next 2 weeks, hopefully.

Thanks for the contribution, especially as it took a little work off my back (there is still a lot to do for this tool).

mtsfoni avatar Feb 07 '24 16:02 mtsfoni

Dear Michael,

sorry for my limited knowledge, but please could You share with me the proposals about the style, because this case I try to commit those changes/adaptations as well.

Really-really thanks a lot for Your efforts in advance! Thanks for helping me out!

gotztibor avatar Feb 09 '24 17:02 gotztibor

Hey Götz, there is one aspect that isn't fully clear to me. We have the private asset:

<!-- runtime true -->
<PackageReference Include="Serilog" Version="3.1.1" PrivateAssets="all" IncludeAssets="runtime" />    

Private asset means to my understanding, please correct me if I am wrong, it belongs to the output of this project, but other project referring this project (via package- or projectReference) will not output this private asset.

So by that understanding, generally your comment <!-- runtime true --> would apply. However, the code currently marks serilog as development dependency.

But there is an extra twist: What if the SBOM is generated for a NuGet-(library)-package? Will the private asset be part of the package or not? Because if it isn't, marking it as development dependencies would be correct again. In that cycloneDX might need a way to just mark it as a private dependency and then figure out later (maybe by the cli-arguments) if it is a dev dependency or not.

So I modified the test cases and added one extra test case to your branch. Currently, on them is failing because serilog is being excluded (it appears in the /bin folder of the project).

By the way: forget about the nitpicks, as I already checked the branch out, I will just do them by myself later. It's just about preference. For example, here I would just prepare a return value for more clarity instead of changing the contents of the list inside the function.

mtsfoni avatar Feb 11 '24 22:02 mtsfoni