Improve error reporting and dynamically skip files already loaded on …

…module path (#17)

* Improve error reporting and dynamically skip files already loaded on module path

* Make use of newer SJH to properly close temporarily opened SecureJars used to compute the module name

build.gradle

@@ -14,19 +14,20 @@ logger.lifecycle("Version: {}", version)
 
 repositories {
     mavenLocal()
+    mavenCentral()
     maven {
-        name 'forge'
-        url 'https://maven.neoforged.net/releases'
+        name 'NeoForge'
+        url 'https://maven.neoforged.net/releases/'
     }
 }
 
 dependencies {
-    compileOnly('org.jetbrains:annotations:24.0.1')
-    implementation('cpw.mods:securejarhandler:3.0.4')
+    compileOnly('org.jetbrains:annotations:24.1.0')
+    implementation('cpw.mods:securejarhandler:3.0.7')
 }
 
 java {
-    toolchain.languageVersion = JavaLanguageVersion.of(17)
+    toolchain.languageVersion = JavaLanguageVersion.of(21)
     withSourcesJar()
 }
 

src/main/java/cpw/mods/bootstraplauncher/BootstrapLauncher.java

@@ -20,13 +20,16 @@
 
 import cpw.mods.cl.JarModuleFinder;
 import cpw.mods.cl.ModuleClassLoader;
+import cpw.mods.jarhandling.JarContents;
 import cpw.mods.jarhandling.JarContentsBuilder;
+import cpw.mods.jarhandling.JarMetadata;
 import cpw.mods.jarhandling.SecureJar;
 import cpw.mods.niofs.union.UnionPathFilter;
 import org.jetbrains.annotations.VisibleForTesting;
 
 import java.io.File;
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.lang.module.Configuration;
 import java.lang.module.ModuleFinder;
 import java.nio.file.Files;
@@ -44,6 +47,7 @@
 import java.util.ServiceLoader;
 import java.util.Set;
 import java.util.function.Consumer;
+import java.util.stream.Collectors;
 
 public class BootstrapLauncher {
     private static final boolean DEBUG = System.getProperties().containsKey("bsl.debug");
@@ -71,6 +75,9 @@ private static void run(boolean classloaderIsolation, String... args) {
         // Ensure backwards compatibility if somebody reads this value later on.
         System.setProperty("legacyClassPath", String.join(File.pathSeparator, legacyClasspath));
 
+        // Loaded modules (name -> fs location)
+        var loadedModules = findLoadedModules();
+
         // TODO: find existing modules automatically instead of taking in an ignore list.
         // The ignore list exempts files that start with certain listed keywords from being turned into modules (like existing modules)
         var ignoreList = System.getProperty("ignoreList", "asm,securejarhandler");
@@ -109,17 +116,44 @@ private static void run(boolean classloaderIsolation, String... args) {
             }
 
             if (Files.notExists(path)) continue;
-            // This computes the name of the artifact for detecting collisions
-            var jar = SecureJar.from(path);
-            if ("".equals(jar.name())) continue;
-            var jarname = pathLookup.computeIfAbsent(path, k -> filenameMap.getOrDefault(filename, jar.name()));
+
+            // This computes the module name for the given artifact
+            String moduleName;
+            try (var jarContent = JarContents.of(path)) {
+                moduleName = JarMetadata.from(jarContent).name();
+            } catch (UncheckedIOException | IOException e) {
+                if (DEBUG) {
+                    System.out.println("bsl: skipping '" + path + "' due to an IO error: " + e);
+                }
+                continue;
+            }
+
+            if ("".equals(moduleName)) {
+                continue;
+            }
+
+            // If a module of the same name is already loaded, skip it
+            var existingModuleLocation = loadedModules.get(moduleName);
+            if (existingModuleLocation != null) {
+                if (!existingModuleLocation.equals(path)) {
+                    throw new IllegalStateException("Module named " + moduleName + " was already on the JVMs module path loaded from " +
+                                                    existingModuleLocation + " but class-path contains it at location " + path);
+                }
+
+                if (DEBUG) {
+                    System.out.println("bsl: skipping '" + path + "' because it is already loaded on boot-path as " + moduleName);
+                }
+                continue;
+            }
+
+            var jarname = pathLookup.computeIfAbsent(path, k -> filenameMap.getOrDefault(filename, moduleName));
             order.add(jarname);
             mergeMap.computeIfAbsent(jarname, k -> new ArrayList<>()).add(path);
         }
 
 
         // Iterate over merged modules map and combine them into one SecureJar each
-        mergeMap.entrySet().stream().sorted(Comparator.comparingInt(e-> order.indexOf(e.getKey()))).forEach(e -> {
+        mergeMap.entrySet().stream().sorted(Comparator.comparingInt(e -> order.indexOf(e.getKey()))).forEach(e -> {
             // skip empty paths
             var name = e.getKey();
             var paths = e.getValue();
@@ -176,6 +210,34 @@ private static void run(boolean classloaderIsolation, String... args) {
         ((Consumer<String[]>) loader.stream().findFirst().orElseThrow().get()).accept(args);
     }
 
+    /**
+     * Find a mapping from module-name to filesystem location for the modules that are on the JVMs boot module path.
+     */
+    private static Map<String, Path> findLoadedModules() {
+        record ModuleWithLocation(String name, Path location) {
+        }
+        return ModuleLayer.boot().configuration().modules().stream()
+                .map(module -> {
+                    var reference = module.reference();
+                    var moduleName = reference.descriptor().name();
+                    var locationUri = reference.location().orElse(null);
+                    if (moduleName.isBlank() || locationUri == null) {
+                        return null;
+                    }
+
+                    Path location;
+                    try {
+                        location = new File(locationUri).toPath();
+                    } catch (IllegalArgumentException ignored) {
+                        return null; // Ignore existing modules with non-filesystem locations
+                    }
+
+                    return new ModuleWithLocation(moduleName, location);
+                })
+                .filter(Objects::nonNull)
+                .collect(Collectors.toMap(ModuleWithLocation::name, ModuleWithLocation::location));
+    }
+
     private static Map<String, String> getMergeFilenameMap() {
         var mergeModules = System.getProperty("mergeModules");
         if (mergeModules == null)
@@ -208,8 +270,8 @@ public boolean test(final String path, final Path basePath) {
 
             int idx = path.lastIndexOf('/');
             return idx < 0 || // Resources at the root are allowed to co-exist
-                idx == path.length() - 1 || // All directories can have a potential to exist without conflict, we only care about real files.
-                !packages.contains(path.substring(0, idx).replace('/', '.')); // If the package hasn't been used by a previous JAR
+                   idx == path.length() - 1 || // All directories can have a potential to exist without conflict, we only care about real files.
+                   !packages.contains(path.substring(0, idx).replace('/', '.')); // If the package hasn't been used by a previous JAR
         }
     }
 
@@ -220,8 +282,7 @@ private static List<String> loadLegacyClassPath() {
             var legacyCPFileCandidatePath = Paths.get(legacyCpPath);
             try {
                 return Files.readAllLines(legacyCPFileCandidatePath);
-            }
-            catch (IOException e) {
+            } catch (IOException e) {
                 throw new IllegalStateException("Failed to load the legacy class path from the specified file: " + legacyCpPath, e);
             }
         }