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

Misdetected cycle between :extractIncludeTestProto and :compileKotlin #624

Closed
javiyt opened this issue Oct 11, 2022 · 19 comments
Closed

Misdetected cycle between :extractIncludeTestProto and :compileKotlin #624

javiyt opened this issue Oct 11, 2022 · 19 comments
Assignees
Labels
Milestone

Comments

@javiyt
Copy link

javiyt commented Oct 11, 2022

After upgrading to version 0.9.1 when we run ./gradlew build task inside a Kotlin project we get the following error:

Misdetected cycle between :extractIncludeTestProto and :compileKotlin

Here is our plugins definition in the build.gradle.kts file:

plugins {
    val kotlinVersion = "1.7.20"
    kotlin("jvm") version kotlinVersion
    kotlin("plugin.spring") version kotlinVersion
    kotlin("kapt") version kotlinVersion

    id("idea")
    id("jacoco")
    id("application")

    id("com.github.ben-manes.versions") version "0.42.0" // https://plugins.gradle.org/plugin/com.github.ben-manes
    // .versions
    id("com.google.cloud.tools.jib") version "3.3.0" // https://plugins.gradle.org/plugin/com.google.cloud.tools.jib
    id("com.google.protobuf") version "0.9.1" // https://plugins.gradle.org/plugin/com.google.protobuf

    id("io.gitlab.arturbosch.detekt") version "1.21.0" // https://plugins.gradle.org/plugin/io.gitlab.arturbosch.detekt

    id("nebula.resolution-rules") version "9.0.0" // https://plugins.gradle.org/plugin/nebula.resolution-rules

    id("org.ajoberstar.grgit") version "5.0.0" // https://plugins.gradle.org/plugin/org.ajoberstar.grgit
    id("org.flywaydb.flyway") version "9.4.0" // https://plugins.gradle.org/plugin/org.flywaydb.flyway
    id("org.jlleitschuh.gradle.ktlint") version "11.0.0" // https://plugins.gradle.org/plugin/org.jlleitschuh.gradle.ktlint
    id("org.owasp.dependencycheck") version "7.2.1" // https://plugins.gradle.org/plugin/org.owasp.dependencycheck
    id("org.sonarqube") version "3.4.0.2513" // https://plugins.gradle.org/plugin/org.sonarqube
    id("org.springframework.boot") version "2.7.4" // https://plugins.gradle.org/plugin/org.springframework.boot
}

Any help to fix it would be appreciated.

@rougsig
Copy link
Collaborator

rougsig commented Oct 11, 2022

Could you please provide minimal reprodusible project?

@rougsig
Copy link
Collaborator

rougsig commented Oct 16, 2022

@javiyt

Could you describe your project configuration in more detail?

  1. Do you modify the protobuf tasks by yourself? Not from the generateProtoTasks block.
  2. Do you modify gradle configurations? (create, remove, etc)
  3. How do you provide proto files to the project? (raw *.proto files, from any archive, etc)
  4. Could you please provide full stacktrace? (just run build with --stacktrace flag)
  5. Does project have any testProtobuf configuration dependencies?

@javiyt
Copy link
Author

javiyt commented Oct 18, 2022

Hello @rougsig as far as it's a private project I can't share it, I'm trying to create another project with the same conditions as the one that is failing that I can share with you.
I can answer some of your questions:

  1. We haven't modify any task.
  2. We have some tweaks to improve kotlin compilation
  3. From an archive
  4. Let me check if I can share it
  5. No, we don't use testProtobuf

@Spikhalskiy
Copy link

Spikhalskiy commented Oct 24, 2022

Face a similar problem in a project that uses spotless. After upgrading from 0.8.19 to 0.9.1 getting

FAILURE: Build failed with an exception.

* What went wrong:
Circular dependency between the following tasks:
:temporal-serviceclient:classes
\--- :temporal-serviceclient:spotlessApply
     \--- :temporal-serviceclient:spotlessKotlinApply
          \--- :temporal-serviceclient:spotlessKotlin
               \--- :temporal-serviceclient:generateTestProto
                    \--- :temporal-serviceclient:extractIncludeTestProto
                         \--- :temporal-serviceclient:classes (*)

classes -> spotlessApply dependency is created explicitly in the gradle script by

    classes.dependsOn 'spotlessApply'

On 0.8.19 the project builds fine.

