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

mateuszkwiecinski · 2023-07-09T19:22:55Z

Description

I wanted to open the project, but it failed to sync for me locally (because my defaults point to Java 20), so I decided to make the build more deterministic. I'll comment out all changes in the PR since some of them require your input, but the tl;dr is:

The project will be built only with a single Java version (that's what the release workflow does)
Only tests will be invoked on various Java versions (that's what project consumers do)
The project will compile on Java 20, and if someone doesn't have Java 11 or 20 installed locally, Gradle will fetch missing ones
I also removed a few of annoying compiler and deprecation warnings

Checklist

Before submitting the PR, please check following (checks which are not relevant may be ignored):

Commit message are well written. In addition to a short title, the commit message also explain why a change is made.
At least one commit message contains a reference Closes #<xxx> or Fixes #<xxx> (replace<xxx> with issue number)
Tests are added
KtLint format has been applied on source code itself and violations are fixed
CHANGELOG.md is updated
PR description added

Documentation is updated. See difference between snapshot and release documentation

Snapshot documentation in case documentation is to be released together with a code change
Release documentation in case documentation is related to a released version of ktlint and has to be published as soon as the change is merged to master
In case of adding a new rule, it needs to be added to experimental rules documentation

.editorconfig

.github/workflows/publish-release-build.yml

.github/workflows/pull-request-with-code.yml

CHANGELOG.md

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts

build.gradle.kts

settings.gradle.kts

.github/workflows/pull-request-with-code.yml

paul-dingemans

Thanks you so much for all explanatory comments on the changes. As I have little Gradle knowledge, I wouldn't have understand what was changed. Now at least, I have some more understanding.

My biggest concern with this change is that there are many locations where a java version is to be changed whenever a new Java version is release or whenever an LTS version is dropped. So all location should be clearly marked with something like Java version for compilation (e.g. latest release Java version) or Supported LTS versions of Java + latest non-LTS version if applicable.

.github/workflows/publish-release-build.yml

.github/workflows/publish-snapshot-build.yml

.github/workflows/pull-request-with-code.yml

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts

settings.gradle.kts

mateuszkwiecinski

I pushed another proposal, with less distributed config:

Java compilation settings are now driven by versions defined in .toml file
Tested Java versions are now defined only within workflow file
Java version hardcoded in workflow files still have to be kept in sync, so I added necessary comments reminding to update the value

@paul-dingemans Please let me know if this suits you any better, or if the complexity in the configuration is not worth having since source of truth for Java setup

.github/workflows/pull-request-with-code.yml

build-logic/build.gradle.kts

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts

gradle/libs.versions.toml

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts

.github/workflows/pull-request-with-code.yml

.github/workflows/pull-request-without-code.yml

build-logic/build.gradle.kts

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts

paul-dingemans

Just saw that I had some more pending review comments.

mateuszkwiecinski

Welp, I couldn't create separate PR, targeting this one since this branch lives only in my forked repository :/
I pushed another set of commits, on top of old ones, but if you want to see the diff alone, it can be viewed here: mateuszkwiecinski#3 🤷

.github/actions/setup-gradle-build/action.yml

.github/workflows/pull-request-with-code.yml

build-logic/build.gradle.kts

build-logic/src/main/kotlin/ktlint-publication.gradle.kts

gradle/libs.versions.toml

paul-dingemans · 2023-07-20T19:46:59Z

We have to wait till next week before @shashachu is back from holidays. I cannot change the configuration so that the remaining checks are run as well.

paul-dingemans · 2023-07-25T19:01:45Z

@mateuszkwiecinski I am not sure whether we are both waiting for the other to proceed. As far as I know, there are still some conversations which I have not resolved because they are not fixed, but I could be wrong.

mateuszkwiecinski · 2023-07-25T19:06:41Z

Yes, sorry, I saw your comments and I still plan to address them as soon as I find some time. Both here and in the other PR I have opened.
If you wanted to speed things up, you should be able to push to the branch, if not, I should be able to take another round tomorrow or on Thursday :)

paul-dingemans · 2023-07-26T09:45:39Z

Yes, sorry, I saw your comments and I still plan to address them as soon as I find some time. Both here and in the other PR I have opened. If you wanted to speed things up, you should be able to push to the branch, if not, I should be able to take another round tomorrow or on Thursday :)

Sure, no problem. Just wanted to make sure that we are aligned.

mateuszkwiecinski · 2023-07-26T19:36:17Z

Oookay, I think I addressed all remaining items now. Please let me know in case I missed something 👀

If you manage to merge the new CI pipeline, I'll monitor build scans for a week or two to see if there are any bottlenecks or low-hanging fruits to improve CI build times 👀

paul-dingemans · 2023-07-28T15:13:09Z

I have merged #2136 earlier today. As of that a merge conflict appead in this PR. I am not sure how to resolve. So please have a look.

Closes pinterest#1888

… 1.8

…ains various java verstions https://jakewharton.com/build-on-latest-java-test-through-lowest-java/

