Skip to content

Cleanup mod remapping wiring#249

Merged
shartte merged 6 commits into
mainfrom
tech-transform-experiments
May 3, 2025
Merged

Cleanup mod remapping wiring#249
shartte merged 6 commits into
mainfrom
tech-transform-experiments

Conversation

@Technici4n

@Technici4n Technici4n commented May 1, 2025

Copy link
Copy Markdown
Member
  • Rewrite how artifact transforms get applied for mod* configurations.
    • Every single artifact of the jar type gets the SRG mapping attribute.
    • Any dependency added to the mod* configuration will add a dependency constraint on the NAMED mapping attribute, thus triggering the artifact transform. This seems to work for classifiers too.
  • Make sure that we don't publish the minecraft mappings attribute, to keep the previous point working. The apiElements and runtimeElements variants still receive the NAMED attribute; the obf ones do not receive any. This keeps cross-project dependencies behaving reasonably:
    • With the disambiguation rule, not requesting anything specific will select the NAMED variant.
    • Requesting SRG will select the attributeless variant, i.e. one of the obf variants, which is what we want.
  • Rename the MinecraftMappings attribute by adding a .v2 because the previous one was published and is therefore broken forever in published mods.

Fixes #240 and hopefully does not reintroduce previously fixed issues. 😓

TODO:

  • Check that dependency constraints don't leak. Probably involves publishing Gradle metadata of a mod that depends on another mod.

@neoforged-pr-publishing

neoforged-pr-publishing Bot commented May 1, 2025

Copy link
Copy Markdown
  • Publish PR to GitHub Packages

Last commit published: 76b8b2775029fd957c90965df9cdd162c0376136.

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 #249' // https://github.com/neoforged/ModDevGradle/pull/249
        url 'https://prmaven.neoforged.net/ModDevGradle/pr249'
        content {
            includeModule('net.neoforged.moddev.legacyforge', 'net.neoforged.moddev.legacyforge.gradle.plugin')
            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')
        }
    }
}

@Technici4n Technici4n changed the title Cleanup artifact transform logic Cleanup mod remapping wiring May 1, 2025

@lukebemish lukebemish left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks by and large fine -- since there is no possibility of consuming something "in the wild" with that attribute the transform should work out fine and all. My one worry is the rampant use of constraints in general may introduce weird behaviour in a few edge cases. For instance, if one does the following:

dependencies {
    modRuntimeOnly 'org.example:abc:0.1.0'
    runtimeOnly 'org.example:abc:0.1.0:someclassifier'
}

The runtimeOnly one is remapped alongside the one that normally should be. I believe this would also apply if a feature variant were used there as well as constraints apply to a whole module with no capability selector. That's enough of an edge case, it perhaps doesn't matter. I have a few other worries implementation-wise but mostly stuff easily fixed.

spec.setDescription("Configuration for dependencies of " + parent.getName() + " that needs to be remapped");
spec.setCanBeConsumed(false);
spec.setCanBeResolved(true);
spec.setCanBeResolved(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combination of canBeConsumed = false and canBeResolved = false means it should probably just be dependencyScope to begin with.

@@ -175,19 +173,12 @@ public Configuration createRemappingConfiguration(Configuration parent) {
// Additionally, we force dependencies to be non-transitive since we cannot apply the attribute hack to transitive dependencies.
spec.withDependencies(dependencies -> dependencies.forEach(dep -> {

@lukebemish lukebemish May 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I... really don't like this because it relies on the modXyz configuration being non-lazy -- which it hopefully will not be! Instead, this whole thing should probably be a parent.getDependencyConstraints().addLater(...) block, outside of (after) the create block for remappingConfig, probably .map-ing remappingConfig.getAllDependencies(), so that this still works if remappingConfig is lazy.

@shartte shartte merged commit 794fa20 into main May 3, 2025
16 of 21 checks passed
@shartte shartte deleted the tech-transform-experiments branch May 3, 2025 11:58
@neoforged-releases

Copy link
Copy Markdown

🚀 This PR has been released as ModDevGradle version 2.0.86.

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.

[Legacy] Source downloading doesn't seem to work anymore

3 participants