@rougsig
Copy link
Collaborator

rougsig commented Oct 24, 2022

@Spikhalskiy What is the spotless version?

classes -> spotlessApply dependency is created explicitly in the gradle script by

Is it created by a spotless plugin or by you?

@Spikhalskiy
Copy link

Spikhalskiy commented Oct 24, 2022

What is the spotless version?

6.11.0 with googleJavaFormat('1.15.0') and ktlint('0.47.1')

Is it created by a flawless plugin or by you?

This dependency is setup in the project (our own) gradle build script. And it's been working with 0.8.x versions of this plugin.

@samzurcher
Copy link

When trying to upgrade from 0.8.18 to 0.9.1, we are seeing a similar issue:

Circular dependency between the following tasks:
:server:infrastructure:base:generateProto
\--- :server:infrastructure:base:kspKotlin
     \--- :server:infrastructure:base:generateProto (*)

(*) - details omitted (listed previously)

With 0.8.18 it works like a charm, only change is an upgrade from 0.8.18 to 0.9.1.

@rougsig
Copy link
Collaborator

rougsig commented Oct 26, 2022

@samzurcher Hello. Do you add ksp plugin and it'a all? No extra build script configuration?

@samzurcher
Copy link

We do have a rather complex build, use code in buildSrc and the KSP plugin to process our code. But all is fine with our build with version 0.8.19 but changing to 0.9.1 results in the above gradle problem.

rougsig added a commit to rougsig/protobuf-gradle-plugin that referenced this issue Oct 27, 2022
@ejona86
Copy link
Collaborator

ejona86 commented Nov 1, 2022

classes.dependsOn 'spotlessApply'

I have a very bad feeling about this. I think there's something very fundamentally wrong.

spotlessApply is not a task accepting input and producing output. Instead, it seems it would be rewriting source files in-place. That invalidates Gradle's dependency system and is bad except when run in a separate gradle invocation on non-generated source code. Even with fixing the circular dependency, I expect there will need to be further spotless configuration to exclude the generated code.

I think it is inherently broken to do classes.dependsOn 'spotlessApply' as you will have inconsistent builds. I think this is a broken usage of spotlessApply.

@Spikhalskiy
Copy link

Spikhalskiy commented Nov 1, 2022

I have a very bad feeling about this. I think there's something very fundamentally wrong.

We want to force linting on every build before classes are compiled.

it would be rewriting source files in-place

correct

That invalidates Gradle's dependency system and is bad except when run in a separate gradle invocation on non-generated source code.

This may be ideologically wrong, but it serves the purpose and it works fine until 0.9.x version of this plugin.

I think it is inherently broken to do classes.dependsOn 'spotlessApply' as you will have inconsistent builds. I think this is a broken usage of spotlessApply.

Honestly, for the majority of users of any build system, it's not important to be "pure or not pure" from the point of view of the build system. Build systems come and go. If it works and serves the purpose - it's good enough. We are not Gradle purists.
If this usage is "broken" from the point of view of the plugin maintainers and will not be compatible going forward, it's fine and understandable. We will just stick to the old version of the plugin for as long as we can, because "inconsistent builds" and "broken usage" lead to the exact outcome we want for our project.

@ejona86
Copy link
Collaborator

ejona86 commented Nov 1, 2022

I was getting really confused by the spotless thing, because I couldn't figure out how it worked without spotless, but I now notice:

:temporal-serviceclient:classes
\--- :temporal-serviceclient:spotlessApply
...
          \--- :temporal-serviceclient:spotlessKotlin
               \--- :temporal-serviceclient:generateTestProto

classes is building classes for main. But testProto is getting pulled in. This custom configuration is adding test code as a dependency of main. That's not going produce good results. spotless tasks simply should not be used this way.

@ejona86
Copy link
Collaborator

ejona86 commented Nov 1, 2022

This may be ideologically wrong, but it serves the purpose and it works fine until 0.9.x version of this plugin.

No, not an ideological thing. It means your build is unsound. You are compiling .java files at the same time those files are rewritten. Whether it "works fine" could depend on things like "how many cores does your machine have" or "whether Chrome is running" or "how an object computes its hash code."

We want to force linting on every build before classes are compiled.

For a build without any generated code, adding the dependency to javaCompile instead of classes would be a better start. But just linting and not rewriting is probably necessary to make it have defined results.

