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

Fix lazily provided layered mappings causing crash #1229

Closed
wants to merge 2 commits into from

Conversation

isXander
Copy link

@isXander isXander commented Dec 10, 2024

Problem

mappings project.provider {
	loom.layered() {
		officialMojangMappings()
	}
}

The above is the configuration that caused the following crash.

> Failed to setup Minecraft, java.lang.IllegalStateException: Layered mappings have already been evaluated

Why is this behaviour wanted?

This allows people to use providers to define the mappings used by a project, for example, a gradle plugin that wraps Loom might define parchment via a extension property.

mappings myExtension.parchmentArtifact.orElse("").map { parchmentArtifact ->
	loom.layered() {
		officialMojangMappings()

		if (!parchmentArtifact.isEmpty()) {
			parchment(parchmentArtifact)
		}
	}
}

excuse the possible invalid groovy syntax, I am used to Kotlin DSL

Code analysis

During setupMinecraft in CompileConfiguration:

// 1
LayeredMappingsFactory.afterEvaluate(configContext);

// 2
final DependencyInfo mappingsDep = DependencyInfo.create(getProject(), Configurations.MAPPINGS);

// 3
final MappingConfiguration mappingConfiguration = MappingConfiguration.create(getProject(), configContext.serviceFactory(), mappingsDep, minecraftProvider);
  1. LayeredMappings are finalised - afterEvaluate method prevents any further configuration being made to the layered spec via loom.layered() {}.
  2. The dependency for mappings is fetched, this is where the provider is called upon and loom.layered() in the buildscript is now evaluated.
  3. Loom then applies the mapping that was fetched in step 2

Can you see the problem?

How did this work before?

Previously, Loom relied on the fact that the buildscript itself is evaluated before setupMinecraft. Which meant that if a loom.layered() {} was called, it would happen before it was finalised.

Using a provider exposed the problem: the layered spec is finalised before actually retrieving the dependency for mappings. The layered spec would finalise and THEN the provider would trigger, trying to call loom.layered() {} .

The fix

The fix is simple, finalise the layered mappings factory AFTER retrieving the dependency. Just swap lines 1 and 2.

// 2
final DependencyInfo mappingsDep = DependencyInfo.create(getProject(), Configurations.MAPPINGS);

// 1
LayeredMappingsFactory.afterEvaluate(configContext);

// 3
final MappingConfiguration mappingConfiguration = MappingConfiguration.create(getProject(), configContext.serviceFactory(), mappingsDep, minecraftProvider);

An integration test has been added to test for this.

@modmuss50 modmuss50 added this to the 1.10 milestone Dec 23, 2024
Comment on lines +169 to 171
final DependencyInfo mappingsDep = DependencyInfo.create(getProject(), Configurations.MAPPINGS);
// Created any layered mapping files. (After resolving the mappings dependency)
LayeredMappingsFactory.afterEvaluate(configContext);
Copy link
Member

Choose a reason for hiding this comment

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

This doesnt seem right to me, LayeredMappingsFactory.afterEvaluate is what generates the layered mapping, but calling DependencyInfo before this has the chance of resolving the mappings before they have been created. I will have a look at this, the real fix is likely a bit more complex.

modmuss50 added a commit to modmuss50/fabric-loom-1 that referenced this pull request Dec 23, 2024
@isXander isXander closed this Dec 26, 2024
@isXander
Copy link
Author

Closed in favour of #1237

modmuss50 added a commit that referenced this pull request Dec 29, 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.

2 participants