Use JrtModule consistently to handle standard library on modern JDKs#1879
Use JrtModule consistently to handle standard library on modern JDKs#1879msridhar merged 24 commits intowala:masterfrom
JrtModule consistently to handle standard library on modern JDKs#1879Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1879 +/- ##
============================================
- Coverage 50.47% 50.43% -0.04%
+ Complexity 12680 12668 -12
============================================
Files 1366 1366
Lines 84890 84889 -1
Branches 14484 14484
============================================
- Hits 42847 42815 -32
- Misses 37317 37345 +28
- Partials 4726 4729 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@julian-certora @juliandolby FYI in case you see any issues |
|
Also, reported test coverage of this patch is misleadingly low. The issue is that some code paths are only exercised on JDK 25, but we don't upload coverage results from JDK 25 CI jobs to codecov (we only upload from JDK 17). I wonder if we could upload coverage files from multiple CI jobs and it would work? But I think we should explore that separately. |
msridhar
changed the title
JrtModule consistently to handle standard library on modern JDKs
| if (jar.endsWith(File.separator + "rt.jar") | ||
| || jar.endsWith(File.separator + "classes.jar")) { |
There was a problem hiding this comment.
These checks make it clear that the value returned by WalaProperties.getJ2SEJarFIles is semantically a Path, not just an arbitrary String. Should we make that official by changing the return types of WalaProperties.getJ2SEJarFIles and WalaProperties.getJDKLibraryFiles? That's a non-backward-compatible change, but this PR are already proposes non-backward-compatible changes to those two methods by changing their throws declarations.
There was a problem hiding this comment.
Is it ok to address this one also in a follow-up? I think it's a good cleanup and I'm fine with breaking backward compatibility, but it's going to involve a bunch of code churn in tests and I'd rather do it in a separate PR.
There was a problem hiding this comment.
Is it ok to address this one also in a follow-up?
Certainly!
|
@liblit thanks for the careful review! I think this is ready for another look. Note that I deleted the |
Quite the opposite; i like the idea of using jrt systematically, and Solidity support currently doesn't use Java stdlibs. |
juliandolby
left a comment
juliandolby
left a comment
There was a problem hiding this comment.
I vaguely recall once trying to make the Java source front end work with JRT, but I never managed it. Maybe that was an older version of Eclipse... I like this change.
Yeah, it seems to work now, at least the regression tests pass 🙂 Codex was helpful for figuring out that one. |
liblit
left a comment
liblit
left a comment
There was a problem hiding this comment.
Just one remaining nit: a static block that could probably be a simple field initialization expression as part of the field declaration.
| if (jar.endsWith(File.separator + "rt.jar") | ||
| || jar.endsWith(File.separator + "classes.jar")) { |
There was a problem hiding this comment.
Is it ok to address this one also in a follow-up?
Certainly!
4 participants
Fixes #1876
Before this change, we would load standard library
jmodfiles for recent JDKs from a well-known place in the filesystem. But, certain JDK distributions like Temurin no longer distribute such files directly. This change allows WALA to run on such JDKs, by loading the standard library files via the pre-existingJrtModulesupport, which loads the files via a special virtual filesystem. This will be a nice benefit for WALA users, since WALA will be able to load the running JVM's standard libraries out of the box on recent versions of JDKs like Temurin that don't package jmod files. To ensure this support doesn't regress, we now install Temurin JDKs by default on CI, and we no longer specifically request Zulu JDKs (which include jmod files) in our Java toolchain configuration.There are some API-incompatible changes in this PR. In particular,
WalaProperties.getJ2SEJarFilesnow can throw aNoJDKLibraryFilesFoundExceptionif there are no standard library files distributed with the JDK, in which case client code should fall back on callingPlatformUtil.getJDKModuleNamesand then loading the modules viaJrtModule. It would be nice to have a single clean code path for handling all these cases, but it's not straightforward, given we still want to support loading standard libraries specified in scope files.There are a couple of cases where tests passed without even loading the standard library; so for now, I just left TODO comments on those to do more detailed handling in the future.
Other related changes:
Java9AnalysisScopeReaderintoAnalysisScopeReader, and deprecated the former.CONSTANT_Dynamicconstant pool entries (exposed since these changes caused all library modules to be loaded on modern JDKs for the first time); add basic support for those.