neoforged / ModDevGradle

A Gradle plugin for developing Minecraft mods using NeoForge
https://projects.neoforged.net/neoforged/moddevgradle
GNU Lesser General Public License v2.1
30 stars 5 forks source link

Support local file dependencies in Jar-in-Jar #94

Open shartte opened 2 months ago

shartte commented 2 months ago

This allows a much cleaner and simpler structure for including service-jars (i.e. core mods) based on including a secondary source set.

We use the filename as the artifact id, and its MD5 hash as the file version. This means two embedded local-file libraries of the same filename are compared using their content hash. If someone embeds a library via GAV and another embeds the same library as a local file, this fails. This is intended only for locally built source sets, and not for including local copies of Maven libraries.

neoforged-pr-publishing[bot] commented 2 months ago

Last commit published: d51a7f059061fde3287cb7f1a0aeafa9280dee2d.

PR Publishing ### The artifacts published by this PR: - :package: [`net.neoforged.moddev:net.neoforged.moddev.gradle.plugin:2.0.33-beta-pr-94-local-file-jar-jar`](https://github.com/neoforged/ModDevGradle/packages/2201025) - :package: [`net.neoforged.moddev.repositories:net.neoforged.moddev.repositories.gradle.plugin:2.0.33-beta-pr-94-local-file-jar-jar`](https://github.com/neoforged/ModDevGradle/packages/2201026) - :package: [`net.neoforged:moddev-gradle:2.0.33-beta-pr-94-local-file-jar-jar`](https://github.com/neoforged/ModDevGradle/packages/2201024) ### Repository Declaration In order to use the artifacts published by the PR, add the following repository to your buildscript: ```gradle repositories { maven { name 'Maven for PR #94' // https://github.com/neoforged/ModDevGradle/pull/94 url 'https://prmaven.neoforged.net/ModDevGradle/pr94' content { includeModule('net.neoforged.moddev', 'net.neoforged.moddev.gradle.plugin') includeModule('net.neoforged.moddev.repositories', 'net.neoforged.moddev.repositories.gradle.plugin') includeModule('net.neoforged', 'moddev-gradle') } } } ```
lukebemish commented 2 months ago

i am unconvinced that baking a bunch of resolution into the artifact ID in the jarJar metadata -- locking the included jar from being used in version resolution too, generally speaking -- is the correct solution here. Namely, I have two worries:

If the real worry is feature variants to expose a single file to then re-include being too complicated -- have we considered just making a small DSL to make that much easier? For instance, neoForge.bundledDependency(task, 'name') or something that'd make a new Configuration, add a capability, add the task as an artifact (all if it doesn't exist), then return a Dependency with the appropriate capability?

shartte commented 2 months ago

if someone includes the same task output in two different modules, this provides an easy footgun

That will work since the have the same filename and hash value. If you define a GAV for what you describe, they would still not be allowed to load simultaneously in production.

gradle already has a built in feature that can work just fine with this -- it's called feature variants. Registering a new feature, then doing a self-dep with jarJar, is not particularly hard all things told.

The average modder has no idea what those are. Sodium is doing that and it might as well be hyroglyphs to anyone reading it.

Why are we limiting it to only files built by the project? Seems rather arbitrary all things told. Gradle logic shouldn't care what made a file. The fact that that check is necessary to make this sensible is what tells me it may not be a good idea in general.

Because it is by far the most common use-case for this, and we already support cross-project dependencies using project for the other case.

shartte commented 2 months ago

@lukebemish I'll go back and actually use a different approach, however: The grouping by group/artifactId in JIJ is just a heuristic, ultimately. The real deciding factor for whether two jar files will be loadable simultaneously at runtime is whether their JPMS module name is unique or not.

For externally sourced dependencies (from Central), the uniqueness of the JPMS module name and the (group, artifactId) tuple usually correlates, but for locally built files that can not be assumed as readily.

As such, I think we should actually determine the JPMS module name when a local file is embedded and use that as the artifactId, as it will allow JIJ to detect the real conflict that would otherwise occur later and produce much better error messages.

lukebemish commented 2 months ago

Idea: if the local thing declares a module version in addition to a module name, can we use that as the version for jarJar? And/or otherwise default to Implementation-Version instead of just falling back to the file hash for a version or whatever?

neoforged-automation[bot] commented 1 week ago

@shartte, this pull request has conflicts, please resolve them for this PR to move forward.