Imperative Configuration V2 #200
Open
+1,681
−1,083
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Last commit published: 2c44516430ba30984829c5d72c5764881a453094. 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 #200' // https://github.com/neoforged/ModDevGradle/pull/200
url 'https://prmaven.neoforged.net/ModDevGradle/pr200'
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-gradle')
includeModule('net.neoforged.moddev.repositories', 'net.neoforged.moddev.repositories.gradle.plugin')
}
}
} |
neoforged-automation
bot
added
the
needs rebase
|
@shartte, this pull request has conflicts, please resolve them for this PR to move forward. |
shartte
force-pushed
the
imperative-config-v2
branch
3 times, most recently
from
aa22bc9 to
2ecd9d7
Compare
neoforged-automation
bot
removed
the
needs rebase
shartte
force-pushed
the
imperative-config-v2
branch
from
2ecd9d7 to
f8ff404
Compare
Matyrobbrt
requested changes
Dec 24, 2024
src/main/java/net/neoforged/moddevgradle/dsl/NeoForgeExtension.java
Outdated
Show resolved
Hide resolved
src/legacy/java/net/neoforged/moddevgradle/legacyforge/dsl/LegacyForgeExtension.java
Show resolved
Hide resolved
src/legacy/java/net/neoforged/moddevgradle/legacyforge/dsl/LegacyForgeModdingSettings.java
Show resolved
Hide resolved
src/legacy/java/net/neoforged/moddevgradle/legacyforge/dsl/ObfuscationExtension.java
Outdated
Show resolved
Hide resolved
| * Contains the list of source sets for which access to Minecraft classes should be configured. | ||
| * Defaults to the main source set, but can also be set to an empty list. | ||
| */ | ||
| public abstract ListProperty<SourceSet> getEnabledSourceSets(); |
There was a problem hiding this comment.
Can you not put this in a common settings class?
src/main/java/net/neoforged/moddevgradle/internal/DataFileCollectionFactory.java
Outdated
Show resolved
Hide resolved
…uscationExtension.java Co-authored-by: Matyrobbrt <[email protected]>
Matyrobbrt
requested changes
Dec 24, 2024
| try { | ||
| toolchainSpec.getLanguageVersion().convention(JavaLanguageVersion.of(versionCapabilities.javaVersion())); | ||
| } catch (IllegalStateException e) { | ||
| // We tried our best |
| // We tried our best | ||
| } | ||
|
|
||
| // Add a filtered parchment repository automatically if enabled |
There was a problem hiding this comment.
Uh? We're pulling Parchment from our mirror.
Comment on lines
+122
to
+123
| task.getParchmentData().from(parchmentData); | ||
| task.getParchmentEnabled().set(parchment.getEnabled()); |
There was a problem hiding this comment.
Instead of these enabled booleans, can we not simply check if the artifact has been configured?
| task.getParchmentConflictResolutionPrefix().set(parchment.getConflictResolutionPrefix()); | ||
|
|
||
| Function<WorkflowArtifact, Provider<RegularFile>> artifactPathStrategy = artifact -> | ||
| modDevBuildDir.map(dir -> dir.dir("artifacts").file(artifactNamingStrategy.getFilename(artifact))); |
There was a problem hiding this comment.
Pull the artifacts dir outside of the lambda.
| /** | ||
| * Collects all dependencies needed by the NeoFormRuntime | ||
| */ | ||
| private static List<Configuration> configureArtifactManifestConfigurations( |
There was a problem hiding this comment.
I'm pedantic but put this method immediately after create.
| } | ||
|
|
||
| public Provider<RegularFile> requestAdditionalMinecraftArtifact(String id, String filename) { | ||
| return requestAdditionalMinecraftArtifact(id, modDevBuildDir.map(dir -> dir.file(filename))); |
There was a problem hiding this comment.
Should this not be in the artifacts dir? Or maybe another dir? Putting everything in the same dir is a bit annoying.
| spec.getDependencies().add(runTypesConfigDependency); | ||
| }); | ||
| } else { | ||
| // Create an implicit configuration |
There was a problem hiding this comment.
What does this mean? The configuration is null.
| Consumer<Configuration> configureModulePath, | ||
| Consumer<Configuration> configureLegacyClasspath, | ||
| Provider<RegularFile> assetPropertiesFile, | ||
| Provider<VersionCapabilities> versionCapabilities |
There was a problem hiding this comment.
Why is this a provider if it is always known ahead of time? In NeoDev you can supply null.
|
|
||
| // FML searches for client resources on the legacy classpath | ||
| private void addClientResources(Project project, Configuration spec, TaskProvider<CreateMinecraftArtifacts> createArtifacts) { | ||
| // FML searches for client resources on the legacy classpath |
| } | ||
|
|
||
| for (var target : testSuite.getTargets()) { | ||
| setupTestTask( |
There was a problem hiding this comment.
The current state of setupTestTask definitely does not support multiple test tasks and will fail miserably trying to configure more than one test task, we should change that or have a nicer error message.
Technici4n
reviewed
Dec 27, 2024
| /** | ||
| * Used to customize the groups of tasks generated by MDG. | ||
| * | ||
| * @param publicTaskGroup Use this group for tasks that are considered to be part of the user-interface of MDG. | ||
| * @param internalTaskGroup Use this group for tasks that are considered to be an implementation detail of MDG. | ||
| */ | ||
| record Branding(String publicTaskGroup, String internalTaskGroup) { | ||
| @ApiStatus.Internal |
There was a problem hiding this comment.
redundant since the package is already internal
src/main/java/net/neoforged/moddevgradle/internal/ModDevExtension.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * This is the top-level {@code neoForge} extension, used to configure the moddev plugin. | ||
| */ | ||
| public abstract class NeoForgeExtension { | ||
| public abstract class NeoForgeExtension extends ModDevExtension { |
There was a problem hiding this comment.
I am not convinced that splitting the extension in two is necessary or useful, when the only difference is changing some field from neoForgeVersion to forgeVersion.
There was a problem hiding this comment.
(My main annoyance is how messy the diff of this PR gets).
| @@ -28,7 +28,7 @@ public void testApplyInEmptyProject() throws IOException { | |||
| .withArguments("tasks", "--all") | |||
| .build(); | |||
|
|
|||
| assertThat(result.getOutput()).contains("createMinecraftArtifacts"); | |||
| assertThat(result.getOutput()).doesNotContain("createMinecraftArtifacts"); | |||
| @@ -29,7 +29,7 @@ public void testApplyInEmptyProject() throws IOException { | |||
| .withArguments("tasks", "--all") | |||
| .build(); | |||
|
|
|||
| assertThat(result.getOutput()).contains("createMinecraftArtifacts"); | |||
| assertThat(result.getOutput()).doesNotContain("createMinecraftArtifacts"); | |||
| @@ -94,7 +94,7 @@ public static TaskProvider<JarJar> registerWithConfiguration(Project project, St | |||
| // jvm version. We could copy DefaultJvmFeature, and search for the target version of the compile task, | |||
| // but this is difficult - we only have a feature name, not the linked source set. For this reason, we use | |||
| // the toolchain version, which is the most likely to be correct. | |||
| attributes.attributeProvider(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, javaPlugin.getToolchain().getLanguageVersion().map(JavaLanguageVersion::asInt)); | |||
| attributes.attributeProvider(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, javaPlugin.getToolchain().getLanguageVersion().orElse(JavaLanguageVersion.current()).map(JavaLanguageVersion::asInt)); | |||
Technici4n
reviewed
Dec 27, 2024
| */ | ||
| public void setVersion(Object version) { | ||
| enableModding(settings -> { | ||
| settings.setForgeVersion(version.toString()); |
Technici4n
reviewed
Dec 27, 2024
| .extendsFrom(configurations.getByName(ModDevPlugin.CONFIGURATION_COMPILE_DEPENDENCIES)); | ||
| public void setVersion(Object version) { | ||
| enableModding(settings -> { | ||
| settings.setNeoForgeVersion(version.toString()); |
|
@shartte, this pull request has conflicts, please resolve them for this PR to move forward. |
neoforged-automation
bot
added
the
needs rebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.