From e0dccae0413121e2b9a3ca4937ad3ea4c69a0f5d Mon Sep 17 00:00:00 2001 From: Paint_Ninja Date: Thu, 18 Apr 2024 20:02:41 +0100 Subject: [PATCH] Improve mod loading errors (#9870) --- .../java/net/minecraftforge/fml/ModList.java | 8 +++--- .../net/minecraftforge/fml/ModLoader.java | 23 ++++++++-------- .../fml/loading/BackgroundWaiter.java | 2 +- .../moddiscovery/AbstractModProvider.java | 12 ++++++--- .../moddiscovery/InvalidModIdentifier.java | 8 +++--- .../loading/moddiscovery/ModDiscoverer.java | 16 ++++++++++-- .../fml/loading/moddiscovery/ModFileInfo.java | 26 ++++++++++++++++--- .../loading/moddiscovery/ModFileParser.java | 3 ++- .../loading/moddiscovery/ModValidator.java | 11 ++++++-- 9 files changed, 78 insertions(+), 31 deletions(-) diff --git a/fmlcore/src/main/java/net/minecraftforge/fml/ModList.java b/fmlcore/src/main/java/net/minecraftforge/fml/ModList.java index 1512297fde..057a071829 100644 --- a/fmlcore/src/main/java/net/minecraftforge/fml/ModList.java +++ b/fmlcore/src/main/java/net/minecraftforge/fml/ModList.java @@ -47,7 +47,7 @@ import java.util.stream.Stream; */ public class ModList { - private static Logger LOGGER = LogManager.getLogger(); + //private static final Logger LOGGER = LogManager.getLogger(); private static ModList INSTANCE; private final List modFiles; private final List sortedList; @@ -120,14 +120,14 @@ public class ModList return executor -> gather( this.mods.stream() .map(mod -> ModContainer.buildTransitionHandler(mod, eventGenerator, progressBar, stateChange, executor)) - .collect(Collectors.toList())) - .thenComposeAsync(ModList::completableFutureFromExceptionList, executor); + .toList() + ).thenComposeAsync(ModList::completableFutureFromExceptionList, executor); } static CompletionStage completableFutureFromExceptionList(List> t) { if (t.stream().noneMatch(e->e.getValue()!=null)) { return CompletableFuture.completedFuture(null); } else { - final List throwables = t.stream().filter(e -> e.getValue() != null).map(Map.Entry::getValue).collect(Collectors.toList()); + final List throwables = t.stream().filter(e -> e.getValue() != null).map(Map.Entry::getValue).toList(); CompletableFuture cf = new CompletableFuture<>(); final RuntimeException accumulator = new RuntimeException(); cf.completeExceptionally(accumulator); diff --git a/fmlcore/src/main/java/net/minecraftforge/fml/ModLoader.java b/fmlcore/src/main/java/net/minecraftforge/fml/ModLoader.java index e1119e6276..58b61679fa 100644 --- a/fmlcore/src/main/java/net/minecraftforge/fml/ModLoader.java +++ b/fmlcore/src/main/java/net/minecraftforge/fml/ModLoader.java @@ -6,6 +6,7 @@ package net.minecraftforge.fml; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Streams; import net.minecraftforge.eventbus.api.Event; import net.minecraftforge.fml.event.IModBusEvent; import net.minecraftforge.fml.loading.FMLEnvironment; @@ -32,6 +33,7 @@ import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.Stream; import static net.minecraftforge.fml.Logging.CORE; import static net.minecraftforge.fml.Logging.LOADING; @@ -91,13 +93,13 @@ public class ModLoader { INSTANCE = this; this.loadingModList = FMLLoader.getLoadingModList(); - this.loadingExceptions = FMLLoader.getLoadingModList().getErrors().stream() + this.loadingExceptions = this.loadingModList.getErrors().stream() .flatMap(ModLoadingException::fromEarlyException) .collect(Collectors.toList()); - this.loadingWarnings = FMLLoader.getLoadingModList().getBrokenFiles().stream() + this.loadingWarnings = this.loadingModList.getBrokenFiles().stream() .map(file -> new ModLoadingWarning(null, ModLoadingStage.VALIDATE, InvalidModIdentifier.identifyJarProblem(file.getFilePath()).orElse("fml.modloading.brokenfile"), file.getFileName())) .collect(Collectors.toList()); - FMLLoader.getLoadingModList().getModFiles().stream() + this.loadingModList.getModFiles().stream() .filter(ModFileInfo::missingLicense) .filter(modFileInfo -> modFileInfo.getMods().stream().noneMatch(thisModInfo -> this.loadingExceptions.stream().map(ModLoadingException::getModInfo).anyMatch(otherInfo -> otherInfo == thisModInfo))) //Ignore files where any other mod already encountered an error .map(modFileInfo -> new ModLoadingException(null, ModLoadingStage.VALIDATE, "fml.modloading.missinglicense", null, modFileInfo.getFile())) @@ -228,10 +230,9 @@ public class ModLoader } catch (CompletionException e) { loadingStateValid = false; Throwable t = e.getCause(); - final List notModLoading = Arrays.stream(t.getSuppressed()) - .filter(obj -> !(obj instanceof ModLoadingException)) - .collect(Collectors.toList()); - if (!notModLoading.isEmpty()) { + boolean hasNotModLoadingEx = Arrays.stream(t.getSuppressed()) + .anyMatch(obj -> !(obj instanceof ModLoadingException)); + if (hasNotModLoadingEx) { LOGGER.fatal("Encountered non-modloading exceptions!", e); statusConsumer.ifPresent(c->c.accept("ERROR DURING MOD LOADING")); throw e; @@ -257,10 +258,10 @@ public class ModLoader .stream() .map(e -> buildModContainerFromTOML(modFile, modInfoMap, e)) .filter(Objects::nonNull) - .collect(Collectors.toList()); + .toList(); if (containers.size() != modInfoMap.size()) { - var modIds = modInfoMap.values().stream().map(IModInfo::getModId).sorted().collect(Collectors.toList()); - var containerIds = containers.stream().map(c -> c != null ? c.getModId() : "(null)").sorted().collect(Collectors.toList()); + var modIds = modInfoMap.values().stream().map(IModInfo::getModId).sorted().toList(); + var containerIds = containers.stream().map(c -> c != null ? c.getModId() : "(null)").sorted().toList(); LOGGER.fatal(LOADING,"File {} constructed {} mods: {}, but had {} mods specified: {}", modFile.getFilePath(), @@ -278,7 +279,7 @@ public class ModLoader loadingExceptions.add(new ModLoadingException(null, ModLoadingStage.CONSTRUCT, "fml.modloading.missingclasses", null, modFile.getFilePath())); } // remove errored mod containers - return containers.stream().filter(mc -> mc.modLoadingStage != ModLoadingStage.ERROR).collect(Collectors.toList()); + return containers.stream().filter(mc -> mc.modLoadingStage != ModLoadingStage.ERROR).toList(); } private ModContainer buildModContainerFromTOML(final IModFile modFile, final Map modInfoMap, final Map.Entry idToProviderEntry) { diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/BackgroundWaiter.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/BackgroundWaiter.java index 3be9974cd6..2d681f4537 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/BackgroundWaiter.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/BackgroundWaiter.java @@ -11,7 +11,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; public class BackgroundWaiter { - private static ExecutorService runner = Executors.newSingleThreadExecutor(); + private static final ExecutorService runner = Executors.newSingleThreadExecutor(); public static void runAndTick(Runnable r, Runnable tick) { ImmediateWindowHandler.updateProgress("Loading bootstrap resources"); diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractModProvider.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractModProvider.java index 2003453b67..be4b1b78bd 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractModProvider.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/AbstractModProvider.java @@ -8,6 +8,7 @@ package net.minecraftforge.fml.loading.moddiscovery; import com.mojang.logging.LogUtils; import cpw.mods.jarhandling.JarMetadata; import cpw.mods.jarhandling.SecureJar; +import net.minecraftforge.fml.loading.EarlyLoadingException; import net.minecraftforge.fml.loading.LogMarkers; import net.minecraftforge.forgespi.language.IConfigurable; import net.minecraftforge.forgespi.language.IModFileInfo; @@ -50,6 +51,9 @@ public abstract class AbstractModProvider implements IModProvider { if (sj.moduleDataProvider().findFile(MODS_TOML).isPresent()) { LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", MODS_TOML, type, path); mod = new ModFile(sj, this, ModFileParser::modsTomlParser); + if (mod.getModFileInfo().getFileProperties().containsKey(ModFileInfo.NOT_A_FORGE_MOD_PROP)) { + return new IModLocator.ModFileOrException(null, new ModFileLoadingException("File \"%s\" is not a Forge mod and cannot be loaded. Look for a Forge version of this mod or consider alternative mods.".formatted(mod.getFileName()))); + } } else if (type != null) { LOGGER.debug(LogMarkers.SCAN, "Found {} mod of type {}: {}", JarFile.MANIFEST_NAME, type, path); mod = new ModFile(sj, this, this::manifestParser, type); @@ -109,20 +113,20 @@ public abstract class AbstractModProvider implements IModProvider { }; } - try { - Files.walk(root) + try (var files = Files.walk(root)) { + files .filter(p -> p.toString().endsWith(".class")) .forEach(consumer); file.setSecurityStatus(holder.value); - } catch(IOException e) { + } catch (IOException e) { e.printStackTrace(); } LOGGER.debug(LogMarkers.SCAN, "Scan finished: {}", file); } - private static class Holder { + private static final class Holder { T value; } diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/InvalidModIdentifier.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/InvalidModIdentifier.java index 6868d69a0c..880fa7d6bf 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/InvalidModIdentifier.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/InvalidModIdentifier.java @@ -12,10 +12,12 @@ import java.nio.file.Path; import java.util.Arrays; import java.util.Optional; import java.util.function.BiPredicate; +import java.util.stream.Stream; import java.util.zip.ZipFile; import static cpw.mods.modlauncher.api.LamdbaExceptionUtils.*; +// TODO: [FML][Loader] By the time this is reached, most stuff this checks for is already filtered out - needs fixing. public enum InvalidModIdentifier { OLDFORGE(filePresent("mcmod.info")), @@ -23,9 +25,9 @@ public enum InvalidModIdentifier { LITELOADER(filePresent("litemod.json")), OPTIFINE(filePresent("optifine/Installer.class")), BUKKIT(filePresent("plugin.yml")), - INVALIDZIP((f,zf) -> !zf.isPresent()); + INVALIDZIP((f,zf) -> zf.isEmpty()); // note: only this one INVALIDZIP check is ran until the todo on this class is fixed - private BiPredicate> ident; + private final BiPredicate> ident; InvalidModIdentifier(BiPredicate> identifier) { @@ -40,7 +42,7 @@ public enum InvalidModIdentifier { public static Optional identifyJarProblem(Path path) { Optional zfo = optionalFromException(() -> new ZipFile(path.toFile())); - Optional result = Arrays.stream(values()). + Optional result = Stream.of(INVALIDZIP). filter(i -> i.ident.test(path, zfo)). map(InvalidModIdentifier::getReason). findAny(); diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModDiscoverer.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModDiscoverer.java index 2b111a3445..9dc87dbd9d 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModDiscoverer.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModDiscoverer.java @@ -23,6 +23,7 @@ import net.minecraftforge.forgespi.locating.IDependencyLocator; import net.minecraftforge.forgespi.locating.IModFile; import net.minecraftforge.forgespi.locating.IModLocator; +import net.minecraftforge.forgespi.locating.ModFileLoadingException; import org.jetbrains.annotations.ApiStatus; import org.slf4j.Logger; @@ -71,6 +72,7 @@ public class ModDiscoverer { List discoveryErrorData = new ArrayList<>(); boolean successfullyLoadedMods = true; List brokenFiles = new ArrayList<>(); + List modFileLoadingExceptions = new ArrayList<>(); boolean distIsDedicatedServer = FMLLoader.getDist().isDedicatedServer(); ImmediateWindowHandler.updateProgress("Discovering mod files"); //Loop all mod locators to get the prime mods to load from. @@ -82,7 +84,17 @@ public class ModDiscoverer { var exceptions = candidates.stream().map(IModLocator.ModFileOrException::ex).filter(Objects::nonNull).toList(); if (!exceptions.isEmpty()) { LOGGER.debug(LogMarkers.SCAN, "Locator {} found {} invalid mod files", locator, exceptions.size()); - brokenFiles.addAll(exceptions.stream().map(e->e instanceof InvalidModFileException ime ? ime.getBrokenFile() : null).filter(Objects::nonNull).toList()); + for (ModFileLoadingException ex : exceptions) { + // pipe exception messages through the discoveryErrorData to avoid swallowing some exceptions and improve error messages + // (no longer a generic "Invalid mod file" for all InvalidModExceptions - it actually shows the exception message now) + if (ex instanceof InvalidModFileException invalidModFileEx) { + var modInfo = invalidModFileEx.getBrokenFile(); + brokenFiles.add(modInfo); + discoveryErrorData.add(new EarlyLoadingException.ExceptionData(ex.getMessage(), modInfo)); + } else { + discoveryErrorData.add(new EarlyLoadingException.ExceptionData(ex.getMessage())); + } + } } var locatedFiles = candidates.stream().map(IModLocator.ModFileOrException::file).filter(Objects::nonNull).collect(Collectors.toList()); @@ -173,7 +185,7 @@ public class ModDiscoverer { //Validate the loading. With a deduplicated list, we can now successfully process the artifacts and load //transformer plugins. - var validator = new ModValidator(modFilesMap, brokenFiles, discoveryErrorData); + var validator = new ModValidator(modFilesMap, brokenFiles, discoveryErrorData, modFileLoadingExceptions); validator.stage1Validation(); return validator; } diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileInfo.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileInfo.java index 751ed3371d..d4b957f720 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileInfo.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileInfo.java @@ -14,6 +14,8 @@ import net.minecraftforge.forgespi.language.IConfigurable; import net.minecraftforge.forgespi.language.IModFileInfo; import net.minecraftforge.forgespi.language.IModInfo; import net.minecraftforge.forgespi.language.MavenVersionAdapter; +import org.apache.maven.artifact.versioning.ArtifactVersion; +import org.apache.maven.artifact.versioning.DefaultArtifactVersion; import org.slf4j.Logger; import javax.security.auth.x500.X500Principal; import java.net.URL; @@ -34,12 +36,17 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; public class ModFileInfo implements IModFileInfo, IConfigurable { private static final Logger LOGGER = LogUtils.getLogger(); + private static final ArtifactVersion NOT_JAVAFML_VER = new DefaultArtifactVersion("2"); + private static final String JAVAFML = "javafml"; + public static final String NOT_A_FORGE_MOD_PROP = "__FORGE__not_a_forge_mod"; + private final IConfigurable config; private final ModFile modFile; private final URL issueURL; @@ -65,7 +72,18 @@ public class ModFileInfo implements IModFileInfo, IConfigurable .map(MavenVersionAdapter::createFromVersionSpec) .orElseThrow(()->new InvalidModFileException("Missing ModLoader version in file", this)); this.languageSpecs = new ArrayList<>(List.of(new LanguageSpec(modLoader, modLoaderVersion))); + // the remaining properties are optional with sensible defaults + boolean notAForgeMod = false; + if (modLoader.equals(JAVAFML) && modLoaderVersion.containsVersion(NOT_JAVAFML_VER)) { + notAForgeMod = true; + this.properties = Map.of(NOT_A_FORGE_MOD_PROP, true); + } else { + this.properties = config.>getConfigElement("properties") + .orElse(Collections.emptyMap()); + } + this.modFile.setFileProperties(this.properties); + this.license = config.getConfigElement("license") .orElse(""); this.showAsResourcePack = config.getConfigElement("showAsResourcePack") @@ -76,12 +94,14 @@ public class ModFileInfo implements IModFileInfo, IConfigurable .orElse(false); this.usesServices = config.>getConfigElement("services") .orElse(List.of()); - this.properties = config.>getConfigElement("properties") - .orElse(Collections.emptyMap()); - this.modFile.setFileProperties(this.properties); this.issueURL = config.getConfigElement("issueTrackerURL") .map(StringUtils::toURL) .orElse(null); + + if (notAForgeMod) { + this.mods = List.of(); + return; + } final List modConfigs = config.getConfigList("mods"); if (modConfigs.isEmpty()) { diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileParser.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileParser.java index 9cac458341..1d3c0dada3 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileParser.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModFileParser.java @@ -13,6 +13,7 @@ import net.minecraftforge.fml.loading.LogMarkers; import net.minecraftforge.forgespi.language.IModFileInfo; import net.minecraftforge.forgespi.locating.IModFile; import net.minecraftforge.forgespi.locating.ModFileFactory; +import org.jetbrains.annotations.Nullable; import org.slf4j.Logger; import java.io.IOException; @@ -30,7 +31,7 @@ public class ModFileParser { return parser.build(modFile); } - public static IModFileInfo modsTomlParser(final IModFile imodFile) { + public static @Nullable IModFileInfo modsTomlParser(final IModFile imodFile) { ModFile modFile = (ModFile) imodFile; LOGGER.debug(LogMarkers.LOADING,"Considering mod file candidate {}", modFile.getFilePath()); final Path modsjson = modFile.findResource("META-INF", "mods.toml"); diff --git a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModValidator.java b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModValidator.java index ac1765f84b..021e5fab45 100644 --- a/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModValidator.java +++ b/fmlloader/src/main/java/net/minecraftforge/fml/loading/moddiscovery/ModValidator.java @@ -17,6 +17,7 @@ import net.minecraftforge.fml.loading.LogMarkers; import net.minecraftforge.fml.loading.ModSorter; import net.minecraftforge.forgespi.language.IModFileInfo; import net.minecraftforge.forgespi.locating.IModFile; +import net.minecraftforge.forgespi.locating.ModFileLoadingException; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; @@ -33,15 +34,21 @@ public class ModValidator { private LoadingModList loadingModList; private final List brokenFiles = new ArrayList<>(); private final List discoveryErrorData; + private final List modFileLoadingExceptions; - public ModValidator(Map> modFiles, List brokenFiles, List discoveryErrorData) { + public ModValidator(Map> modFiles, List brokenFiles, List discoveryErrorData, List modFileLoadingExceptions) { this.candidateMods = lst(modFiles.get(IModFile.Type.MOD)); this.gameLibraries = lst(modFiles.get(IModFile.Type.GAMELIBRARY)); this.candidateMods.addAll(this.gameLibraries); this.candidatePlugins = lst(modFiles.get(IModFile.Type.LANGPROVIDER)); this.candidatePlugins.addAll(lst(modFiles.get(IModFile.Type.LIBRARY))); this.discoveryErrorData = discoveryErrorData; - brokenFiles.stream().map(IModFileInfo::getFile).forEach(this.brokenFiles::add); + this.brokenFiles.addAll(brokenFiles.stream().map(IModFileInfo::getFile).toList()); + this.modFileLoadingExceptions = modFileLoadingExceptions; + } + + public ModValidator(Map> modFiles, List brokenFiles, List discoveryErrorData) { + this(modFiles, brokenFiles, discoveryErrorData, List.of()); } private static List lst(List files) {