Repository navigation
build(deps): update classgraph from 4.8.197 to 5.0.0 - #1425
Conversation
Classgraph 4.x is end-of-life. Classgraph 6.x supports only Java 25 and later. Classgraph 5.x is an interim version that continues support for Java 17 to 24. This change adopts Classgraph 5.x to maintain compatibility with the current build, which uses Java 17. Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
ClassGraph version and catalog scan gradle/libs.versions.toml, core/src/test/java/io/substrait/extension/DefaultExtensionCatalogTest.java |
The ClassGraph version changes from 4.8.197 to 5.0.0. The extension catalog test enables classpath and non-system-module scanning before restricting the scan to substrait/extensions. |
Native-image package scanning isthmus-cli/src/main/java/io/substrait/isthmus/cli/RegisterAtRuntime.java, isthmus-cli/build.gradle.kts |
PackageScanner receives GraalVM FeatureAccess, scans application classpath entries with class and annotation metadata, and loads discovered classes without initialization through the application classloader. Class-loading failures from ClassNotFoundException or LinkageError produce an IllegalStateException that names the class. The native-image configuration initializes ClassGraph’s VfsFileSystemProvider at build time. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Sequence Diagram(s)
sequenceDiagram
participant RegisterAtRuntime
participant PackageScanner
participant ClassGraph
participant ApplicationClassLoader
RegisterAtRuntime->>PackageScanner: Construct with FeatureAccess
PackageScanner->>ClassGraph: Scan application classpath
ClassGraph-->>PackageScanner: Return matching ClassInfo entries
PackageScanner->>ApplicationClassLoader: Load discovered classes without initialization
ApplicationClassLoader-->>PackageScanner: Return resolved classes
Suggested reviewers: nielspardon
Merge Risk: 
ad447
-
No concrete merge-blocking risk is established in the reviewed changes.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the primary change: updating the ClassGraph dependency from 4.8.197 to 5.0.0. It uses a valid Conventional Commit format. Description check The description explains the rationale for the dependency update and states the Java 17 compatibility requirement. It provides the required change context and is sufficiently complete. Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
| private List<Class<?>> loadClasses(ClassInfoList classInfos) { | ||
| List<Class<?>> classes = new ArrayList<>(); | ||
| for (ClassInfo classInfo : classInfos) { | ||
| Class<?> c = access.findClassByName(classInfo.getName()); | ||
| if (c == null) { | ||
| throw new IllegalStateException("Class not found: " + classInfo.getName()); | ||
| } | ||
| classes.add(c); | ||
| } | ||
| return classes; | ||
| } |
There was a problem hiding this comment.
findClassByName doesn't only return null for a missing class. In GraalVM 25.0.2 it is imageClassLoader.findClass(className).get(). ImageClassLoader.findClass catches both ClassNotFoundException and LinkageError and wraps them in a TypeResult whose get() returns null. So a class that is on the classpath but fails to link, for example a NoClassDefFoundError from a missing optional dependency, is reported as "Class not found" and the real cause is lost. ClassGraph 4's loadClasses() kept the cause: it threw IllegalArgumentException("Could not load class …", e).
findClassByName loads through getApplicationClassLoader(), so calling Class.forName on that loader behaves the same and keeps the exception:
| private List<Class<?>> loadClasses(ClassInfoList classInfos) { | |
| List<Class<?>> classes = new ArrayList<>(); | |
| for (ClassInfo classInfo : classInfos) { | |
| Class<?> c = access.findClassByName(classInfo.getName()); | |
| if (c == null) { | |
| throw new IllegalStateException("Class not found: " + classInfo.getName()); | |
| } | |
| classes.add(c); | |
| } | |
| return classes; | |
| } | |
| private List<Class<?>> loadClasses(ClassInfoList classInfos) { | |
| ClassLoader loader = access.getApplicationClassLoader(); | |
| List<Class<?>> classes = new ArrayList<>(); | |
| for (ClassInfo classInfo : classInfos) { | |
| try { | |
| classes.add(Class.forName(classInfo.getName(), false, loader)); | |
| } catch (ClassNotFoundException | LinkageError e) { | |
| throw new IllegalStateException("Could not load class " + classInfo.getName(), e); | |
| } | |
| } | |
| return classes; | |
| } |
There was a problem hiding this comment.
Implemented exactly the suggested fix, just with a slightly different shape of code that seemed more easily readable to me.
Use Class.forName instead of Feature.FeatureAccess.findClassByName to load classess so underlying exceptions are preserved in the event of a class load failure. These failures are never expected to occur but having them can aid debugging while developing changes to the GraalVM runtime registration code. Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
Classgraph 4.x is end-of-life. Classgraph 6.x supports only Java 25 and later. Classgraph 5.x is an interim version that continues support for Java 17 to 24.
This change adopts Classgraph 5.x to maintain compatibility with the current build, which uses Java 17.