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

Replace lib:/// scheme by mvn:/// and file:/// and jar+file:/// everywhere #1969

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

Conversation

jurgenvinju
Copy link
Member

@jurgenvinju jurgenvinju commented Jun 10, 2024

This PR:

  • removes the implementation and use of the lib:/// scheme. This also removes a lot of printing on stderr.
  • builts on the introduction of the mvn:/// scheme
  • moves the original resolution code from the lib:/// scheme implementation to PathConfig.createProjectConfig...
  • adds path resolution error messages and warnings to a PathConfig field called messages, including version clashes on the rascal and rascal-lsp project, missing pom.xml files, etc.
  • changes all clients of PathConfig.createProjectConfig to pretty print resolved paths and error messages.

These were the TODO's:

  • shorten all paths into the local maven repo to mvn:///groupid!artifactid!version
  • replace lib:/// in the processing of dependencies (Require-Libraries) with other schemes, including mvn, file, and jar+file. This means sometimes dynamically computing an absolute path rather than leaving it to an opaque scheme.
  • remove Require-Libraries implementation (is replaced by mvn dependency:list and bundle dependencies in Eclipse)
  • remove lib resolver from the implementation
  • test, test, test
  • figure out how the rascal summaries for the IDE can work without the lib:/// scheme that is now introduced by the Packager. Some form of relocation is necessary, but it can't be the old lib:/// scheme anymore.

This fixes #1916, #1961, #1826, #1825, #1824, #1777, #1767, and #1478

Downstream projects like rascal-lsp and rascal-maven-plugin may need some modifications:

  • In particular everywhere we use URIUtil.correctLocation("lib", name) should become PathConfig.resolveDependencyFromResourcesOnCurrentClasspath("name") or URIUtil.correctLocation("mvn", etc.)
  • Where we receive a new PathConfig instance we can chose to register or print the diagnostic messages.

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 40.30303% with 197 lines in your changes missing coverage. Please review.

Project coverage is 49%. Comparing base (1ce9e59) to head (4597800).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/org/rascalmpl/library/util/PathConfig.java 37% 103 Missing and 19 partials ⚠️
src/org/rascalmpl/library/Messages.java 51% 28 Missing and 4 partials ⚠️
...rg/rascalmpl/interpreter/utils/RascalManifest.java 9% 10 Missing ⚠️
src/org/rascalmpl/shell/ShellEvaluatorFactory.java 0% 8 Missing ⚠️
...rascalmpl/uri/file/MavenRepositoryURIResolver.java 50% 4 Missing and 3 partials ⚠️
src/org/rascalmpl/uri/URIUtil.java 50% 2 Missing and 3 partials ⚠️
src/org/rascalmpl/shell/CommandOptions.java 0% 4 Missing ⚠️
src/org/rascalmpl/library/Prelude.java 0% 2 Missing ⚠️
src/org/rascalmpl/library/util/Eval.java 0% 2 Missing ⚠️
.../org/rascalmpl/uri/StandardLibraryURIResolver.java 81% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@           Coverage Diff            @@
##              main   #1969    +/-   ##
========================================
  Coverage       49%     49%            
  Complexity    6302    6302            
========================================
  Files          664     664            
  Lines        59632   59572    -60     
  Branches      8647    8637    -10     
========================================
+ Hits         29476   29507    +31     
+ Misses       27944   27843   -101     
- Partials      2212    2222    +10     

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

@jurgenvinju
Copy link
Member Author

jurgenvinju commented Jun 27, 2024

tests to run

  • rascal project tests
  • flybytes project (lots of Java jar dependencies)
  • salix project
  • drambiguity project (depends on salix)

@jurgenvinju
Copy link
Member Author

This PR fixes #1961

@jurgenvinju jurgenvinju self-assigned this Sep 16, 2024
@jurgenvinju jurgenvinju marked this pull request as ready for review September 16, 2024 13:29
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

I've taken a first look at the code, not sure why everything is happening yet, but it seems like this will cause a whole downstream of changes and issues, so we're going to have to write down some new documentation of how it is supposed to be used I think.

src/org/rascalmpl/library/Messages.java Show resolved Hide resolved
src/org/rascalmpl/library/Messages.java Outdated Show resolved Hide resolved
src/org/rascalmpl/library/util/PathConfig.java Outdated Show resolved Hide resolved
src/org/rascalmpl/library/util/Reflective.rsc Show resolved Hide resolved
src/org/rascalmpl/uri/StandardLibraryURIResolver.java Outdated Show resolved Hide resolved
src/org/rascalmpl/uri/URIUtil.java Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New mvn scheme to replace lib scheme.
2 participants