If someone doesn't have required java installed Gradle will attempt to install one

The build needs less than 0.5GB of a java heap memory. There is no value in reserving more than half of available RAM just for the Gradle demon. It's better to keep Gradle daemon small and leave the memory available to other forked processes (i.e. tests)

CI doesn't modify files, there is no value in putting extra effort in efficiently tracking changed files. From test runs VFS comes with noticable performance impact on Windows build

shashachu · 2023-07-31T16:20:23Z

I think the checks have been updated properly now.

paul-dingemans · 2023-07-31T16:32:28Z

I think the checks have been updated properly now.

Looks like it. The build succeeded. I will merge.

…ous Java versions (pinterest#2120) * Replace testing on java 19 with testing on java 20 Closes pinterest#1888 * Unify `yml` files formating * Do NOT compile the project with various Java version, instead test agains various java verstions https://jakewharton.com/build-on-latest-java-test-through-lowest-java/ * Add `-add-opens` jvmArg to prevent ignored warnings * Migrate away deprecated `java.net.URL` constructor * Setup toolchain resolver If someone doesn't have required java installed Gradle will attempt to install one * Extract common build setup + fix memory settings The build needs less than 0.5GB of a java heap memory. There is no value in reserving more than half of available RAM just for the Gradle demon. It's better to keep Gradle daemon small and leave the memory available to other forked processes (i.e. tests) * Disable VFS on CI CI doesn't modify files, there is no value in putting extra effort in efficiently tracking changed files. From test runs VFS comes with noticable performance impact on Windows build * Update comment with extra clarification * Disable automatic build scans publishing on local builds * Update kotlin dev version * Fix `-PkotlinDev` not running tests agains configured kotlin version * Fix `-PkotlinDev` artifacts version * Run `-PkotlinDev` build as a separate workflow job in parallel Co-authored-by: paul-dingemans <paul-dingemans@users.noreply.github.com>

Goooler · 2023-08-01T03:50:20Z

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts

-tasks.withType<JavaCompile>().configureEach {
-    sourceCompatibility = JavaVersion.VERSION_1_8.toString()
-    targetCompatibility = JavaVersion.VERSION_1_8.toString()
+val requestedJdkVersion = project.findProperty("testJdkVersion")?.toString()?.toInt()


We should keep using lazy APIs, https://docs.gradle.org/current/javadoc/org/gradle/api/provider/ProviderFactory.html#gradleProperty-java.lang.String-

What's the value of doing extra abstraction if the property is consumed immediately, at the configuration time?

Lazy APIs are useful to take the burden off the single-threaded configuration phase and do more things during task execution time. The documentation: https://docs.gradle.org/current/userguide/lazy_configuration.html#lazy_properties doesn't mention our setup as an example use case that would benefit lazy configuration.
I'd still suggest using simpler version, as long as it's not deprecated, or adding extra complexity comes with any performance benefit 🤷

It's the Gradle recommending way, I'd prefer to follow the best practice.

It's the Gradle recommending way

Can you link where Gradle recommends using providers in use cases like this? I agree it's a general recommendation to:

wire Gradle models

wiring Task inputs & outputs

Avoiding doing work during configuration phase
None of them are applicable here, and blindly using lazy APIs doesn't bring any value.

I can also link this: which says:

Avoid simplifying calls like obj.getProperty().get() and obj.getProperty().set(T) in your code by introducing additional getters and setters.

which is exactly what you'd have to do if you used gradleProperty() api 🤷

An alternative approach to using native Gradle APIs (like I did), is to use kts delegates, in this case:val testJdkVersion: String? by project, but that's a syntactic sugar and doesn't change behavior (in context of being lazy or not) 🤷

I'll be happy to be proven wrong, otherwise forcing to use lazy for properties that are consumed only during configuration phase is useless 🤷

mateuszkwiecinski changed the title ~~Setup toolchains, compile project with Java 20 only, run test on various java versions~~ Setup toolchains, compile project with Java 20 only, run test on various Java versions Jul 9, 2023

mateuszkwiecinski commented Jul 9, 2023

View reviewed changes

mateuszkwiecinski marked this pull request as ready for review July 9, 2023 20:07

mateuszkwiecinski force-pushed the set_toolchains branch from b0cb889 to 98da6d2 Compare July 11, 2023 17:28

mateuszkwiecinski commented Jul 11, 2023

View reviewed changes

.github/workflows/pull-request-with-code.yml Show resolved Hide resolved

.github/workflows/pull-request-with-code.yml Outdated Show resolved Hide resolved

paul-dingemans reviewed Jul 11, 2023

View reviewed changes

mateuszkwiecinski force-pushed the set_toolchains branch from 98da6d2 to 34c5ae0 Compare July 15, 2023 06:00

mateuszkwiecinski commented Jul 15, 2023

View reviewed changes

paul-dingemans requested changes Jul 15, 2023

View reviewed changes

mateuszkwiecinski force-pushed the set_toolchains branch from 9c5ab50 to 171790c Compare July 15, 2023 22:03

mateuszkwiecinski commented Jul 15, 2023

View reviewed changes

build-logic/src/main/kotlin/ktlint-kotlin-common.gradle.kts Outdated Show resolved Hide resolved

paul-dingemans reviewed Jul 16, 2023

View reviewed changes

mateuszkwiecinski force-pushed the set_toolchains branch from 171790c to 2ab6b3b Compare July 16, 2023 08:21

mateuszkwiecinski commented Jul 16, 2023

View reviewed changes

mateuszkwiecinski force-pushed the set_toolchains branch from 414b307 to 0e5fc2a Compare July 16, 2023 19:58

mateuszkwiecinski force-pushed the set_toolchains branch from 0e5fc2a to 8f119c0 Compare July 26, 2023 11:37

paul-dingemans and others added 8 commits July 29, 2023 00:43

Replace testing on java 19 with testing on java 20

88bde93

Closes pinterest#1888

Fix toolchains. Compile project with Java 20, continue targeting Java…

c1a0e41

… 1.8

Unify yml files formating

7370ea0

Fix compiler warning

b03a4be

Do NOT compile the project with various Java version, instead test ag…

dbff741

…ains various java verstions https://jakewharton.com/build-on-latest-java-test-through-lowest-java/

Add -add-opens jvmArg to prevent ignored warnings

652aef7

Fix readme badge

9bf8592

Migrate away deprecated java.net.URL constructor

b4f6146

mateuszkwiecinski added 16 commits July 29, 2023 00:43

Setup toolchain resolver

bad36e7

If someone doesn't have required java installed Gradle will attempt to install one

Improve CI performance

cf3464b

Update workflow name

9313464

Move JDK compile/target version to libs.versions.toml file

12cdbf1

Add more explanation comments

711e6bf

Rename targetJavaVersion -> buildLogicTargetJavaVersion

b9d6b7a

Pre-register all testOnJdk test tasks for all LTS java versions

9de34cc

Disable VFS on CI

1931e09

CI doesn't modify files, there is no value in putting extra effort in efficiently tracking changed files. From test runs VFS comes with noticable performance impact on Windows build

Update comment with extra clarification

18f069f

Disable automatic build scans publishing on local builds

455b383

Update kotlin dev version

0ab55cc

Fix -PkotlinDev not running tests agains configured kotlin version

a49c0d5

Fix -PkotlinDev artifacts version

3a0abff

Run -PkotlinDev build as a separate workflow job in parallel

3ab1521

Add extra comment explaining CI runner settings

53bd98f

mateuszkwiecinski force-pushed the set_toolchains branch from 8f119c0 to 53bd98f Compare July 28, 2023 22:57

paul-dingemans merged commit 760279e into pinterest:master Jul 31, 2023
21 checks passed

mateuszkwiecinski deleted the set_toolchains branch July 31, 2023 17:20

Goooler reviewed Aug 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mateuszkwiecinski commented Jul 9, 2023 •

edited

paul-dingemans left a comment

mateuszkwiecinski left a comment

paul-dingemans left a comment

mateuszkwiecinski left a comment

paul-dingemans commented Jul 20, 2023

paul-dingemans commented Jul 25, 2023

mateuszkwiecinski commented Jul 25, 2023

paul-dingemans commented Jul 26, 2023

mateuszkwiecinski commented Jul 26, 2023

paul-dingemans commented Jul 28, 2023

shashachu commented Jul 31, 2023

paul-dingemans commented Jul 31, 2023

Goooler Aug 1, 2023

mateuszkwiecinski Aug 1, 2023 •

edited

Goooler Aug 1, 2023

mateuszkwiecinski Aug 1, 2023 •

edited

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

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

Conversation

mateuszkwiecinski commented Jul 9, 2023 • edited

Description

Checklist

paul-dingemans left a comment

Choose a reason for hiding this comment

mateuszkwiecinski left a comment

Choose a reason for hiding this comment

paul-dingemans left a comment

Choose a reason for hiding this comment

mateuszkwiecinski left a comment

Choose a reason for hiding this comment

paul-dingemans commented Jul 20, 2023

paul-dingemans commented Jul 25, 2023

mateuszkwiecinski commented Jul 25, 2023

paul-dingemans commented Jul 26, 2023

mateuszkwiecinski commented Jul 26, 2023

paul-dingemans commented Jul 28, 2023

shashachu commented Jul 31, 2023

paul-dingemans commented Jul 31, 2023

Goooler Aug 1, 2023

Choose a reason for hiding this comment

mateuszkwiecinski Aug 1, 2023 • edited

Choose a reason for hiding this comment

Goooler Aug 1, 2023

Choose a reason for hiding this comment

mateuszkwiecinski Aug 1, 2023 • edited

Choose a reason for hiding this comment

mateuszkwiecinski commented Jul 9, 2023 •

edited

mateuszkwiecinski Aug 1, 2023 •

edited

mateuszkwiecinski Aug 1, 2023 •

edited