From b4f68746137f9588ce1687b5728d889bcd1b640a Mon Sep 17 00:00:00 2001 From: modmuss50 Date: Mon, 15 Mar 2021 18:07:54 +0000 Subject: [PATCH 1/4] Fix race condition in asset downloading --- .../minecraft/assets/MinecraftAssetsProvider.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java b/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java index 64722c60..ad005817 100644 --- a/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java +++ b/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java @@ -105,15 +105,12 @@ public class MinecraftAssetsProvider { } } else { executor.execute(() -> { - ProgressLogger progressLogger; + ProgressLogger progressLogger = loggers.pollFirst(); - if (loggers.isEmpty()) { + if (progressLogger == null) { //Create a new logger if we need one progressLogger = ProgressLogger.getProgressFactory(project, MinecraftAssetsProvider.class.getName()); progressLogger.start("Downloading assets...", "assets"); - } else { - // use a free logger if we can - progressLogger = loggers.pop(); } String assetName = entry.getKey(); From c033246a9fa4223cfa0da95822d65e3787b64fde Mon Sep 17 00:00:00 2001 From: modmuss50 Date: Thu, 18 Mar 2021 14:38:29 +0000 Subject: [PATCH 2/4] Add a strict mode when download files, will be a bit slower but should help solve some issues. --- .../assets/MinecraftAssetsProvider.java | 2 +- .../loom/util/HashedDownloadUtil.java | 20 +++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java b/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java index ad005817..dfb52b3c 100644 --- a/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java +++ b/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/assets/MinecraftAssetsProvider.java @@ -124,7 +124,7 @@ public class MinecraftAssetsProvider { progressLogger.progress(String.format("%-30.30s", assetName) + " - " + sha1); try { - HashedDownloadUtil.downloadIfInvalid(new URL(Constants.RESOURCES_BASE + sha1.substring(0, 2) + "/" + sha1), file, sha1, project.getLogger(), true); + HashedDownloadUtil.downloadIfInvalid(new URL(Constants.RESOURCES_BASE + sha1.substring(0, 2) + "/" + sha1), file, sha1, project.getLogger(), true, false); } catch (IOException e) { throw new RuntimeException("Failed to download: " + assetName, e); } diff --git a/src/main/java/net/fabricmc/loom/util/HashedDownloadUtil.java b/src/main/java/net/fabricmc/loom/util/HashedDownloadUtil.java index da294b8e..374c2bf1 100644 --- a/src/main/java/net/fabricmc/loom/util/HashedDownloadUtil.java +++ b/src/main/java/net/fabricmc/loom/util/HashedDownloadUtil.java @@ -41,16 +41,27 @@ import net.fabricmc.loom.LoomGradlePlugin; public class HashedDownloadUtil { public static void downloadIfInvalid(URL from, File to, String expectedHash, Logger logger, boolean quiet) throws IOException { + downloadIfInvalid(from, to, expectedHash, logger, quiet, true); + } + + public static void downloadIfInvalid(URL from, File to, String expectedHash, Logger logger, boolean quiet, boolean strict) throws IOException { if (LoomGradlePlugin.refreshDeps) { delete(to); } if (to.exists()) { - String sha1 = getSha1(to, logger); + if (strict) { + if (Checksum.equals(to, expectedHash)) { + // The hash matches the target file + return; + } + } else { + String sha1 = getSha1(to, logger); - if (expectedHash.equals(sha1)) { - // The hash in the sha1 file matches - return; + if (expectedHash.equals(sha1)) { + // The hash in the sha1 file matches + return; + } } } @@ -62,6 +73,7 @@ public class HashedDownloadUtil { if ((code < 200 || code > 299) && code != HttpURLConnection.HTTP_NOT_MODIFIED) { //Didn't get what we expected + delete(to); throw new IOException(connection.getResponseMessage() + " for " + from); } From 8e424f3f50e36b2eae5c7b501d159b2b8b6eac54 Mon Sep 17 00:00:00 2001 From: modmuss50 Date: Thu, 18 Mar 2021 22:00:06 +0000 Subject: [PATCH 3/4] Try to cleanup files when failing better, should hopefully increase the changes of things working on a subsequent run. Also improve the error message a little. --- .../loom/configuration/LoomDependencyManager.java | 2 +- .../loom/configuration/providers/MinecraftProvider.java | 9 +++++++-- .../providers/minecraft/MinecraftMappedProvider.java | 6 ++++-- src/main/java/net/fabricmc/loom/util/DownloadUtil.java | 2 ++ 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/main/java/net/fabricmc/loom/configuration/LoomDependencyManager.java b/src/main/java/net/fabricmc/loom/configuration/LoomDependencyManager.java index 397908c5..ff2ab892 100644 --- a/src/main/java/net/fabricmc/loom/configuration/LoomDependencyManager.java +++ b/src/main/java/net/fabricmc/loom/configuration/LoomDependencyManager.java @@ -130,7 +130,7 @@ public class LoomDependencyManager { try { provider.provide(info, afterTasks::add); } catch (Exception e) { - throw new RuntimeException("Failed to provide " + dependency.getGroup() + ":" + dependency.getName() + ":" + dependency.getVersion() + " : " + e.toString(), e); + throw new RuntimeException("Failed to provide " + dependency.getGroup() + ":" + dependency.getName() + ":" + dependency.getVersion() + " : " + e.toString() + "\n\tEnsure minecraft is not open and try running with --refresh-dependencies. Use --stacktrace to see the full stacktrace.", e); } } } diff --git a/src/main/java/net/fabricmc/loom/configuration/providers/MinecraftProvider.java b/src/main/java/net/fabricmc/loom/configuration/providers/MinecraftProvider.java index e25b8ca5..89c1646a 100644 --- a/src/main/java/net/fabricmc/loom/configuration/providers/MinecraftProvider.java +++ b/src/main/java/net/fabricmc/loom/configuration/providers/MinecraftProvider.java @@ -102,8 +102,7 @@ public class MinecraftProvider extends DependencyProvider { try { mergeJars(getProject().getLogger()); } catch (ZipError e) { - DownloadUtil.delete(minecraftClientJar); - DownloadUtil.delete(minecraftServerJar); + deleteFiles(); getProject().getLogger().error("Could not merge JARs! Deleting source JARs - please re-run the command and move on.", e); throw new RuntimeException(); @@ -119,6 +118,12 @@ public class MinecraftProvider extends DependencyProvider { versionManifestJson = new File(getExtension().getUserCache(), "version_manifest.json"); } + public void deleteFiles() { + DownloadUtil.delete(minecraftClientJar); + DownloadUtil.delete(minecraftServerJar); + DownloadUtil.delete(minecraftMergedJar); + } + private void downloadMcJson(boolean offline) throws IOException { if (getExtension().isShareCaches() && !getExtension().isRootProject() && versionManifestJson.exists() && !isRefreshDeps()) { return; diff --git a/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/MinecraftMappedProvider.java b/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/MinecraftMappedProvider.java index 6301050a..5f540ba9 100644 --- a/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/MinecraftMappedProvider.java +++ b/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/MinecraftMappedProvider.java @@ -38,6 +38,7 @@ import net.fabricmc.loom.configuration.DependencyProvider; import net.fabricmc.loom.configuration.providers.MinecraftProvider; import net.fabricmc.loom.configuration.providers.mappings.MappingsProvider; import net.fabricmc.loom.util.Constants; +import net.fabricmc.loom.util.DownloadUtil; import net.fabricmc.loom.util.TinyRemapperMappingsHelper; import net.fabricmc.tinyremapper.OutputConsumerPath; import net.fabricmc.tinyremapper.TinyRemapper; @@ -83,8 +84,9 @@ public class MinecraftMappedProvider extends DependencyProvider { mapMinecraftJar(); } catch (Throwable t) { // Cleanup some some things that may be in a bad state now - minecraftMappedJar.delete(); - minecraftIntermediaryJar.delete(); + DownloadUtil.delete(minecraftMappedJar); + DownloadUtil.delete(minecraftIntermediaryJar); + getExtension().getMinecraftProvider().deleteFiles(); getExtension().getMappingsProvider().cleanFiles(); throw new RuntimeException("Failed to remap minecraft", t); } diff --git a/src/main/java/net/fabricmc/loom/util/DownloadUtil.java b/src/main/java/net/fabricmc/loom/util/DownloadUtil.java index 9bb2b7e2..115e53d8 100644 --- a/src/main/java/net/fabricmc/loom/util/DownloadUtil.java +++ b/src/main/java/net/fabricmc/loom/util/DownloadUtil.java @@ -219,5 +219,7 @@ public class DownloadUtil { if (etagFile.exists()) { etagFile.delete(); } + + HashedDownloadUtil.delete(file); } } From 8a5467cdcaa2ad8760e1c4ceb55ade5e800872d8 Mon Sep 17 00:00:00 2001 From: Octavia Togami Date: Mon, 29 Mar 2021 00:48:52 -0700 Subject: [PATCH 4/4] Fix name comparision in GroovyXmlUtil (#373) Nodes can also have groovy.xml.QNames, which need to be compared using their matches(Object) method. --- .../net/fabricmc/loom/util/GroovyXmlUtil.java | 17 ++++- .../fabricmc/loom/GroovyXmlUtilTest.groovy | 74 +++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 src/test/groovy/net/fabricmc/loom/GroovyXmlUtilTest.groovy diff --git a/src/main/java/net/fabricmc/loom/util/GroovyXmlUtil.java b/src/main/java/net/fabricmc/loom/util/GroovyXmlUtil.java index c07fa64c..801cc118 100644 --- a/src/main/java/net/fabricmc/loom/util/GroovyXmlUtil.java +++ b/src/main/java/net/fabricmc/loom/util/GroovyXmlUtil.java @@ -29,13 +29,14 @@ import java.util.Optional; import java.util.stream.Stream; import groovy.util.Node; +import groovy.xml.QName; public final class GroovyXmlUtil { private GroovyXmlUtil() { } public static Node getOrCreateNode(Node parent, String name) { for (Object object : parent.children()) { - if (object instanceof Node && name.equals(((Node) object).name())) { + if (object instanceof Node && isSameName(((Node) object).name(), name)) { return (Node) object; } } @@ -45,7 +46,7 @@ public final class GroovyXmlUtil { public static Optional getNode(Node parent, String name) { for (Object object : parent.children()) { - if (object instanceof Node && name.equals(((Node) object).name())) { + if (object instanceof Node && isSameName(((Node) object).name(), name)) { return Optional.of((Node) object); } } @@ -53,6 +54,18 @@ public final class GroovyXmlUtil { return Optional.empty(); } + private static boolean isSameName(Object nodeName, String givenName) { + if (nodeName instanceof String) { + return nodeName.equals(givenName); + } + + if (nodeName instanceof QName) { + return ((QName) nodeName).matches(givenName); + } + + throw new UnsupportedOperationException("Cannot determine if " + nodeName.getClass() + " is the same as a String"); + } + public static Stream childrenNodesStream(Node node) { //noinspection unchecked return (Stream) (Stream) (((List) node.children()).stream().filter((i) -> i instanceof Node)); diff --git a/src/test/groovy/net/fabricmc/loom/GroovyXmlUtilTest.groovy b/src/test/groovy/net/fabricmc/loom/GroovyXmlUtilTest.groovy new file mode 100644 index 00000000..a379b727 --- /dev/null +++ b/src/test/groovy/net/fabricmc/loom/GroovyXmlUtilTest.groovy @@ -0,0 +1,74 @@ +/* + * This file is part of fabric-loom, licensed under the MIT License (MIT). + * + * Copyright (c) 2016, 2017, 2018 FabricMC + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package net.fabricmc.loom + +import groovy.xml.QName +import net.fabricmc.loom.util.GroovyXmlUtil +import spock.lang.Specification + +class GroovyXmlUtilTest extends Specification { + def "getOrCreateNode finds existing node"() { + when: + def xmlTree = new XmlParser().parseText(text) + def existingNode = xmlTree[innerName] + def actualNode = GroovyXmlUtil.getOrCreateNode(xmlTree, innerName) + + then: + existingNode.text() == actualNode.text() + + where: + innerName | text + "bar" | "inner content to ensure correct" + "dependencies" | "inner content to ensure correct" + } + + def "getOrCreateNode creates a node if needed"() { + when: + def xmlTree = new XmlParser().parseText(text) + def actualNode = GroovyXmlUtil.getOrCreateNode(xmlTree, innerName) + + then: + xmlTree[QName.valueOf(actualNode.name().toString())] != null + + where: + innerName | text + "bar" | "" + "dependencies" | "" + } + + def "getNode finds existing node"() { + when: + def xmlTree = new XmlParser().parseText(text) + def actualNode = GroovyXmlUtil.getNode(xmlTree, innerName) + + then: + actualNode.isPresent() + + where: + innerName | text + "bar" | "inner content to ensure correct" + "dependencies" | "inner content to ensure correct" + } +}