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: add transitive extraction for Maven pom.xml #399

Closed
wants to merge 23 commits into from

Conversation

cuixq
Copy link
Collaborator

@cuixq cuixq commented Jan 17, 2025

This PR adds a new pomxmlnet extractor to first parse pom.xml then perform dependency resolution to extract both direct and transitive dependencies. Most code are migrated from OSV-Scanner but switch to use the virtual file system.

To achieve this goal, this PR also adds some internal packages:

  • mavenutil: Maven utilities to help parse and resolve dependencies
  • datasource: clients to talk to registries for package and version information
  • resolution: clients (and tests) needed for dependency resolution

We still need to figure out how to registry this extractor to the list and only enable it with some flags.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

LGTM with a quick pass.

@cuixq cuixq marked this pull request as ready for review January 21, 2025 00:04
@cuixq cuixq requested a review from erikvarga January 21, 2025 00:08
@erikvarga
Copy link
Collaborator

Mostly had a couple of style-related comments (consistency with rest of OSV-SCALIBR and google-internal linters), I'll defer to Rex for the functional parts of the migration from the OSV-Scanner.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Added a bunch of comments, a lot of them are probably from a lack of domain knowledge about Maven, feel free to ignore the ones that don't make sense.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

LGTM

copybara-service bot pushed a commit that referenced this pull request Feb 4, 2025
PiperOrigin-RevId: 722906889
@G-Rath
Copy link
Collaborator

G-Rath commented Feb 4, 2025

fyi this bumps the codebase to Go 1.23 which technically goes against the "2 latest versions" policy we're following in osv-scanner and that I proposed bring over in #383 - that was going to get landed later this month once Go 1.24 comes out

@G-Rath G-Rath mentioned this pull request Feb 4, 2025
@G-Rath
Copy link
Collaborator

G-Rath commented Feb 4, 2025

oh I just realised this PR is actually still open, but it's also in main - @cuixq was that actually intentional or has something gone wrong with copybara?

@G-Rath G-Rath mentioned this pull request Feb 4, 2025
@another-rex
Copy link
Collaborator

We're leaving this open to see if copybara will actually close it automatically when it does garbage collection 😅 . Doesn't look like it so far...

@erikvarga
Copy link
Collaborator

erikvarga commented Feb 7, 2025

We're leaving this open to see if copybara will actually close it automatically when it does garbage collection 😅 . Doesn't look like it so far...

Copybara might not handle commits well that we had to patch and merge ourselves. I'd say we can just close this manually.

@erikvarga erikvarga closed this Feb 7, 2025
@cuixq cuixq deleted the transitive branch February 10, 2025 00:53
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.

6 participants