Configure languageVersion via toolchain by Goooler · Pull Request #5735 · detekt/detekt

Goooler · 2023-01-27T18:23:14Z

Follow up #5614.

https://kotlinlang.org/docs/gradle-configure-project.html#gradle-java-toolchains-support
https://jakewharton.com/build-on-latest-java-test-through-lowest-java

https://kotlinlang.org/docs/gradle-configure-project.html#gradle-java-toolchains-support

codecov · 2023-01-27T18:38:45Z

Codecov Report

Merging #5735 (e6c7329) into main (06e38d3) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #5735   +/-   ##
=========================================
  Coverage     84.66%   84.66%           
  Complexity     3833     3833           
=========================================
  Files           548      548           
  Lines         13058    13058           
  Branches       2302     2302           
=========================================
  Hits          11056    11056           
  Misses          868      868           
  Partials       1134     1134

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

3flex

I did something very similar in #5676 and reverted in #5685 because:

The original PR only was only half a solution - it meant that all builds effectively ran on Java 1.8 regardless of which Java version the Gradle build was running. This is fine for the main source build but not for tests, which we want to run on multiple Java versions.

If you can make updates to allow for that then I'd be happy to approve but there'll likely be config changes to a few places in the build and potentially the GitHub workflows as well.

https://jakewharton.com/build-on-latest-java-test-through-lowest-java

github-actions · 2023-01-28T03:11:08Z

	Warnings
⚠️	It looks like this PR contains functional changes without a corresponding test.
⚠️	This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against e6c7329

Goooler · 2023-01-28T03:12:41Z

I was noticing that too, the solution might be Build on latest Java, test through lowest Java, but there is a NoSuchMethodError in detekt-compiler-plugin test

CompilerTest > with a source file that contains violations() FAILED
    java.lang.NoSuchMethodError: 'void org.jetbrains.kotlin.cli.common.arguments.K2JVMCompilerArguments.setSingleModule(boolean)'
        at app//com.tschuchort.compiletesting.KotlinCompilation$commonK2JVMArgs$1.invoke(KotlinCompilation.kt:359)
        at app//com.tschuchort.compiletesting.KotlinCompilation$commonK2JVMArgs$1.invoke(KotlinCompilation.kt:300)
        at app//com.tschuchort.compiletesting.AbstractKotlinCompilation.commonArguments(AbstractKotlinCompilation.kt:133)
        at app//com.tschuchort.compiletesting.KotlinCompilation.commonK2JVMArgs(KotlinCompilation.kt:300)
        at app//com.tschuchort.compiletesting.KotlinCompilation.compileJvmKotlin(KotlinCompilation.kt:488)
        at app//com.tschuchort.compiletesting.KotlinCompilation.compile(KotlinCompilation.kt:664)
        at app//io.github.detekt.compiler.plugin.util.CompilerTestUtils.compile(CompilerTestUtils.kt:21)
        at app//io.github.detekt.compiler.plugin.CompilerTest.with a source file that contains violations(CompilerTest.kt:11)

3flex · 2023-01-28T09:29:19Z

The test task is currently disabled on detekt-compiler-plugin, so these auto generated tasks should be disabled on that project too.

# Conflicts: # build-logic/src/main/kotlin/module.gradle.kts

Goooler · 2023-01-28T10:04:31Z

Disabled. Let's see the impacts on build time.

3flex · 2023-01-28T21:49:24Z

Yeah that was going to be my next point, but I wanted to see the actual CI runtimes first.

They're not good (which is expected). The loss of parallelism for test execution on the different JVM versions really hurts. This tradeoff isn't worth it just to use toolchains.

cortinico · 2023-01-29T19:45:32Z

The loss of parallelism for test execution on the different JVM versions really hurts.

+1 for this. I'd rather keep explicit Java versions in the CI matrix as it's also easier for users to understand which version of Java broke

Goooler · 2023-01-30T01:54:11Z

Agreed, I'll leave this PR open until we find a new way to optimize, for the same reason, there is a regression of using toolchain in pinterest/ktlint#1787.

# Conflicts: # .github/workflows/pre-merge.yaml # build.gradle.kts

Goooler · 2023-03-20T04:12:55Z

It looks fine for now.

cortinico · 2023-03-20T14:10:28Z


 plugins {
    id("com.gradle.enterprise") version "3.12.4"
+    id("org.gradle.toolchains.foojay-resolver-convention") version "0.4.0"


Why is this needed?

For Windows, check https://github.com/detekt/detekt/actions/runs/4464478367/jobs/7840655669

It's not needed for Windows specifically, it's required any time a requested JDK is not found locally. It's a requirement in most cases if using toolchains.

More here: https://docs.gradle.org/current/userguide/toolchains.html#sub:download_repositories

# Conflicts: # build-logic/src/main/kotlin/module.gradle.kts # detekt-gradle-plugin/settings.gradle.kts

picklebento · 2023-05-29T16:50:08Z

Is this PR still blocked? Could we make a call on whether to move forward or to close this?

cortinico · 2023-06-02T17:35:48Z

My 2 c here: I'd love us to use jvmToolchain here but I'm not a big fan on adding yet another Gralde plugin to handle the Win case. So I'd rather close this for now and get back to it?

3flex · 2023-09-08T07:42:05Z

I'm going to close this, though not because of the foojay plugin (that's essentially a requirement to use toolchains outside of controlled environments where JDKs are all pre installed).