@Spikhalskiy
Copy link

Spikhalskiy commented Nov 1, 2022

@ejona86

adding the dependency to javaCompile instead of classes would be a better start

compileJava also works with 0.8.x and broken in 0.9.x pretty much the same way as classes.

This discussion in Spotless: diffplug/spotless#453 shows that

project.afterEvaluate {
    compileJava.dependsOn('spotlessApply')
}

is an intended usage of spotless.

This approach works in our project with 0.8.x of this plugin, but with 0.9.x it leads to:

Circular dependency between the following tasks:
:temporal-sdk:compileJava
\--- :temporal-serviceclient:compileJava
     \--- :temporal-serviceclient:spotlessApply
          \--- :temporal-serviceclient:spotlessKotlinApply
               \--- :temporal-serviceclient:spotlessKotlin
                    \--- :temporal-serviceclient:generateTestProto
                         \--- :temporal-serviceclient:extractIncludeTestProto
                              +--- :temporal-sdk:compileJava (*)
                              +--- :temporal-serviceclient:classes
                              |    \--- :temporal-serviceclient:compileJava (*)
                              +--- :temporal-serviceclient:compileJava (*)
                              +--- :temporal-test-server:compileJava
                              |    +--- :temporal-sdk:compileJava (*)
                              |    +--- :temporal-serviceclient:compileJava (*)
                              |    +--- :temporal-test-server:generateProto
                              |    |    \--- :temporal-test-server:extractIncludeProto
                              |    |         +--- :temporal-sdk:compileJava (*)
                              |    |         \--- :temporal-serviceclient:compileJava (*)
                              |    \--- :temporal-test-server:spotlessApply
                              |         \--- :temporal-test-server:spotlessKotlinApply
                              |              \--- :temporal-test-server:spotlessKotlin
                              |                   +--- :temporal-test-server:generateProto (*)
                              |                   \--- :temporal-test-server:generateTestProto
                              |                        \--- :temporal-test-server:extractIncludeTestProto
                              |                             +--- :temporal-sdk:compileJava (*)
                              |                             +--- :temporal-serviceclient:compileJava (*)
                              |                             +--- :temporal-test-server:classes
                              |                             |    \--- :temporal-test-server:compileJava (*)
                              |                             +--- :temporal-test-server:compileJava (*)
                              |                             \--- :temporal-testing:compileJava
                              |                                  +--- :temporal-sdk:compileJava (*)
                              |                                  +--- :temporal-serviceclient:compileJava (*)
                              |                                  \--- :temporal-test-server:compileJava (*)
                              \--- :temporal-testing:compileJava (*)

@ejona86
Copy link
Collaborator

ejona86 commented Nov 1, 2022

Which also works with 0.8.x versions of this plugin and broken with 0.9.x with

The reason it worked in 0.8.x is because 0.8.x was broken and wasn't adding the generated source to the source sets. Since that spotless task is for both main and test, there can be no guarantee it will work with code generated during the gradle build. Maybe it will work if we make a fix for the ksp issue or such, but there is no way to guarantee it will work unless you use separate spotless tasks for main vs test (dunno if spotless has those or not).

@Spikhalskiy
Copy link

Spikhalskiy commented Nov 1, 2022

Thank you for the explanation, it makes sense. In our project, we disable spotless for generated code anyway by doing targetExclude '**/generated/*.
It looks like the best course of action for us is to stay on an older version of protobuf plugin for now until spotless maintainers maybe find a way around this change.

@ejona86
Copy link
Collaborator

ejona86 commented Nov 3, 2022

@samzurcher, do you know why your kspKotlin depends on generateProto? I played around with the KSP playground example a wasn't able to get such a dependency naturally.

@schmist
Copy link

schmist commented Nov 4, 2022

@ejona86, we set the dependency generateProto -> kspKotlin explicitly since we generate proto files with ksp, but we do not know where the dependency kspKotlin -> generateProto is comming from. It was not there with version 0.8.x.

rougsig added a commit that referenced this issue Nov 4, 2022
* Do not use compileClasspath as source of proto files

* Add failing test case for #624

* Fix codenarc problems
@ejona86 ejona86 added this to the 0.9.2 milestone Jan 10, 2023
@ejona86
Copy link
Collaborator

ejona86 commented Jan 10, 2023

Fixed by #631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants