NuGet strategy consolidation #1461
Conversation
There was a problem hiding this comment.
I think we need some tests that test the new behaviour works and actually runs the correct strategy depending on whether a project.assets.json file or a .csproj file is found during discovery.
We should have a test plan that tests these two cases as well, so we can manually validate that this works.
docs/features/manual-dependencies.md
Outdated
| - `cargo` - Rust dependencies that are typically found at [crates.io](https://crates.io/). | ||
| - `carthage` - Dependencies as specified by the [Carthage](https://github.com/Carthage/Carthage) package manager. | ||
| - `composer` - Dependencies specified by the PHP package manager [Composer](https://getcomposer.org/), which are located on [Packagist](https://packagist.org/). | ||
| - `cpan` - Dependencies located on the [CPAN package manager](https://www.cpan.org/). | ||
| <!-- markdown-link-check-disable-next-line --> | ||
| - `cran` - Dependencies located on the [CRAN](https://cran.r-project.org/) like repository. |
There was a problem hiding this comment.
Any reason to re-enable these? I think they were disabled because they were flaky in the past
There was a problem hiding this comment.
Not knowing a reason to re-enable, I've added these back
|
|
||
| ## Requirements | ||
| | Strategy | Direct Deps | Transitive Deps | Edges | |
There was a problem hiding this comment.
[nit] Can you prettify these markdown tables?
I do it through the Markdown Table Prettifier extension: https://marketplace.visualstudio.com/items?itemName=darkriszty.markdown-table-prettify
Just install that, and then "command-P prettify markdown tables" in each markdown doc
There was a problem hiding this comment.
Thanks for the extension link; done and done
src/Strategy/NuGet.hs
Outdated
| import Strategy.NuGet.PackageReference qualified as PackageReference | ||
| import Strategy.NuGet.PackageReference qualified as ProjectAssetsJson |
There was a problem hiding this comment.
You're importing the same package twice but calling it different things. Shouldn't the second one of these be imported from Strategy.NuGet.ProjectAssetsJson?
There was a problem hiding this comment.
I've picked this up and am working from the assumption that this was a typo and meant to be Strategy.NuGet.ProjectAssetsJson
src/Strategy/NuGet.hs
Outdated
| analyzeProjectStaticOnly _ = getDeps | ||
|
|
||
| getDeps :: (Has ReadFS sig m, Has Diagnostics sig m) => NuGetProject -> m DependencyResults | ||
| getDeps project = context "NuGet" (getAssetsJsonDeps project <||> getPackageReferenceDeps project) |
There was a problem hiding this comment.
It feels weird that we're just running getAssetsJsonDeps on the file even if it's not named project.assets.json. Should we switch on the filename, or add a new field to the Nuget type that tells us which type of analysis we should be doing?
There was a problem hiding this comment.
I changed this to call getAssetsJsonDeps only on project.assets.json. I think that we should probably rely on the file in question living where it does to avoid some perverse edge case like "this other JSON file I found is a valid project.assets.json file but dotnet will not treat it as such. I hold this opinion lightly, will revert if desired.
james-fossa
force-pushed
the
nuget-strategy
branch
2 times, most recently
from
ecfcc6a to
cb7b6e4
Compare
…led project.assets.json
james-fossa
force-pushed
the
nuget-strategy
branch
from
678d056 to
13cf642
Compare
docs/features/manual-dependencies.md
Outdated
| @@ -54,6 +54,7 @@ Supported dependency types: | |||
| - `pub` - Dart dependencies found at [pub.dev](https://www.pub.dev/). | |||
| - `pypi` - Python dependencies that are typically found at [Pypi.org](https://pypi.org/). | |||
| - `swift` - Swift dependencies using the [Swift Package Manager](https://www.swift.org/package-manager/). | |||
| <!-- markdown-link-check-disable-next-line --> | |||
There was a problem hiding this comment.
Okay, now I'm reversing my previous comment. This comment is not on current master, and I don't think this PR should change that
| @@ -0,0 +1,17 @@ | |||
| # Quick reference: NuGet | |||
There was a problem hiding this comment.
This file is new in this PR and not referenced anywhere in the documentation. There are other <language>-qr.md files in the repo, but I think it's confusing to have both nuget.md and nuget-qr.md.
So, I know you didn't add it, but I think we should delete it
| @@ -38,7 +55,20 @@ testServiceStackForPkgConfig = | |||
| it "should find targets" $ \(result, _) -> do | |||
| length result `shouldBe` 4 | |||
|
|
|||
| testDotnetCoreTwoExampleForPackageAssetsJson :: Spec | |||
There was a problem hiding this comment.
Should this be named testDotnetCoreTwoExampleForProjectAssetsJson?
james-fossa
force-pushed
the
nuget-strategy
branch
from
0507649 to
10b30a5
Compare
…qr.md, fix a Nuget spec name
Overview
All of the CLI’s nuget strategies run everytime fossa analyze is run. This causes duplicate information to be uploaded on almost every run. The worst part of this is that one of the strategies analyzes a lockfile with determined versions and the other analyzes a manifest with versions that may not be present in the final build.
From Nugets website
The project.json file maintains a list of packages used in a project, known as a package management format. It supersedes packages.config but is in turn superseded by PackageReference with NuGet 4.0+.
https://fossa.com/blog/managing-dependencies-net-csproj-packagesconfig/
Acceptance criteria
Nuget analysis strategies are not all run and utilize fallbacks to ensure that duplicate dependencies are not uploaded.
Implementation decisions here do not prevent https://fossa.atlassian.net/browse/ANE-702 from being implemented.
Consolidate analysis for project.assets.json and package reference files because they have the same entry point, .csproj files. The other strategies can remain the same and be ran independently.
Documentation is updated to display what the fallback order is.
Testing plan
Risks
[Question] - Was looking into writing an integration test to test the case where we have both
.csprojfiles (triggers PackageReference strategy) andproject.assets.jsonfiles, that way we can test out the consolidation changes in this pr. After looking at our integration tests, it looks like we test against open source repos , the issue withproject.assets.jsonfiles is that they are not typically included in open source repos as they are generated automatically by the .NET SDK during the restore process. We also didn't have an existing integration test for theproject.assets.jsonstrategy either. I feel like we should still have a test for this but am unsure on the best way to go about it. Should I just clone a repo, generate aproject.assets.jsonfile and push those changes to a new repo so that we can test against for our tests?Metrics
References
Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an# Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.