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

bump kotlin to 1.5.31, bump ktlint to 0.43.2 #597

Merged
merged 2 commits into from
Aug 23, 2022
Merged

bump kotlin to 1.5.31, bump ktlint to 0.43.2 #597

merged 2 commits into from
Aug 23, 2022

Conversation

AleksanderBrzozowski
Copy link
Contributor

Based on this, I decided to create a PR that would bump ktlint to newer version. Instead of trying to update to the newest version in one PR, I find it more convenient to split the update into more smaller parts.

Ktlint 0.43.2 requires at least kotlin 1.4 API. That's why I needed to change the minimum required gradle version to 6.8.

In next PR I would try to update gradle from 7.1.1 -> 7.2 :)

set the minimum version of gradle to 6.8

previous versions of gradle don't support kotlin 1.4. Kotlin 1.4 is the minimum for ktlint 0.43.2

add changelog
@@ -10,22 10,11 @@ import org.jlleitschuh.gradle.ktlint.testdsl.build
import org.jlleitschuh.gradle.ktlint.testdsl.project
import org.junit.jupiter.api.DisplayName

@GradleTestVersions(minVersion = "6.6.1")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed, because min gradle version is 6.8

* See https://docs.gradle.org/6.6-milestone-3/userguide/configuration_cache.html#config_cache:requirements:build_listeners
* ```
*/
private val maxProblemsFlag = "-Dorg.gradle.unsafe.configuration-cache.max-problems=2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if it is still applicable, I run the tests with the value set to 0 and everything worked correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to restore and set the value to 0?

@AleksanderBrzozowski
Copy link
Contributor Author

@JLLeitschuh Could you review the PR? 🙏

@AleksanderBrzozowski
Copy link
Contributor Author

@JLLeitschuh up :)

@@ -63,6 63,6 @@ open class KtlintBasePlugin : Plugin<Project> {
}

companion object {
const val LOWEST_SUPPORTED_GRADLE_VERSION = "6.0"
const val LOWEST_SUPPORTED_GRADLE_VERSION = "6.8"
Copy link
Owner

Choose a reason for hiding this comment

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

Is this consistent with the Android Gradle Plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I am not sure what you mean, but we need at least 6.8 version of gradle to make it work with ktlint 0.43.2.
Here is the table
image

Copy link
Owner

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

Just the one question, other than that, this looks good to me

@AleksanderBrzozowski
Copy link
Contributor Author

@JLLeitschuh Can you once again run the tests against windows?
They failed with some strange error, I hope that its not related to the changes introduced in this PR :)

@JLLeitschuh
Copy link
Owner

It's not. Windows fail with an OOM error that I haven't figured out yet

@AleksanderBrzozowski
Copy link
Contributor Author

So, are we gonna merge this PR, even with failing windows build? ;)

@JLLeitschuh
Copy link
Owner

Thank you for your contribution. Sorry for taking so long. I've been super busy with Black Hat and DEFCON prepping as a speaker the past month.

@JLLeitschuh JLLeitschuh merged commit 38f2260 into JLLeitschuh:master Aug 23, 2022
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