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

Add NuGet source #261

Merged
merged 10 commits into from
May 15, 2020
Merged

Add NuGet source #261

merged 10 commits into from
May 15, 2020

Conversation

zarenner
Copy link
Contributor

@zarenner zarenner commented May 7, 2020

Add NuGet / dotnet as a supported package source type.

There are a number of open questions remaining since NuGet is somewhat unique in terms of the number of different license mechanisms it has, the most common being fetching licenses from a URL.

FYI this is the first Ruby I've ever written, so I apologize in advance for any atrocities 😃

@zarenner
Copy link
Contributor Author

zarenner commented May 7, 2020

A large number of Microsoft packages that any ASP.NET Core project will likely use have a specific, not SPDX recognized license. Is there any way to have this license show up as something different than other so that they don’t have to manually be reviewed? It looks like anything not in choosealicense is consider an invalid license.

Copy link
Contributor

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

❤️ thanks for opening this!

It's been awhile since I've done any .net development. Is this implementation specific to any specific .net version or is it a general solution that works for anything that uses nuget?

@zarenner
Copy link
Contributor Author

zarenner commented May 8, 2020

❤️ thanks for opening this!

It's been awhile since I've done any .net development. Is this implementation specific to any specific .net version or is it a general solution that works for anything that uses nuget?

This implementation works for all .NET Core projects, and newer .NET Framework projects that use PackageReference, where nuget resolves the package graph during restore and produces files indicating where the packages were downloaded to.

It will not work for older .NET Framework projects that use the packages.config mechanism. Implementing support for packages.config here is quite difficult as it's harder to find the packages on disk without parsing MSBuild.

Copy link
Contributor

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

A bunch of comments, some related to ruby idioms and some not.

The only blocker that I'm requesting changes on is that the source should evaluate the entire dependency source tree. The logic and usage of excluded_project? will prevent that.

Copy link
Contributor

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

Sorry for keeping this blocked, I'd like to make sure I understand your use case so that we're not (re)building unnecessary or potentially dangerous functionality into the product. How time sensitive is adding this source for you?

@jonabc
Copy link
Contributor

jonabc commented May 15, 2020

Ignore the failing cabal tests, it's a common occurrence that something in a new cabal release breaks the setup script 😢 . I'll get around to fixing it when I have some free time.

@jonabc
Copy link
Contributor

jonabc commented May 15, 2020

I've updated the test GH Action workflow to fix the failing cabal builds, they should be back to ✅ after updating from master.

Copy link
Contributor

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

✨ looks great

Comment on lines +176 to +203
def enumerate_dependencies
json = JSON.parse(project_assets_file)
nuget_packages_dir = json["project"]["restore"]["packagesPath"]
json["targets"].each_with_object({}) do |(_, target), dependencies|
target.each do |reference_key, reference|
# Ignore project references
next unless reference["type"] == "package"
package_id_parts = reference_key.partition("/")
name = package_id_parts[0]
version = package_id_parts[-1]
id = "#{name}-#{version}"

# Already know this package from another target
next if dependencies.key?(id)

path = File.join(nuget_packages_dir, json["libraries"][reference_key]["path"])
dependencies[id] = NuGetDependency.new(
name: id,
version: version,
path: path,
metadata: {
"type" => NuGet.type,
"name" => name
}
)
end
end.values
end
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this would be easier to read and could avoid the dangling .values if this was broken up into separate methods that (1) gathered metadata and (2) mapped metadata to NugetDependency.

not blocking, 👍 to clean this up later so this source can be released.

@zarenner zarenner merged commit 2be11a6 into licensee:master May 15, 2020
@zarenner zarenner deleted the zarenner/nuget branch May 15, 2020 20:42
@jonabc jonabc mentioned this pull request May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants