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: improve resolving of maven based dependencies #7981

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

netomi
Copy link

@netomi netomi commented Dec 1, 2023

This fixes #7965 .

Opening a draft PR to dicuss the speed improvements.

The idea of this change is as follow:

  • let the pom file being resolved initially so that it is present in the local maven repository
  • resolve the pom file of the artifact using resolveArtifact which will take advantage of the local repo and gives information from which repo it was resolved
  • use the resolve repository to retrieve the checksum files for the binary / source artifacts

With this changes it works pretty fast. There are some assumptions that might not be ok for the general case:

  • it is assumed that sources are located in the same repository as the binary artifact itself, imho this is a safe assumption to make
  • when a checksum file is present it is assumed that the artifact is also present (otherwise the default resolution mechanism of maven would be broken imho)
  • if no checksum is found for an artifact, it is checked if the artifact exists by doing a PeekTask and returning an empty RemoteArtifact if it fails

As mentioned in the related ticket, this mode could be made optional so that a user explicitly needs to enable that mode. For isolated cases like when running as GitHub Action, I would make this the default behavior as you gain nothing by the current way of determining the repo while having considerable slowdowns.

Copy link

codecov bot commented Dec 2, 2023

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (d785b4f) 66.99% compared to head (39f1284) 66.46%.
Report is 55 commits behind head on main.

Files Patch % Lines
...nagers/maven/src/main/kotlin/utils/MavenSupport.kt 0.00% 44 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7981      +/-   ##
============================================
- Coverage     66.99%   66.46%   -0.54%     
+ Complexity     2041     2038       -3     
============================================
  Files           356      357       +1     
  Lines         17103    17297     +194     
  Branches       2443     2502      +59     
============================================
+ Hits          11459    11496      +37     
- Misses         4623     4768     +145     
- Partials       1021     1033      +12     
Flag Coverage Δ
test 36.36% <0.00%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@netomi
Copy link
Author

netomi commented Dec 2, 2023

gradle tests are failing, need to look into that, did mainly test with maven projects so far.

Ok, so in case of gradle, we can not rely on the fact that the remote repository will be returned as this information is not present in a gradle cache it seems, will update the PR accordingly.

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.

Improve analysis of maven based projects
1 participant