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.

Sign up for GitHub

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

Jump to bottom

Introduce Truffle Espresso to make GroovyShell available under GraalVM Native Image #23873

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Introduce Truffle Espresso to make GroovyShell available under GraalVM Native Image #23873

merged 1 commit into from
Feb 2, 2023

Conversation

linghengqian
Copy link
Member

@linghengqian linghengqian commented Jan 31, 2023 •
edited
Loading

For #21347 and #22899.

Changes proposed in this pull request:

  • Introduce Truffle Espresso to make GroovyShell available under GraalVM Native Image
  • Update GraalVM Reachability Metadata Repository to 0.2.6

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.

@linghengqian linghengqian mentioned this pull request Jan 31, 2023
Closed
7 tasks
@linghengqian linghengqian force-pushed the espresso-vm branch 2 times, most recently from 3181ba4 to 7783545 Compare January 31, 2023 11:06
linghengqian
linghengqian commented Jan 31, 2023
View reviewed changes
infra/util/pom.xml Outdated
Comment on lines 52 to 63
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
<executions>
<execution>
<id>copy</id>
<goals>
<goal>copy</goal>
</goals>
<phase>package</phase>
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Logically, we need to advance the Maven life cycle that executes copy to process-test-classes, so that we can verify the unit tests of Truffle Espresso in the future nativeTest.

  • But I'm not sure how to make org.apache.shardingsphere:shardingsphere-infra-util-groovy:${project.version} advance ./mvnw clean install so that the task of copy gets the corresponding JAR.

  • Is there any way for Maven to solve this kind of problem without stepping in the CI file?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • If the answer to this question is no, considering that the classes that really need to be executed on the Espresso VM are no different on 5.3.1 and 5.3.2, I can use shardingsphere infra-util:5.3.1 to provide the classpath required for espresso.

@linghengqian linghengqian marked this pull request as ready for review January 31, 2023 12:35
linghengqian
linghengqian commented Jan 31, 2023
View reviewed changes
.../src/main/java/org/apache/shardingsphere/infra/util/expr/EspressoInlineExpressionParser.java Outdated
Comment on lines 38 to 55
static {
// https://github.com/oracle/graal/issues/4555 not yet closed
String javaHome = System.getenv("GRAALVM_HOME");
if (javaHome == null) {
javaHome = System.getenv("JAVA_HOME");
}
if (javaHome == null) {
throw new RuntimeException("Failed to determine the system's environment variable GRAALVM_HOME or JAVA_HOME!");
}
System.setProperty("org.graalvm.home", javaHome);
URL resource = Thread.currentThread().getContextClassLoader().getResource("espresso-need-libs");
assert null != resource;
String dir = resource.getPath();
String javaClasspath = String.join(":", dir + "/groovy.jar", dir + "/guava.jar", dir + "/shardingsphere-infra-util-groovy.jar");
POLYGLOT = Context.newBuilder().allowAllAccess(true)
.option("java.MultiThreaded", "true")
.option("java.Classpath", javaClasspath)
.build();
}
Copy link
Member Author

@linghengqian linghengqian Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • This class is difficult to set up tests because Espresso is not available outside of GraalVM, it is not standalone like other Truffle implementations.

  • But if necessary, I can add a CI to complete the test under GraalVM CE.

linghengqian
linghengqian commented Jan 31, 2023
View reviewed changes
infra/util/pom.xml Outdated
Comment on lines 63 to 74
<phase>process-test-classes</phase>
<configuration>
<artifactItems>
<artifactItem>
<groupId>org.apache.shardingsphere</groupId>
<artifactId>shardingsphere-infra-util-groovy</artifactId>
<version>${project.version}</version>
<artifactId>shardingsphere-infra-util</artifactId>
<version>5.3.1</version>
<type>jar</type>
<overWrite>true</overWrite>
<destFileName>shardingsphere-infra-util-groovy.jar</destFileName>
<destFileName>shardingsphere-infra-util.jar</destFileName>
</artifactItem>
Copy link
Member Author

Choose a reason for hiding this comment

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

linghengqian
linghengqian commented Jan 31, 2023
View reviewed changes
pom.xml Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2023 •
edited
Loading

Codecov Report

Merging #23873 (6d4ea10) into master (3e275a6) will decrease coverage by 0.03%.
The diff coverage is 63.11%.

❗ Current head 6d4ea10 differs from pull request most recent head 892a612. Consider uploading reports for the commit 892a612 to get more accurate results

@@             Coverage Diff              @@
##             master   #23873      +/-   ##
============================================
- Coverage     50.19%   50.16%   -0.03%     
  Complexity     1567     1567              
============================================
  Files          3247     3249       +2     
  Lines         53483    53534      +51     
  Branches       9800     9812      +12     
============================================
+ Hits          26844    26857      +13     
- Misses        24280    24312      +32     
- Partials       2359     2365       +6
Impacted Files Coverage Δ
...nfra/util/expr/EspressoInlineExpressionParser.java 0.00% <0.00%> (ø)
...sphere/infra/util/expr/InlineExpressionParser.java 43.47% <40.90%> (-50.97%) ⬇️
...til/groovy/expr/HotspotInlineExpressionParser.java 94.44% <94.44%> (ø)
...handler/distsql/ral/hint/enums/HintSourceType.java 42.85% <0.00%> (+42.85%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

linghengqian
linghengqian commented Jan 31, 2023
View reviewed changes
Copy link
Member Author

@linghengqian linghengqian left a comment

Choose a reason for hiding this comment

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

@linghengqian linghengqian force-pushed the espresso-vm branch 2 times, most recently from 1ec235c to 8492c7e Compare January 31, 2023 15:46
@linghengqian linghengqian changed the title Introduce Truffle Espresso to make GroovyShell available under GraalVM Native Image and update GraalVM Reachability Metadata Repository to 0.2.6 Introduce Truffle Espresso to make GroovyShell available under GraalVM Native Image Feb 1, 2023
@linghengqian linghengqian force-pushed the espresso-vm branch 3 times, most recently from 892a612 to 106668d Compare February 1, 2023 17:55
@linghengqian
Introduce Truffle Espresso to make GroovyShell available under GraalV…
d7b0e8e 
…M Native Image and update GraalVM Reachability Metadata Repository to 0.2.6
@terrymanu terrymanu merged commit 7e12ccd into apache:master Feb 2, 2023
@terrymanu terrymanu added this to the 5.3.2 milestone Feb 2, 2023
@linghengqian linghengqian deleted the espresso-vm branch February 2, 2023 05:53
This was referenced Feb 2, 2023
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrymanu terrymanu terrymanu approved these changes

Assignees
No one assigned
Labels
type: build
Projects
None yet
Milestone
5.3.2
Development

Successfully merging this pull request may close these issues.
3 participants
@linghengqian @codecov-commenter @terrymanu