Validate remapped mods also have a remapped access widener. (#549)

* Validate remapped mods also have a remapped access widener.

* Fix checkstyle

build.gradle

@@ -78,7 +78,7 @@ dependencies {
 
 	// tinyfile management
 	implementation ('net.fabricmc:tiny-remapper:0.7.0')
-	implementation 'net.fabricmc:access-widener:2.0.1'
+	implementation 'net.fabricmc:access-widener:2.1.0'
 	implementation 'net.fabricmc:mapping-io:0.2.1'
 
 	implementation ('net.fabricmc:lorenz-tiny:4.0.2') {

src/main/java/net/fabricmc/loom/build/ModCompileRemapper.java

@@ -141,7 +141,7 @@ public class ModCompileRemapper {
 				}
 
 				try {
-					ModProcessor.processMods(project, modDependencies);
+					new ModProcessor(project).processMods(modDependencies);
 				} catch (IOException e) {
 					// Failed to remap, lets clean up to ensure we try again next time
 					modDependencies.forEach(info -> info.getRemappedOutput().delete());

src/main/java/net/fabricmc/loom/configuration/LoomDependencyManager.java

@@ -25,6 +25,9 @@
 package net.fabricmc.loom.configuration;
 
 import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -39,13 +42,14 @@ import org.gradle.api.artifacts.ExternalModuleDependency;
 import org.gradle.api.artifacts.repositories.MavenArtifactRepository;
 
 import net.fabricmc.loom.LoomGradleExtension;
+import net.fabricmc.loom.LoomGradlePlugin;
 import net.fabricmc.loom.LoomRepositoryPlugin;
 import net.fabricmc.loom.build.ModCompileRemapper;
 import net.fabricmc.loom.configuration.DependencyProvider.DependencyInfo;
-import net.fabricmc.loom.configuration.mods.ModProcessor;
 import net.fabricmc.loom.configuration.providers.mappings.MappingsProviderImpl;
 import net.fabricmc.loom.util.Constants;
 import net.fabricmc.loom.util.SourceRemapper;
+import net.fabricmc.loom.util.ZipUtils;
 
 public class LoomDependencyManager {
 	private static class ProviderList {
@@ -147,7 +151,7 @@ public class LoomDependencyManager {
 
 			for (Dependency dependency : configuration.getAllDependencies()) {
 				for (File input : configuration.files(dependency)) {
-					JsonObject jsonObject = ModProcessor.readInstallerJson(input, project);
+					JsonObject jsonObject = readInstallerJson(input);
 
 					if (jsonObject != null) {
 						if (extension.getInstallerData() != null) {
@@ -176,6 +180,20 @@ public class LoomDependencyManager {
 		}
 	}
 
+	public static JsonObject readInstallerJson(File file) {
+		try {
+			byte[] bytes = ZipUtils.unpackNullable(file.toPath(), "fabric-installer.json");
+
+			if (bytes == null) {
+				return null;
+			}
+
+			return LoomGradlePlugin.GSON.fromJson(new String(bytes, StandardCharsets.UTF_8), JsonObject.class);
+		} catch (IOException e) {
+			throw new UncheckedIOException("Failed to try and read installer json from", e);
+		}
+	}
+
 	private static void handleInstallerJson(JsonObject jsonObject, Project project) {
 		LoomGradleExtension extension = LoomGradleExtension.get(project);
 

src/main/java/net/fabricmc/loom/configuration/mods/ModProcessor.java

@@ -26,18 +26,13 @@ package net.fabricmc.loom.configuration.mods;
 
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.UncheckedIOException;
-import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.jar.JarFile;
-import java.util.stream.Collectors;
-import java.util.zip.ZipEntry;
 
 import com.google.gson.JsonObject;
 import org.gradle.api.Project;
@@ -47,7 +42,6 @@ import net.fabricmc.accesswidener.AccessWidenerReader;
 import net.fabricmc.accesswidener.AccessWidenerRemapper;
 import net.fabricmc.accesswidener.AccessWidenerWriter;
 import net.fabricmc.loom.LoomGradleExtension;
-import net.fabricmc.loom.LoomGradlePlugin;
 import net.fabricmc.loom.api.mappings.layered.MappingsNamespace;
 import net.fabricmc.loom.configuration.RemappedConfigurationEntry;
 import net.fabricmc.loom.configuration.processors.dependency.ModDependencyInfo;
@@ -62,37 +56,53 @@ import net.fabricmc.tinyremapper.OutputConsumerPath;
 import net.fabricmc.tinyremapper.TinyRemapper;
 
 public class ModProcessor {
-	public static void processMods(Project project, List<ModDependencyInfo> processList) throws IOException {
+	private static final String fromM = MappingsNamespace.INTERMEDIARY.toString();
-		if (processList.stream().noneMatch(ModDependencyInfo::requiresRemapping)) {
+	private static final String toM = MappingsNamespace.NAMED.toString();
-			return;
+
+	private final Project project;
+
+	public ModProcessor(Project project) {
+		this.project = project;
 	}
 
+	public void processMods(List<ModDependencyInfo> processList) throws IOException {
 		ArrayList<ModDependencyInfo> remapList = new ArrayList<>();
 
 		for (ModDependencyInfo info : processList) {
 			if (info.requiresRemapping()) {
-				if (info.getRemappedOutput().exists()) {
+				project.getLogger().debug("{} requires remapping", info.getInputFile());
-					info.getRemappedOutput().delete();
+				Files.deleteIfExists(info.getRemappedOutput().toPath());
-				}
 
 				remapList.add(info);
 			}
 		}
 
-		remapJars(project, processList);
+		if (remapList.isEmpty()) {
+			project.getLogger().debug("No mods to remap, skipping");
+			return;
+		}
 
+		try {
+			remapJars(remapList);
+		} catch (Exception e) {
+			project.getLogger().error("Failed to remap %d mods".formatted(remapList.size()), e);
+
+			for (ModDependencyInfo info : remapList) {
+				Files.deleteIfExists(info.getRemappedOutput().toPath());
+			}
+
+			throw e;
+		}
+
+		// Check all the mods we expect exist
 		for (ModDependencyInfo info : processList) {
 			if (!info.getRemappedOutput().exists()) {
 				throw new RuntimeException("Failed to find remapped mod" + info);
 			}
 		}
-
-		for (ModDependencyInfo info : remapList) {
-			stripNestedJars(info.getRemappedOutput());
-		}
 	}
 
-	private static void stripNestedJars(File file) {
+	private void stripNestedJars(File file) {
 		// Strip out all contained jar info as we dont want loader to try and load the jars contained in dev.
 		try {
 			ZipUtils.transformJson(JsonObject.class, file.toPath(), Map.of("fabric.mod.json", json -> {
@@ -107,7 +117,7 @@ public class ModProcessor {
 	/**
 	 * Remap another mod's access widener from intermediary to named, so that loader can apply it in our dev-env.
 	 */
-	private static byte[] remapAccessWidener(byte[] input, Remapper remapper) {
+	private byte[] remapAccessWidener(byte[] input, Remapper remapper) {
 		int version = AccessWidenerReader.readVersion(input);
 
 		AccessWidenerWriter writer = new AccessWidenerWriter(version);
@@ -122,28 +132,23 @@ public class ModProcessor {
 		return writer.write();
 	}
 
-	private static void remapJars(Project project, List<ModDependencyInfo> processList) throws IOException {
+	private void remapJars(List<ModDependencyInfo> remapList) throws IOException {
-		LoomGradleExtension extension = LoomGradleExtension.get(project);
+		final LoomGradleExtension extension = LoomGradleExtension.get(project);
-		String fromM = MappingsNamespace.INTERMEDIARY.toString();
+		final MinecraftMappedProvider mappedProvider = extension.getMinecraftMappedProvider();
-		String toM = MappingsNamespace.NAMED.toString();
+		final MappingsProviderImpl mappingsProvider = extension.getMappingsProvider();
 
-		MinecraftMappedProvider mappedProvider = extension.getMinecraftMappedProvider();
+		Path intermediaryJar = mappedProvider.getIntermediaryJar().toPath();
-		MappingsProviderImpl mappingsProvider = extension.getMappingsProvider();
-
-		Path mc = mappedProvider.getIntermediaryJar().toPath();
 		Path[] mcDeps = project.getConfigurations().getByName(Constants.Configurations.LOADER_DEPENDENCIES).getFiles()
 				.stream().map(File::toPath).toArray(Path[]::new);
 
-		List<ModDependencyInfo> remapList = processList.stream().filter(ModDependencyInfo::requiresRemapping).collect(Collectors.toList());
-
 		project.getLogger().lifecycle(":remapping " + remapList.size() + " mods (TinyRemapper, " + fromM + " -> " + toM + ")");
 
-		TinyRemapper remapper = TinyRemapper.newRemapper()
+		final TinyRemapper remapper = TinyRemapper.newRemapper()
 				.withMappings(TinyRemapperHelper.create(mappingsProvider.getMappings(), fromM, toM, false))
 				.renameInvalidLocals(false)
 				.build();
 
-		remapper.readClassPathAsync(mc);
+		remapper.readClassPathAsync(intermediaryJar);
 		remapper.readClassPathAsync(mcDeps);
 
 		final Map<ModDependencyInfo, InputTag> tagMap = new HashMap<>();
@@ -169,6 +174,7 @@ public class ModProcessor {
 			tagMap.put(info, tag);
 		}
 
+		try {
 			// Apply this in a second loop as we need to ensure all the inputs are on the classpath before remapping.
 			for (ModDependencyInfo info : remapList) {
 				try {
@@ -176,58 +182,36 @@ public class ModProcessor {
 
 					outputConsumer.addNonClassFiles(info.getInputFile().toPath(), NonClassCopyMode.FIX_META_INF, remapper);
 					outputConsumerMap.put(info, outputConsumer);
-				String accessWidener = info.getAccessWidener();
 
-				if (accessWidener != null) {
+					final ModDependencyInfo.AccessWidenerData accessWidenerData = info.getAccessWidenerData();
-					accessWidenerMap.put(info, remapAccessWidener(ZipUtils.unpack(info.inputFile.toPath(), accessWidener), remapper.getRemapper()));
+
+					if (accessWidenerData != null) {
+						project.getLogger().debug("Remapping access widener in {}", info.getInputFile());
+						byte[] remappedAw = remapAccessWidener(accessWidenerData.content(), remapper.getEnvironment().getRemapper());
+						accessWidenerMap.put(info, remappedAw);
 					}
 
 					remapper.apply(outputConsumer, tagMap.get(info));
 				} catch (Exception e) {
-				remapper.finish();
-				Files.deleteIfExists(info.getRemappedOutput().toPath());
-
 					throw new RuntimeException("Failed to remap: " + info.getRemappedNotation(), e);
 				}
 			}
-
+		} finally {
 			remapper.finish();
+		}
 
 		for (ModDependencyInfo info : remapList) {
 			outputConsumerMap.get(info).close();
 			byte[] accessWidener = accessWidenerMap.get(info);
 
 			if (accessWidener != null) {
-				ZipUtils.replace(info.getRemappedOutput().toPath(), info.getAccessWidener(), accessWidener);
+				assert info.getAccessWidenerData() != null;
+				ZipUtils.replace(info.getRemappedOutput().toPath(), info.getAccessWidenerData().path(), accessWidener);
 			}
 
+			stripNestedJars(info.getRemappedOutput());
+
 			info.finaliseRemapping();
 		}
 	}
-
-	public static JsonObject readInstallerJson(File file, Project project) {
-		try {
-			LoomGradleExtension extension = LoomGradleExtension.get(project);
-
-			String jsonStr;
-
-			try (JarFile jarFile = new JarFile(file)) {
-				ZipEntry entry = jarFile.getEntry("fabric-installer.json");
-
-				if (entry == null) {
-					return null;
-				}
-
-				try (InputStream inputstream = jarFile.getInputStream(entry)) {
-					jsonStr = new String(inputstream.readAllBytes(), StandardCharsets.UTF_8);
-				}
-			}
-
-			return LoomGradlePlugin.GSON.fromJson(jsonStr, JsonObject.class);
-		} catch (IOException e) {
-			e.printStackTrace();
-		}
-
-		return null;
-	}
 }

src/main/java/net/fabricmc/loom/configuration/processors/dependency/ModDependencyInfo.java

@@ -27,17 +27,19 @@ package net.fabricmc.loom.configuration.processors.dependency;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.InputStreamReader;
+import java.io.UncheckedIOException;
 import java.nio.charset.StandardCharsets;
-import java.util.jar.JarEntry;
+import java.nio.file.Path;
-import java.util.jar.JarFile;
 
 import com.google.gson.JsonObject;
 import org.apache.commons.io.FileUtils;
 import org.gradle.api.artifacts.Configuration;
 import org.jetbrains.annotations.Nullable;
 
+import net.fabricmc.accesswidener.AccessWidenerReader;
 import net.fabricmc.loom.LoomGradlePlugin;
+import net.fabricmc.loom.api.mappings.layered.MappingsNamespace;
+import net.fabricmc.loom.util.ZipUtils;
 
 public class ModDependencyInfo {
 	private final String group;
@@ -47,9 +49,11 @@ public class ModDependencyInfo {
 	public final String classifier;
 	public final File inputFile;
 	public final Configuration targetConfig;
-
 	public final RemapData remapData;
 
+	@Nullable
+	private final AccessWidenerData accessWidenerData;
+
 	private boolean forceRemap = false;
 
 	public ModDependencyInfo(String group, String name, String version, @Nullable String classifier, File inputFile, Configuration targetConfig, RemapData remapData) {
@@ -60,6 +64,12 @@ public class ModDependencyInfo {
 		this.inputFile = inputFile;
 		this.targetConfig = targetConfig;
 		this.remapData = remapData;
+
+		try {
+			this.accessWidenerData = tryReadAccessWidenerData(getInputFile().toPath());
+		} catch (IOException e) {
+			throw new UncheckedIOException("Failed to read access widener data from" + inputFile, e);
+		}
 	}
 
 	public String getRemappedNotation() {
@@ -106,13 +116,42 @@ public class ModDependencyInfo {
 		return inputFile;
 	}
 
+	private boolean outputHasInvalidAccessWidener() {
+		if (accessWidenerData == null) {
+			// This mod doesn't use an AW
+			return false;
+		}
+
+		assert getRemappedOutput().exists();
+		final AccessWidenerData outputAWData;
+
+		try {
+			outputAWData = tryReadAccessWidenerData(getRemappedOutput().toPath());
+		} catch (IOException e) {
+			throw new UncheckedIOException("Failed to read output access widener data from " + getRemappedOutput(), e);
+		}
+
+		if (outputAWData == null) {
+			// We know for sure the input has an AW, something is wrong if the output hasn't got one.
+			return true;
+		}
+
+		// The output jar must have an AW in the "named" namespace.
+		return !MappingsNamespace.NAMED.toString().equals(outputAWData.header().getNamespace());
+	}
+
 	public boolean requiresRemapping() {
-		return !getRemappedOutput().exists() || inputFile.lastModified() <= 0 || inputFile.lastModified() > getRemappedOutput().lastModified() || forceRemap || !getRemappedPom().exists();
+		return !getRemappedOutput().exists() || inputFile.lastModified() <= 0 || inputFile.lastModified() > getRemappedOutput().lastModified() || forceRemap || !getRemappedPom().exists() || outputHasInvalidAccessWidener();
 	}
 
 	public void finaliseRemapping() {
 		getRemappedOutput().setLastModified(inputFile.lastModified());
 		savePom();
+
+		// Validate that the remapped AW is what we want.
+		if (outputHasInvalidAccessWidener()) {
+			throw new RuntimeException("Failed to validate remapped access widener in " + getRemappedOutput());
+		}
 	}
 
 	private void savePom() {
@@ -147,23 +186,26 @@ public class ModDependencyInfo {
 		return classifier != null && !classifier.isEmpty();
 	}
 
-	public String getAccessWidener() throws IOException {
+	@Nullable
-		try (JarFile jarFile = new JarFile(getInputFile())) {
+	public AccessWidenerData getAccessWidenerData() {
-			JarEntry modJsonEntry = jarFile.getJarEntry("fabric.mod.json");
+		return accessWidenerData;
+	}
 
-			if (modJsonEntry == null) {
+	private static AccessWidenerData tryReadAccessWidenerData(Path inputJar) throws IOException {
+		byte[] modJsonBytes = ZipUtils.unpack(inputJar, "fabric.mod.json");
+		JsonObject jsonObject = LoomGradlePlugin.GSON.fromJson(new String(modJsonBytes, StandardCharsets.UTF_8), JsonObject.class);
+
+		if (!jsonObject.has("accessWidener")) {
 			return null;
 		}
 
-			try (InputStream inputStream = jarFile.getInputStream(modJsonEntry)) {
+		String accessWidenerPath = jsonObject.get("accessWidener").getAsString();
-				JsonObject json = LoomGradlePlugin.GSON.fromJson(new InputStreamReader(inputStream), JsonObject.class);
+		byte[] accessWidener = ZipUtils.unpack(inputJar, accessWidenerPath);
+		AccessWidenerReader.Header header = AccessWidenerReader.readHeader(accessWidener);
 
-				if (!json.has("accessWidener")) {
+		return new AccessWidenerData(accessWidenerPath, header, accessWidener);
-					return null;
 	}
 
-				return json.get("accessWidener").getAsString();
+	public record AccessWidenerData(String path, AccessWidenerReader.Header header, byte[] content) {
-			}
-		}
 	}
 }

src/test/groovy/net/fabricmc/loom/test/LoomTestConstants.groovy

@@ -28,7 +28,7 @@ import org.gradle.util.GradleVersion
 
 class LoomTestConstants {
     public final static String DEFAULT_GRADLE = GradleVersion.current().getVersion()
-    public final static String PRE_RELEASE_GRADLE = "7.4-20211124232407+0000"
+    public final static String PRE_RELEASE_GRADLE = "7.4-20211201231918+0000"
 
     public final static String[] STANDARD_TEST_VERSIONS = [DEFAULT_GRADLE, PRE_RELEASE_GRADLE]
 }