-
Notifications
You must be signed in to change notification settings - Fork 8
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
be/jvm: create JAR files instead of a collection of class files #1976
Conversation
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.
This looks good.
A number of minor suggestions and requests for additional PRs:
I would like to keep the -classes
backend to create classes and have a -jar
backend to create a jar file. Can you change the patch and split SAVE
into two phases SAVE_CLASSES
and SAVE_JAR
and add the jar option to JVMOptions and Fuzion.java?
Also, please add the verbose output of + xyz.jar
.
src/dev/flang/be/jvm/JVM.java
Outdated
String[] dependencies = { | ||
"dev/flang/be/interpreter/OpenResources.class", | ||
"dev/flang/be/jvm/runtime/Any.class", | ||
"dev/flang/be/jvm/runtime/AnyI.class", | ||
"dev/flang/be/jvm/runtime/Intrinsics.class", | ||
"dev/flang/be/jvm/runtime/Runtime.class", | ||
"dev/flang/be/jvm/runtime/Runtime$1.class", | ||
"dev/flang/be/jvm/runtime/Runtime$2.class", | ||
"dev/flang/be/jvm/runtime/Runtime$Abort.class", | ||
"dev/flang/util/ANY.class", | ||
"dev/flang/util/List.class", | ||
}; | ||
|
||
for (var d : dependencies) | ||
{ | ||
Files.createDirectory(PATH_FOR_CLASSES); | ||
} | ||
catch (IOException io) | ||
{ | ||
Errors.error("JVM backend I/O error", | ||
"While creating directory '" + PATH_FOR_CLASSES + "', received I/O error '" + io + "'"); | ||
jvm._jos.putNextEntry(new JarEntry(d)); | ||
jvm._jos.write(Files.readAllBytes(jvm._options.fuzionHome() | ||
.resolve("classes") | ||
.resolve(d) | ||
.normalize() | ||
.toAbsolutePath())); |
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.
This is ok for now, but eventually I would like to to have a build/jars/fzrt.jar
created by Makefile that contains these classes and add a dependency in the generated .jar
file instead of duplicating the classes.
JarOutputStream _jos; | ||
|
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.
Please add a comment what this is (same for my code where these comments are missing, but in a separeate patch).
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.
Can you explain what _numLocals
is for?
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.
Will add a PR for _numLocals
documentation, that is really very cryptic.
{ | ||
try | ||
{ | ||
jvm._jos.close(); |
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.
Would be great to also create a shell script with the base name of the main clazz that performs java -jar <baseName>.jar
. Same should be done for -classes
(in a speparate patch).
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.
For the JAR files, do you know about binfmt_misc?
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.
No, I did not know binfmt_misc so far. Seems useful, needs to be set up by the user. I would like to provide an easy and ideally uniform way to run the created code, independent of the backend (C/jar/classes/...).
23b9611
to
ff7582a
Compare
The comments have been addressed, insofar they affect this patch. Please review again, @fridis. |
A first look into generating JAR files from the JVM backend. @fridis, I want to know what you think about this...