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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gotztibor
Copy link

Please see #Issue847

@mtsfoni
Copy link
Contributor

mtsfoni commented Feb 6, 2024

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.

@gotztibor
Copy link
Author

gotztibor commented Feb 7, 2024

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!

@mtsfoni
Copy link
Contributor

mtsfoni commented Feb 7, 2024

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).

Tibor Götz added 3 commits February 9, 2024 18:11
…xcluded.

Signed-off-by: Tibor Götz <tibor.gotz.ext@siemens-healthineers.com>
Signed-off-by: Tibor Götz <tibor.gotz.ext@siemens-healthineers.com>
…y remove them.

Signed-off-by: Tibor Götz <tibor.gotz.ext@siemens-healthineers.com>
@gotztibor
Copy link
Author

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!

@mtsfoni mtsfoni self-requested a review February 11, 2024 21:25
… the problem with privatAssets.

Signed-off-by: MTsfoni <mibau89@gmail.com>
Signed-off-by: MTsfoni <mibau89@gmail.com>
Signed-off-by: MTsfoni <mibau89@gmail.com>
@mtsfoni
Copy link
Contributor

mtsfoni commented Feb 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants