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

Legacy plugin for 1.17 to 1.20.1 #118

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

Legacy plugin for 1.17 to 1.20.1 #118

wants to merge 34 commits into from

Conversation

shartte
Copy link
Collaborator

@shartte shartte commented Jul 29, 2024

Adds a separate net.neoforged.moddev.legacy plugin that lives in a different sourceset and can be used to develop for Minecraft and Forge 1.17 to 1.20.1.

@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Jul 29, 2024

  • Publish PR to GitHub Packages

Last commit published: 72efbd34f1d2de32da59c63ee9b4bcdf27aaec56.

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

@neoforged-automation
Copy link

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

@neoforged-automation neoforged-automation bot added the needs rebase This Pull Request needs to be rebased before being merged label Aug 13, 2024
@neoforged-automation neoforged-automation bot removed the needs rebase This Pull Request needs to be rebased before being merged label Sep 6, 2024
@Matyrobbrt Matyrobbrt marked this pull request as ready for review September 10, 2024 09:41
@Matyrobbrt Matyrobbrt changed the title Legacy Legacy plugin for 1.17 to 1.20.1 Sep 10, 2024
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

Not too bad. However, it is clear that this can only work as long as keep the toolchain essentially the same. When we will remove the legacy CP, this legacy plugin will go out of the window. This is also why I don't see this ever supporting 1.16. This raises the question of whether the plugin should exist in the same branch or not.

@@ -1,6 +1,6 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.9-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-bin.zip
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-all.zip

We are writing a plugin, I want the sources.

Comment on lines +9 to +18
repositories {
mavenLocal()
maven {
name 'cursemaven'
url 'https://cursemaven.com'
content {
includeGroup "curse.maven"
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove unused mavens. (Before merging)


dependencies {
modCompileOnly('mezz.jei:jei-1.20.1-forge:15.17.0.76') {
transitive = false
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Comment on lines +26 to +32
dependencies {
modCompileOnly('mezz.jei:jei-1.20.1-forge:15.17.0.76') {
transitive = false
}
modRuntimeOnly('curse.maven:mekanism-268560:5662583')
modImplementation('curse.maven:applied-energistics-2-223794:5641282')
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to have these deps in repo? 😅

import javax.inject.Inject;
import java.util.List;

public abstract class MixinExtension {
Copy link
Member

Choose a reason for hiding this comment

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

We should move all extensions to a root extension. For example neoForgeLegacy.

@@ -244,7 +257,7 @@ public static ModFoldersProvider getIdeaModFoldersProvider(Project project,
folders = getModFoldersForGradle(project, modsProvider, includeUnitTests);
}

var modFoldersProvider = project.getObjects().newInstance(ModFoldersProvider.class);
var modFoldersProvider = project.getObjects().newInstance(ModFoldersProvider.class, project);
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary.

@@ -395,24 +408,29 @@ record AssetProperties(String assetIndex, String assetsRoot) {

abstract class ModFoldersProvider implements CommandLineArgumentProvider {
@Inject
public ModFoldersProvider() {
public ModFoldersProvider(Project project) {
getClassesArgument().set(project.provider(() -> {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the ugly provider call. You can use getModFolders().map(...).


public record UserDevConfig(String mcp, String sources, String universal, List<String> libraries, List<String> modules,
Map<String, UserDevRunType> runs) implements Serializable {
public static UserDevConfig from(File userDevFile) {
// For backwards compatibility reasons we also support loading this from the userdev jar
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this at all, it's such a hack.

@@ -0,0 +1,8 @@

Copy link
Member

Choose a reason for hiding this comment

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

not working (yet)?

Comment on lines 1062 to +1065
RunUtils.escapeJvmArg(RunUtils.getEclipseModFoldersProvider(project, run.getMods(), false).getArgument())
)
.args(RunUtils.escapeJvmArg(RunUtils.getArgFileParameter(prepareTask.getProgramArgsFile().get())))
.envVar(run.getEnvironment().get())
.envVar(RunUtils.replaceModClassesEnv(run, () -> RunUtils.getEclipseModFoldersProvider(project, run.getMods(), false)))
Copy link
Member

Choose a reason for hiding this comment

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

Not to mention that this sets both MOD_CLASSES and --fml.modFolders.

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.

3 participants