Copilot commented on code in PR #2504:
URL: https://github.com/apache/groovy/pull/2504#discussion_r3175977209
##########
src/main/java/org/codehaus/groovy/tools/RootLoader.java:
##########
@@ -109,8 +113,16 @@ public RootLoader(final LoaderConfiguration lc) {
Thread.currentThread().setContextClassLoader(this);
+ // Skip URLs already on the bootstrap (system) classpath: the launcher
+ // puts the core groovy jar on -classpath to start GroovyStarter, and
+ // groovy-starter.conf's "load lib/*.jar" then re-globs it. Adding the
+ // same jar to both loaders lets each define its own copy of every core
+ // class, and trips duplicate-resource detection.
+ Set<File> bootstrap = bootstrapClasspathFiles();
Review Comment:
The new logic is based on `System.getProperty("java.class.path")`, which is
the application/system classpath, not the JVM bootstrap classpath. To avoid
confusion for future maintainers, consider renaming
`bootstrapClasspathFiles`/`bootstrap` and adjusting the comment to refer to the
system/application classpath explicitly.
##########
src/main/java/org/codehaus/groovy/tools/RootLoader.java:
##########
@@ -179,4 +191,33 @@ public void addURL(final URL url) {
protected Class<?> findClass(final String name) throws
ClassNotFoundException {
throw new ClassNotFoundException(name);
}
+
+ private static Set<File> bootstrapClasspathFiles() {
+ Set<File> files = new HashSet<>();
+ String cp = System.getProperty("java.class.path");
+ if (cp == null || cp.isEmpty()) return files;
+ for (String entry : cp.split(File.pathSeparator)) {
+ if (entry.isEmpty()) continue;
+ File f = canonicalFile(new File(entry));
+ if (f != null) files.add(f);
+ }
+ return files;
+ }
+
+ private static File canonicalFile(URL url) {
+ if (!"file".equals(url.getProtocol())) return null;
+ try {
+ return canonicalFile(new File(url.toURI()));
+ } catch (URISyntaxException | IllegalArgumentException e) {
+ return null;
+ }
+ }
+
+ private static File canonicalFile(File f) {
+ try {
+ return f.getCanonicalFile();
+ } catch (IOException e) {
Review Comment:
`File#getCanonicalFile()` may also throw `SecurityException` if read access
is denied. Consider catching `SecurityException` alongside `IOException` and
falling back to `getAbsoluteFile()` to keep behavior consistent with the intent
of this helper.
##########
src/main/java/org/codehaus/groovy/tools/RootLoader.java:
##########
@@ -179,4 +191,33 @@ public void addURL(final URL url) {
protected Class<?> findClass(final String name) throws
ClassNotFoundException {
throw new ClassNotFoundException(name);
}
+
+ private static Set<File> bootstrapClasspathFiles() {
+ Set<File> files = new HashSet<>();
+ String cp = System.getProperty("java.class.path");
Review Comment:
`System.getProperty("java.class.path")` can throw `SecurityException` in
restricted environments. Consider catching `SecurityException` here (and
treating it like an empty classpath) so RootLoader construction doesn't fail
under a security manager / sandboxed execution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
