-
Notifications
You must be signed in to change notification settings - Fork 77
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
Always load Rascal modules and Java classes of std
from the current rascal
(attempt 2)
#2112
base: replace-lib-by-mvn-and-others
Are you sure you want to change the base?
Conversation
This PR is intended to replace #2103. The main change relative to that PR is, per @jurgenvinju's suggestion, non- Instead of giving this special treatment only to |
I think I like this approach, but I don't think typepal should be treated in the future like std lib. Specifically, you want to be able to work on typepal itself while inside VS Code. No need to treat it in a special way like we do the std lib. |
Ah yes, good point. I'll do that. |
Great work!
This one I don't understand. I'm expecting we will read Rascal modules from The references to other modules in library jars are going to be via the
I think we do need a special scheme for the typepal sources, for the same reason as we have std and mvn: source level debugging and LSP references. If we look at this example:
The LSP breaks because the |
…ove code to separate method)
I've implemented Davy's suggestion and updated the issue description accordingly. I think Jurgen's main remaining point is to introduce a |
for (String srcName : manifest.getSourceRoots(manifestRoot)) { | ||
var srcFolder = URIUtil.getChildLocation(manifestRoot, srcName); | ||
|
||
if (!reg.exists(srcFolder) || !reg.isDirectory(srcFolder)) { | ||
messages.append(Messages.error("Source folder " + srcFolder + " does not exist.", getRascalMfLocation(rascalProject))); | ||
} | ||
|
||
srcsWriter.append(srcFolder); | ||
srcsWriter.append(srcFolder); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Exactly the same as the base branch, but indented because of the new outer conditional)
Let me explain my stance on this a bit more verbosely (although I've tried to keep it short).
To me this looks like more a typepal/rascal-core feature that should be able to relatize it's locations (as it already does, since it stores the locations in a logical form, and maps it to a physical at the last moment). But not something that we should try and solve by introducing a new kind of special location. |
I don't like it that much either, but we need a installation-location-independent URI for the libraries inside rascal.jar. Since they end up in the extension folder and not in the Features like the debugger and browsing through the standard libraries with M3 and .tpl summaries depend on this. |
It is (since we have
When we The difference in efficiency for loading modules should be neglibable as compared to the original I like "boot" since there is a bootstrap dependency on these libraries for making rascal work. It is very similar to the bootstrap bytecode/dll classpath of the JVM. The packager can simply rewrite all source URIs to |
Overview
This PR implements the following approach for loading Rascal modules of
rascal
:rascal/src/org/rascalmpl/library/util/PathConfig.java
Lines 667 to 692 in 7766602
Java classes of
rascal
are always loaded from the "current"rascal
(as perresolveCurrentRascalRuntimeJar()
).Thus, after this PR:
✔️ Rascal modules and Java classes of
rascal
insidestd
must be loaded from the same version. This partly fixes Rascal modules and Java classes ofrascal
are loaded from different versions rascal-language-servers#538.rascal
outsidestd
may be loaded from different versions. However, currently, there are no Rascal modules ofrascal
outsidestd
that rely on Java classes. Moreover, after the merge oftypepal
andrascal-core
intorascal
: (1) there are no Rascal modules oftypepal
that rely on Java classes; (2) there are a few Rascal modules ofrascal-core
that rely on Java classes, but the impact seems negligible in the context of this PR.std
of the deployedrascal.jar
(part of the VS Code extension), which is put on the class path of the REPL's JVM. Even whenrascal
itself is open in the workspace, changes made tostd
will not be loaded in the REPL. To test such changes, a standalone Rascal shell needs to be created (but it doesn't have debugging support).Examples (VS Code)
Create REPL in
rascal
Notes:
.../library
is loaded from the deployed jar (rascal-0.40.8-SNAPSHOT.jar
).../typepal
) ofrascal
are loaded from the workspaceCreate REPL in some project
a
(withrascal
open in the workspace)Notes:
.../library
is loaded from the deployed jar (rascal-0.40.8-SNAPSHOT.jar
).../typepal
) ofrascal
are loaded from the workspaceCreate REPL in some project
a
(withoutrascal
open in the workspace)Notes::
.../library
and.../typepal
are loaded from the deployed jar (rascal-0.40.8-SNAPSHOT.jar
)rascal
aren't loadedExamples (
RascalShell
)Create REPL in
rascal
(using aRascalShell
in the samerascal
)Notes:
.../library
is loaded from the currentrascal
(~/Desktop/rascal3/rascal
).../typepal
) ofrascal
are loaded from the main project (~/Desktop/rascal3/rascal
), which happens to be the samerascal
Create REPL in
rascal
(using aRascalShell
in a differentrascal
)Notes:
.../library
is loaded from the currentrascal
(~/Desktop/rascal3/rascal
).../typepal
) ofrascal
are loaded from the main project (~/Desktop/rascal-which-version/projects/rascal
), which happens to be a differentrascal
Create REPL in some project
a
Notes:
.../library
and.../typepal
are loaded from the currentrascal
(~/Desktop/rascal3/rascal
)rascal
aren't loaded