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.

Sign up for GitHub

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

Jump to bottom

Adding TargetFrameworks to the list of data retreived from NuGet packages #365

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Adding TargetFrameworks to the list of data retreived from NuGet packages #365

wants to merge 2 commits into from

Conversation

JamieMagee
Copy link
Member

Successor to #79

Original PR description:

Per a conversation about adding target framework information to the component detection data, this is a PR that shows what that could look like.

Things get slightly tricky here since packages declare target frameworks through the dependency declarations in the nuspec, as well as the folder structure of the package. This hasn't been through much testing beyond the unit tests, but they've been augmented to test some potential patterns.

I mainly want to get some feedback on this before I spend much more time on testing. I can potentially test this against all NuGet.org packages pretty easily, which also gives us a test oracle since they calculate target frameworks as well.

Things that might need consideration:

  • Addition of NuGet.Packaging as a dependency
  • how the data is plumbed through the NuGetComponent (I followed the Authors example as an array of >additional strings)
  • Removal of async from ProcessFile (nothing in there was async when I finished)
  • Shifting more responsibility to NuGetNuspecUtilities even through it's not strictly nuspec-related >anymore.
  • Using the nuspec reader rather than bespoke XML parsing

I'm open to any and all feedback here.

@marklio @JamieMagee
Adding TargetFrameworks to the list of data retreived from NuGet pack…
9a11767 
…ages.

* Take dependency on NuGet.Packaging rather than re-implement nuget logic
* Add TargetaFrameworks to NuGetComponent
* Refactor NuGetNuspecUtilities to gather target framework data
  * Take IComponentStream instead of raw stream, so we have extra data associated with the component and for consistency in the APIs.
  * Return ALL the data from these utilities via tuples instead of just the bytes of the nuspec
  * For loose nuspec files, consider their location to be an unzipped package, and gather target framework from the sibling directories
  * For nupkgs, calculate target frameworks from the other entries in the zip file
  * Use official NuspecReader instead of bespoke nuspec parsing
@JamieMagee JamieMagee requested a review from a team as a code owner November 15, 2022 20:04
@JamieMagee JamieMagee mentioned this pull request Nov 15, 2022
Closed
marklio
marklio reviewed Nov 15, 2022
View reviewed changes
src/Microsoft.ComponentDetection.Detectors/nuget/NuGetNuspecUtilities.cs
}
}

public static (string Name, string Version, string[] Authors, HashSet<string> TargetFrameworks) GetNuspecDataFromNuspecStream(Stream sourceStream)
Copy link

Choose a reason for hiding this comment

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

There is a question of whether we should assume a directory containing a .nuspec file is an expanded .nupkg. There may be adjacent directories and subdirectories that could contain relevant target framework data that perhaps should be analyzed as in GetNuGetPackageDataFromNupkg above.

@marklio
Copy link

marklio commented Nov 15, 2022

Thanks for rebasing/bumping this, @JamieMagee ! When I first wrote this, this was very much intended to be a draft to start a conversation on how target framework info could be added to component detection. I'm very happy to engage in whatever capacity is most helpful in seeing progress here.

@github-actions
Copy link

github-actions bot commented Jan 4, 2023

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

@SokoFromNZ
Copy link

Hi guys
Any progress on this in the near future? I actually would love to read more properties from the .nupkg/.nuspec...

  • copyright
  • urls
  • licencse
  • authors
  • ...basically all the main props in a nuspec

@cobya
Copy link
Contributor

cobya commented Nov 30, 2023

@JamieMagee is this still needed?

@SokoFromNZ
Copy link

It definitely is needed by me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marklio marklio marklio left review comments

@grvillic grvillic Awaiting requested review from grvillic grvillic is a code owner automatically assigned from microsoft/ose-component-detection-maintainers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JamieMagee @marklio @SokoFromNZ @cobya