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

Use maven-model to parse the pom.xml and calculate paths #2181

Merged
merged 56 commits into from
Mar 20, 2025

Conversation

DavyLandman
Copy link
Member

@DavyLandman DavyLandman commented Mar 7, 2025

This makes us more flexible and allows us to detect errors in the pom, and maybe fix them with projects in the workspace.

It also removes a whole set of maven dependencies that we recently accepted.

todo:

  • some test pom.xmls and junit test around them
  • download from the configured repositories
  • check the checksums while/after downloading for a minimum level of authenticity, and absence of download bitrot: Native pom parsing/checksum #2184
  • properly generate offsets & lengths based on the offset & lenght
  • make sure we store in the local maven repository in the correct way
  • calculate the class path in case of conflicting dependencies
  • deal with null version numbers, that maven somehow resolves
  • check how we're not racing a download with maven, it might do some locking on the file level?
  • remove as many dependencies on maven execution API from this pom.xml (to enable the rascal-maven-plugin)
  • remove the unshaded variant of rascal, as this should not be necessary anymore

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 63.85350% with 227 lines in your changes missing coverage. Please review.

Project coverage is 46%. Comparing base (c81f775) to head (bde7e38).
Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
src/org/rascalmpl/library/util/PathConfig.java 30% 40 Missing and 8 partials ⚠️
src/org/rascalmpl/util/maven/MavenParser.java 61% 35 Missing and 11 partials ⚠️
src/org/rascalmpl/util/maven/Artifact.java 69% 23 Missing and 4 partials ⚠️
src/org/rascalmpl/util/maven/Dependency.java 57% 17 Missing and 5 partials ⚠️
...scalmpl/util/maven/SimpleRepositoryDownloader.java 76% 15 Missing and 5 partials ⚠️
src/org/rascalmpl/util/maven/Util.java 62% 11 Missing and 8 partials ⚠️
src/org/rascalmpl/util/maven/SimpleResolver.java 78% 9 Missing and 3 partials ⚠️
...c/org/rascalmpl/util/maven/ArtifactCoordinate.java 70% 10 Missing and 1 partial ⚠️
.../org/rascalmpl/util/maven/ChecksumInputStream.java 75% 7 Missing ⚠️
.../rascalmpl/util/maven/SimpleWorkspaceResolver.java 68% 4 Missing and 2 partials ⚠️
... and 4 more
Additional details and impacted files
@@           Coverage Diff            @@
##              main   #2181    +/-   ##
========================================
  Coverage       46%     46%            
- Complexity    6074    6214   +140     
========================================
  Files          748     758    +10     
  Lines        62518   63070   +552     
  Branches      9338    9419    +81     
========================================
+ Hits         29170   29559   +389     
- Misses       31176   31310   +134     
- Partials      2172    2201    +29     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

var loc = context.pom;
var artifactLoc = me.getLocation("artifactId");
if (artifactLoc != null) {
loc = IRascalValueFactory.getInstance().sourceLocation(loc , 0, 0, artifactLoc.getLineNumber(), artifactLoc.getColumnNumber(), artifactLoc.getLineNumber(), artifactLoc.getColumnNumber() + 1);
Copy link
Member

Choose a reason for hiding this comment

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

This is broken. Source locations with 0,0 as offset and length are never queried for line and column information. The two have to align; as they are intended to be redundant information.

if (artifactLoc != null) {
loc = IRascalValueFactory.getInstance().sourceLocation(loc , 0, 0, artifactLoc.getLineNumber(), artifactLoc.getColumnNumber(), artifactLoc.getLineNumber(), artifactLoc.getColumnNumber() + 1);
}
messages.append(Messages.warning("I could not resolve dependency in maven repository: " + me.getGroupId() + ":" + me.getArtifactId() + ":" + me.getVersion(), loc));
Copy link
Member

Choose a reason for hiding this comment

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

The use of "I" is a style breach for rascal exceptions and error messages. "Rascal" would be better or "Rascal dependency resolution could not..."

Copy link
Member

@jurgenvinju jurgenvinju left a comment

Choose a reason for hiding this comment

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

An enormous jump in a short time. It's a lot of code but it makes all our lives a lot simpler.

Copy link
Member

@jurgenvinju jurgenvinju left a comment

Choose a reason for hiding this comment

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

I didn't see any checksum checking in this code; which is a minimum level of "security" checking for transport correctness and the absence of a man-in-the-middle attack. Of course this is not "secure" in any serious way, but it is a "bottom bar" to jump over.

@DavyLandman
Copy link
Member Author

Yes, we'll add checksum support. The PR is still in draft mode, there are about 10 TODOs in the code, and we also have to move the old code away.

I will however note that the checksum feature is primarily for detecting corrupted downloads. HTTPS is a better protection against mitm. Especially since the checksums aren't signed, so any MITM can also just rewrite the checksum.

@jurgenvinju
Copy link
Member

I am running against mvn jar loading issues in rascal-maven-plugin. @rodinaarssen and I think that our lives will be easier once the current PR is merged. Then we have fewer maven-x projects to depend on (and shaded) and we hope that will resolve the complex issues we are running into. Groetjes! See usethesource/rascal-maven-plugin#28

DavyLandman and others added 24 commits March 13, 2025 16:58
…epo-from-settings

Retrieve local repo from system property or settings.xml when available
…le-improvement

Using a temp file in the same directory as the target
…scope

Implemented system scope resolving
@DavyLandman DavyLandman marked this pull request as ready for review March 20, 2025 14:17
@DavyLandman DavyLandman merged commit 44c357a into main Mar 20, 2025
8 checks passed
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.

3 participants