Support unobfuscated versions + bug fixes & improvements#12
Support unobfuscated versions + bug fixes & improvements#12SizableShrimp wants to merge 15 commits intoneoforged:mainfrom
Conversation
Right now, the code doesn't work because the new 26.1 snapshot also brought with it Java 25, and MergeTool uses an outdated version of ASM.
Use ProcessMinecraftJar task from InstallerTools to unify merging and remapping code Fix bug where old commits would be pushed even if the --push flag was not set Fix bug where all commits would be pushed again if the branch was up-to-date with the remote When generating 1.21.11, Snowblower will now additionally generate 1.21.11_unobfuscated in addition, to support cleaner diffs for unobfuscated versions after 1.21.11 (e.g., 26.1-snapshot-1).
… versions All unobfuscated versions except for 1.21.11_unobfuscated are excluded by default; they can be included by explicitly adding them to the included versions list. Improved detection for cases when starting over the branch is necessary (rather than forcing an error on the user or starting over too often). * Rather than restarting every time a new version of Snowblower is released, now a manually incremented version ID is used, so branches are only restarted when actually necessary. * Snowblower will now detect if the latest committed version on a branch is newer than the target and skip generating. * Snowblower will accurately report if the latest version either doesn't exist, is older than the start version, or filtered out by the current branch configuration (or start over with --start-over-if-required). Improved error reporting to use Logger#error in more cases, rather than throwing an exception. Added more logging during generation/setup so that the computed configuration is more transparent to users.
Also fix general bugs with JAR file iteration Update InstallerTools to a PR build to fix support for pre-1.16 versions
Overhauled partial cache checking to be smarter; the partial cache for the decompiled jar will now be skipped if any individual task is outdated in the partial cache. Extracted decompilation from Generator to DecompileTask
Use MergeTool for obfuscated game versions to respect dist annotations on class members Changed unobfuscated game versions to add dist annotations with ProcessMinecraftJar instead of the OnlyInPlugin provided by neoforged's VineflowerPlugins, since VF's plugin system currently has bugs (and the plugin itself does not respect dist annotations on nested classes)
Last commit published: f8596b32349b31ead01191191e1cc4c4ba21665c - version: PR PublishingThe artifacts published by this PR:Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name = "Maven for PR #12" // https://github.com/neoforged/snowblower/pull/12
url = uri("https://prmaven.neoforged.net/snowblower/pr12")
content {
includeModule("net.neoforged", "snowblower")
}
}
} |
| } | ||
|
|
||
| public boolean isUnobfuscated() { | ||
| return FIRST_UNOBFUSCATED_RELEASE_DATE.compareTo(this.releaseTime) <= 0 || this.id.version().endsWith("_unobfuscated"); |
There was a problem hiding this comment.
I don't think this is a guarantee and I'd instead compare the version against 26.1-snapshot-1. There could always be a 1.21.12 until 26.1 is released.
There was a problem hiding this comment.
Hmm, this poses a bit of a problem because by rewriting MinecraftVersion to use a custom class, I no longer have access to comparisons, at least not without consulting the version manifest. (And the manifest would do no better here.)
The main reason for this change is it simplifies the MinecraftVersion class a lot, and I don't need a huge if block for the snapshots that needs to be maintained to determine sorting. And it hasn't really been necessary, since the version manifest gives sorting.
While I understand your point, I think it's unlikely to come up. How often do they release new versions for the current version when they have started the next development cycle? Also, it's debatable whether or not they will designate 1.21.12 (if such a version were to come out) as unobfuscated by default or not. Thus, I think it's reasonable to keep this as-is for now and reevaluate if it turns out not to be the case.
There was a problem hiding this comment.
This far into the development cycle, I think Mojang is only going to release a new hotfix when there's a security issue or massive game-breaking flaw. I think we can proceed with this heuristic for now, and cross that bridge when we get to it as Shrimp says.
| mappings = IMappingFile.load(in).reverse(); | ||
| } | ||
|
|
||
| try (var inFs = FileSystems.newFileSystem(serverJar); |
There was a problem hiding this comment.
ZipInputStream/ZipOutputStream is probably going to faster given that NIO isn't needed here (it's a case of simply listing all files and copying some of them over). But this is a nitpick.
| LOGGER.info("Discovering and downloading artifacts for {} versions", versions.size()); | ||
| GitHubActions.logStartGroup("Discovering and downloading artifacts"); | ||
|
|
||
| try (ExecutorService executor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors())) { |
There was a problem hiding this comment.
This could be a virtual thread pool without a limit
There was a problem hiding this comment.
I guess I haven't compared them, but would an unbounded number of threads not be slowed down by high CPU contention? I can test and come back to this.
There was a problem hiding this comment.
If we do go virtual threads here (which I'm pretty sure is a good fit at least on its face), what I'm not sure is if it would be better to have each download be a separate virtual thread rather than each version (with multiple downloads) on a virtual thread.
It would require some refactoring to pass the executor around, so each call to Util.downloadFile is its own task.
However, what I will say is that because Snowblower runs on Java 21, it doesn't have JEP 491: Synchronize Virtual Threads without Pinning (delivered in Java 24). So, if we move to virtual threads, that synchronized block below will have to become a Semaphore or other java.util.concurrent lock, to avoid pinning virtual threads to platform threads.
|
|
||
| var githubAppId = parser.accepts("github-app-id", "The ID of a GitHub app to use for git auth").withRequiredArg().ofType(String.class); | ||
| var githubInstallationRepo = parser.accepts("github-installation-repo", "The name of the repository to use as the installation target of the GitHub app").availableIf(githubAppId).withRequiredArg(); | ||
| var githubActionsO = parser.accepts("github-actions", "Whether Snowblower is being executed in a GitHub Actions environment (to enhance logging); defaults to the parsed boolean value of the \"GITHUB_ACTIONS\" environment variable if available") |
There was a problem hiding this comment.
Why is this configurable? Are there cases where it's run in a GHA environment without the env var set? If so, that sounds like a bug. Besides, I'd argue that the env var is itself a config option and this is redundant.
There was a problem hiding this comment.
The main reason is simply the fact that I originally coded it to use the flag without accounting for the environment variable. I'll change this accordingly.
| java { | ||
| toolchain { | ||
| languageVersion = JavaLanguageVersion.of(17) | ||
| languageVersion = JavaLanguageVersion.of(21) |
There was a problem hiding this comment.
Given 25 is the newest LTS, I'd go the full mile and bump to 25.
There was a problem hiding this comment.
I'm fine with either, but the code wouldn't be using any features of J25. I only bumped it to J21 to begin with since the latest InstallerTools version required. Keeping it at J21 means less headache for CI environments, Snowblower's main target, or users in general since J25 is still relatively new.
sciwhiz12
left a comment
There was a problem hiding this comment.
A read-through of it looks good. Mostly found some small details and some other comments.
We should update Gradle, the plugins (particularly to switch to net.neoforged.licenser), and the license header as it still says "Forge Development LLC" even for new files.
| private final boolean special; | ||
|
|
||
| Type() { | ||
| this(false); | ||
| } | ||
|
|
||
| Type(boolean special) { | ||
| this.special = special; | ||
| } | ||
|
|
||
| public boolean isSpecial() { | ||
| return this.special; | ||
| } |
There was a problem hiding this comment.
Technically, you could reduce isSpecial to just be a check on this == SPECIAL.
| import java.util.regex.Pattern; | ||
|
|
||
| public record MinecraftVersion(Type type, String version) { | ||
| // Version formats per slicedlime - https://x.com/slicedlime/status/1995886660417192442 |
There was a problem hiding this comment.
For completeness' sake (and to avoid having to refer to Twitter/X just to check), you should include the pattern here: <year>.<drop>[.<hotfix>][-<snapshot-type>-<build>], where <snapshot-type> can be snapshot, pre, or rc.
| } | ||
|
|
||
| public boolean isUnobfuscated() { | ||
| return FIRST_UNOBFUSCATED_RELEASE_DATE.compareTo(this.releaseTime) <= 0 || this.id.version().endsWith("_unobfuscated"); |
There was a problem hiding this comment.
This far into the development cycle, I think Mojang is only going to release a new hotfix when there's a security issue or massive game-breaking flaw. I think we can proceed with this heuristic for now, and cross that bridge when we get to it as Shrimp says.
There was a problem hiding this comment.
A thought: this could be expanded in the future to also encompass other CI systems that have equivalent functionality. TeamCity's service messages comes to mind in this regard. But that can be implemented in the future.
|
|
||
| try (var fs = FileSystems.newFileSystem(getOurJar(), (ClassLoader) null)) { | ||
| try (var fs = Util.isDev() ? null : FileSystems.newFileSystem(getOurJar(), (ClassLoader) null)) { | ||
| Path copyParentFolder = Util.isDev() ? Util.getSourcePath() : fs.getRootDirectories().iterator().next(); |
There was a problem hiding this comment.
This ought to be a null check now, since fs is null when running from a development environment.
|
|
||
| ### April Fools' Day versions | ||
|
|
||
| Snowblower also supports generating branches for April Fools' Day versions, separate from the mainline releases. Snowblower includes default support for `20w14infinite`, `22w13oneblockatatime`, `23w13a_or_b`, `24w14potato`, and `25w14craftmine` under the branch name `april-fools/<version>`. These branches will generate exactly two versions: the base version that the given April Fools' Day version is believed to have been forked from, and the April Fools' Day version itself. |
There was a problem hiding this comment.
Going to be fun to remember to update this with more April Fools versions as they release and get added here. 😆
| // If making changes to generation that affect the output (e.g., updating the decompiler or adding/removing decompiler args), increment this number. | ||
| public static final int VERSION_ID = 2; |
There was a problem hiding this comment.
Perhaps we should have either a CONTRIBUTING.md or a section in the README which mentions this. We might forget otherwise 😅
| // Allow resuming by finding the last thing we generated | ||
| int skipCount = this.getSkipCount(versions, filteredVersions, toGenerate, range[0]); | ||
| if (skipCount == -1) { | ||
| return; |
There was a problem hiding this comment.
I'd like to have a brief comment here to note that -1 means an error occured, so no further processing should be done. (getSkipCount already logs the error.)
| exclude.addAll(UnobfuscatedVersions.getVersionsToExclude()); | ||
| if (this.branch.includeVersions() != null) | ||
| exclude.removeAll(this.branch.includeVersions()); | ||
| this.branch.includeVersions().forEach(exclude::remove); |
There was a problem hiding this comment.
This confused me for a bit, until I opened IDEA and tried reverting this change. Apparently, IDEA has an inspection for calling removeAll for a Set with a given List argument, since AbstractSet and its descendants call List#contains for each set element (linear search for each element).
| targetVer = filteredVersions.getLast().id(); | ||
| } else { | ||
| var exclude = versions.stream().filter(v -> v.id().getType().isSpecial()).map(VersionInfo::id).collect(Collectors.toList()); | ||
| var exclude = filteredVersions.stream().map(VersionInfo::id).filter(id -> id.type().isSpecial()).collect(Collectors.toSet()); |
There was a problem hiding this comment.
Only now do I realize that Collectors#toSet has no guarantees if the returned collection is mutable. It may be wise to either copy the resulting set or use Collections#toCollection(LinkedHashSet::new). (Just in case.)
There was a problem hiding this comment.
While the written contract specifies no guarantee, after over a decade it's clear this isn't changing anytime soon, too much software relies on it. Additionally Collectors#toUnmodifiableSet was added in java 10, and Stream#toList() was added for immutable lists too, so precedent also supports that they're more likely to just add alternative methods than to touch one that has existed for a decade and so many people rely on.
|
@Pablete1234 that is actually intended as the afaik unobf versions are only included with the first unobf release which would be |
|
Shouldn't the |

Snowblower as it currently stands has a number of deficiencies which this PR attempts to address.
Main features:
26.1-snapshot-1. This is also set up to work automatically with future versions, like26.1-pre-1or26.1-rc-1._unobfuscatedversion variants are also supported, but every unobfuscated version except1.21.11_unobfuscatedrequire explicit inclusion using a branch config. (The complete list of versions with unobfuscated variants is25w45a,25w46a,1.21.11-pre1,1.21.11-pre2,1.21.11-pre3,1.21.11-pre4,1.21.11-pre5,1.21.11-rc1,1.21.11-rc2,1.21.11-rc3, and1.21.11.)1.21.11and earlier <->26.1and later,1.21.11_unobfuscatedis included in the version list by default. Since this version includes names for EVERY local variable and parameter in the game that were previously autogenerated, the1.21.11<->1.21.11_unobfuscateddiff is extremely large (see picture below). Unfortunately, there isn't really any way around this. It means that tracking down changes across this1.21.11<->26.1boundary will become a lot more annoying when using git blame. But hey, at least we have names for everything now!--start-over-if-requiredflag, making it smarter for detecting more complicated incompatible changes and starting over the branch where appropriate. (This part needs more testing to be honest, but I don't feel like writing a test harness at the moment.)--github-actions [enabled]flag).Closes #8.
Supersedes #11.