Instead I'd like to wait until this is ready later this year: gradle/build-tool-roadmap#61

We can then just use a standard config to set up the test suites instead of developing a custom one. Things are working ok as they are so there's no real reason to switch yet.

detekt-ci · 2023-09-08T07:42:58Z

	Warnings
⚠️	It looks like this PR contains functional changes without a corresponding test.
⚠️	This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against e6c7329

Configure languageVersion via toolchain

177da03

https://kotlinlang.org/docs/gradle-configure-project.html#gradle-java-toolchains-support

github-actions Bot added the build label Jan 27, 2023

vercel Bot deployed to Preview January 27, 2023 18:29 View deployment

schalkms approved these changes Jan 27, 2023

View reviewed changes

3flex requested changes Jan 27, 2023

View reviewed changes

3flex and others added 3 commits January 28, 2023 11:01

Remove more jvmTargets

4cc9aaa

Run tests on major JDKs but only build on 17

82c8407

https://jakewharton.com/build-on-latest-java-test-through-lowest-java

Merge remote-tracking branch 'origin/trunk' into use_toolchain

9cd4bb9

github-actions Bot added the ci label Jan 28, 2023

github-advanced-security AI found potential problems Jan 28, 2023

View reviewed changes

vercel Bot deployed to Preview January 28, 2023 03:18 View deployment

Cleanup

04369f0

vercel Bot deployed to Preview January 28, 2023 03:25 View deployment

Goooler added 2 commits January 28, 2023 17:54

Disable tests for detekt-compiler-plugin

6f785d8

Merge remote-tracking branch 'origin/trunk' into use_toolchain

862e661

# Conflicts: # build-logic/src/main/kotlin/module.gradle.kts

vercel Bot deployed to Preview January 28, 2023 10:07 View deployment

3flex marked this pull request as draft January 30, 2023 02:08

cortinico added the blocked label Jan 30, 2023

Goooler mentioned this pull request Mar 13, 2023

build config: Use buildSrc convention plugins to configure the Dokka subprojects Kotlin/dokka#2704

Merged

aSemy mentioned this pull request Mar 13, 2023

Regression: All versions of Java are not tested in CI/CD Kotlin/dokka#2917

Closed

Merge branch 'main' into use_toolchain

69aa39b

# Conflicts: # .github/workflows/pre-merge.yaml # build.gradle.kts

Goooler force-pushed the use_toolchain branch from eb9a1f0 to ab29283 Compare March 19, 2023 18:52

Set test toolchain via gradleProperty

43742bb

Goooler force-pushed the use_toolchain branch from ab29283 to 43742bb Compare March 19, 2023 18:52

vercel Bot had a problem deploying to Preview March 19, 2023 18:55 Failure

Intro foojay-resolver-convention plugin

fd2198d

vercel Bot had a problem deploying to Preview March 19, 2023 19:01 Failure

vercel Bot had a problem deploying to Preview March 19, 2023 19:27 Failure

Goooler force-pushed the use_toolchain branch from 3b74c6e to e64f31a Compare March 19, 2023 19:30

vercel Bot had a problem deploying to Preview March 19, 2023 19:32 Failure

Goooler force-pushed the use_toolchain branch from e64f31a to fd2198d Compare March 20, 2023 03:36

Stick with Java 17 on CI

840f172

vercel Bot had a problem deploying to Preview March 20, 2023 03:43 Failure

Apply convention plugin for detekt-gradle-plugin

9694c4f

vercel Bot deployed to Preview March 20, 2023 03:54 View deployment

Goooler marked this pull request as ready for review March 20, 2023 04:30

cortinico reviewed Mar 20, 2023

View reviewed changes

Merge branch 'main' into use_toolchain

f4dce3c

vercel Bot deployed to Preview March 21, 2023 02:33 View deployment

Goooler requested a review from 3flex March 22, 2023 03:46

Merge branch 'main' into use_toolchain

e6c7329

# Conflicts: # build-logic/src/main/kotlin/module.gradle.kts # detekt-gradle-plugin/settings.gradle.kts

vercel Bot deployed to Preview April 9, 2023 16:57 View deployment

Goooler mentioned this pull request Jul 12, 2023

Setup toolchains, compile project with Java 20 only, run test on various Java versions pinterest/ktlint#2120

Merged

9 tasks

3flex closed this Sep 8, 2023

Goooler deleted the use_toolchain branch September 8, 2023 07:43

Uh oh!

Conversation

Goooler commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

3flex left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Goooler commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

3flex commented Jan 28, 2023

Uh oh!

Goooler commented Jan 28, 2023

Uh oh!

3flex commented Jan 28, 2023

Uh oh!

cortinico commented Jan 29, 2023

Uh oh!

Goooler commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Goooler commented Mar 20, 2023

Uh oh!

cortinico Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Goooler Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

3flex Jun 4, 2023

Choose a reason for hiding this comment

Uh oh!

Goooler Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

picklebento commented May 29, 2023

Uh oh!

cortinico commented Jun 2, 2023

Uh oh!

3flex commented Sep 8, 2023

Uh oh!

detekt-ci commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Goooler commented Jan 27, 2023 •

edited

Loading

codecov Bot commented Jan 27, 2023 •

edited

Loading

github-actions Bot commented Jan 28, 2023 •

edited

Loading

Goooler commented Jan 28, 2023 •

edited

Loading

Goooler commented Jan 30, 2023 •

edited

Loading