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

feat (dotnet support) complete dotnet support with fallbacks. #450

Merged
merged 12 commits into from Apr 29, 2019

Conversation

zlav
Copy link
Member

@zlav zlav commented Apr 26, 2019

Add complete support for .NET.
This closes #173, closes #172, closes #378, closes #408.

Significant changes:

  1. Additional analysis methods supported.
  2. Fully supported fallbacks that check for valid dependency graphs.
  3. Improved discovery which prioritizes certain files over others. Ideally this would be handled by the Analyze() function, but discovery is very difficult to do inline with analysis for package reference files.
  4. Strategies and corresponding structs for unmarshalling were broken into their own files.

Todo

  • cli documentation
  • development documentation

I also need to determine if the type switch in project_json.go is the correct way to do things.

@codecov

This comment has been minimized.

@codecov

This comment has been minimized.

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #450 into master will increase coverage by 2%.
The diff coverage is 79.79%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #450    +/-   ##
========================================
+ Coverage   49.71%   51.71%    +2%     
========================================
  Files          83       91     +8     
  Lines        4385     4654   +269     
========================================
+ Hits         2180     2407   +227     
- Misses       1990     2014    +24     
- Partials      215      233    +18
Impacted Files Coverage Δ
analyzers/nuget/analyze.go 40.74% <39.21%> (ø)
buildtools/dotnet/package_reference.go 76.27% <76.27%> (ø)
analyzers/nuget/nuget.go 51.42% <88.88%> (ø)
buildtools/dotnet/project_assets.go 89.47% <89.47%> (ø)
buildtools/dotnet/packages_config.go 90.9% <90.9%> (ø)
buildtools/dotnet/nuspec.go 91.3% <91.3%> (ø)
buildtools/dotnet/project_json.go 92.3% <92.3%> (ø)
buildtools/dotnet/dotnet.go 85.71% <95.65%> (ø)
analyzers/php/php.go
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a24f68...5c7631d. Read the comment docs.

@zlav zlav requested review from dchenk and cnr April 26, 2019 17:24
Copy link
Contributor

@dchenk dchenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a very thorough job, a good example of what a fossa-cli solution for a language ecosystem could look like. :)

I know you didn't implement NuGet support initially, but with these improvements I think the docs in docs/integrations/nuget.md should be updated too. In particular, we currently say:

NuGet support in FOSSA CLI depends on the following tools existing in your environment:

- [.NET Core CLI](https://docs.microsoft.com/en-us/dotnet/core/tools/) (defaults to `dotnet`, configure with `$DOTNET_BINARY`)
- [NuGet](https://www.nuget.org/downloads) (defaults to `nuget`, configure with `$NUGET_BINARY`)

Yet the CLI apparently doesn't use the nuget binary or the NUGET_BINARY environment variable anywhere.

Similarly, if we're going to let users manually specify a resolution strategy (https://github.com/fossas/fossa-cli/pull/450/files#diff-9b9e9c38147534ae1a6273a90dd4e969R22), we should document this somehow, e.g.:

You can specify which dependency resolution tool the CLI should use by
adding a `strategy: strategy_name` property under `options` in your `.fossa.yml`.
For example:
analyze:
  modules:
  - name: dn1.csproj
    type: nuget
    target: dn1.csproj
    path: .
    options:
      strategy: paket

I'm not even sure if that's correct.

Perhaps the nuget.md doc should be renamed to dotnet.md and list nuget as just one of the deps manager. Again, I don't know this ecosystem well enough, so maybe I'm not making sense.

analyzers/nuget/analyze.go Outdated Show resolved Hide resolved
analyzers/nuget/analyze.go Outdated Show resolved Hide resolved
analyzers/nuget/nuget.go Outdated Show resolved Hide resolved
analyzers/nuget/nuget.go Show resolved Hide resolved
@zlav
Copy link
Member Author

zlav commented Apr 27, 2019

@dchenk your comments are spot on about cli documentation, previously we checked for the existence of the dotnet binary but I removed part of that functionality because we weren't actually using it to determine dependencies. You are correct about how to specify options as well, thanks for the feedback!

@zlav zlav requested a review from dchenk April 29, 2019 18:18
Copy link
Contributor

@dchenk dchenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great overhaul of support for .NET and improvement of the docs. :)

@zlav zlav merged commit bfdfaa1 into master Apr 29, 2019
@zlav zlav deleted the feat/dotnet-improvements branch September 11, 2019 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants