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 "Multiple builders are available" when syncing project in IDEA #17366

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

mlopatkin
Copy link
Member

There is a workaround that prevents adding a builder multiple times:
https://git.io/JZLn1
This workaround relies on the fact that there is the builders field in
the BuilderRegistry. The field was replaced with registrations in
a recent commit so the workaround no longer works.

This commit introduces a quick fix by providing builders field that
is backed by the data in registration.

Proper fix will probably require figuring out why the plugin is applied
twice to the same Gradle instance.

Fixes #17319

There is a workaround that prevents adding a builder multiple times:
https://git.io/JZLn1
This workaround relies on the fact that there is the `builders` field in
the BuilderRegistry. The field was replaced with `registrations` in
a recent commit so the workaround no longer works.

This commit introduces a quick fix by providing `builders` field that
is backed by the data in `registration`.

Proper fix will probably require figuring out why the plugin is applied
twice to the same Gradle instance.
@mlopatkin mlopatkin added the @core Issue owned by GBT Core label Jun 7, 2021
@mlopatkin mlopatkin added this to the 7.1 RC2 milestone Jun 7, 2021
@mlopatkin mlopatkin self-assigned this Jun 7, 2021
@big-guy
Copy link
Member

big-guy commented Jun 7, 2021

@bot-gradle test this

@bot-gradle
Copy link
Collaborator

OK, I've already triggered ReadyForMerge build for you.

@big-guy
Copy link
Member

big-guy commented Jun 7, 2021

@bot-gradle test and merge

@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@big-guy
Copy link
Member

big-guy commented Jun 7, 2021

@mlopatkin I manually tested this with the sample project and I don't see the failure now.

I also investigated why we're seeing the plugin applied multiple times. The problem isn't that the plugin is being mistakenly applied multiple times. Since Gradle 4.0, we run init scripts against included builds, so there are multiple instances of Gradle (one for each build) and this plugin gets applied to each of them. For some reason, the plugin is using project evaluation listeners to add the model builder.

I think it's possible to greatly simplify what it's doing.

I didn't fix the init script for Gradle versions older than 4.0, but I think it's possible to support those versions too.

I think something like this is all that's needed:

import org.gradle.tooling.provider.model.ToolingModelBuilderRegistry
import org.gradle.tooling.provider.model.ToolingModelBuilder
import org.gradle.util.GradleVersion

import javax.inject.Inject

apply plugin: JetGradlePlugin

class JetGradlePlugin implements Plugin<Gradle> {
    private final ToolingModelBuilderRegistry toolingModelBuilderRegistry

    @Inject
    JetGradlePlugin(ToolingModelBuilderRegistry toolingModelBuilderRegistry) {
        this.toolingModelBuilderRegistry = toolingModelBuilderRegistry
    }

    void apply(Gradle gradle) {
        def extraModelBuilderInstance =
            GradleVersion.current() >= GradleVersion.version("4.4")
            ? ExtraModelBuilder.class.classLoader.loadClass(ExtraModelBuilder.class.typeName + "\$ForGradle44").newInstance()
            : new ExtraModelBuilder();

        toolingModelBuilderRegistry.register(extraModelBuilderInstance)
        // TODO: Remove this
        println("registered $extraModelBuilderInstance on ${gradle}")
        if (GradleVersion.current() >= GradleVersion.version("3.1") && 
            GradleVersion.current() < GradleVersion.version("4.0")) {
            // TODO: add this model builder to included builds, using some version of the old code
        }
    }
}

// TODO: Remove this, this was just for testing
class CustomModel implements Serializable {
    String path
}

class ExtraModelBuilder implements ToolingModelBuilder {
    boolean canBuild(String modelName) {
        return modelName == "CustomModel"
    }

    Object buildAll(String modelName, Project project) {
        return new CustomModel(path: project.path)
    }
}

class ExtraModelBuilder$ForGradle44 extends ExtraModelBuilder {}

But maybe as a first step, could we ask JetBrains to switch to using ToolingModelBuilderRegistry.getBuilder() to check for the registration, instead of introspecting the implementation:
https://docs.gradle.org/current/javadoc/org/gradle/tooling/provider/model/ToolingModelBuilderRegistry.html#getBuilder-java.lang.String-

This has been available for ages, so backwards compatibility shouldn't be an issue.

And then they can look at modifying the init script to be more like what I've put above.

@bot-gradle bot-gradle merged commit 79bbe8b into release Jun 8, 2021
@mlopatkin mlopatkin deleted the ml/workaround-for-idea-sync-bug branch June 8, 2021 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@core Issue owned by GBT Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants