Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

shartte
Copy link
Collaborator

@shartte shartte commented Jul 10, 2024

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
Copy link

neoforged-pr-publishing bot commented Jul 10, 2024

  • Publish PR to GitHub Packages

Last commit published: 06cf6a7e4ded7967ebbd43044edf5fd9dd502d3f.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

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
Copy link
Contributor

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 someone includes the same task output in two different modules, this provides an easy footgun -- as soon as the versions of those two modules differ in production, we end up with two copies of it trying to load. Remember that a single project can produce more than one artifact being distributed in the end!
  • 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.
  • 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.

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
Copy link
Collaborator Author

shartte commented Jul 11, 2024

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 shartte marked this pull request as draft July 11, 2024 09:34
@shartte
Copy link
Collaborator Author

shartte commented Jul 11, 2024

@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
Copy link
Contributor

lukebemish commented Jul 13, 2024

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?

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@shartte shartte marked this pull request as ready for review August 11, 2024 17:04
README.md Outdated Show resolved Hide resolved
testproject/jijtest/build.gradle Outdated Show resolved Hide resolved
testproject/jijtest/build.gradle Outdated Show resolved Hide resolved
@neoforged-automation neoforged-automation bot added the needs rebase This Pull Request needs to be rebased before being merged label Sep 1, 2024
@neoforged-automation
Copy link

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

@neoforged-automation neoforged-automation bot removed the needs rebase This Pull Request needs to be rebased before being merged label Sep 2, 2024
README.md Show resolved Hide resolved
@neoforged-automation neoforged-automation bot added the needs rebase This Pull Request needs to be rebased before being merged label Oct 9, 2024
@neoforged-automation
Copy link

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

@neoforged-automation neoforged-automation bot removed the needs rebase This Pull Request needs to be rebased before being merged label Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants