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

Gradle tweaks #4222

Merged
merged 14 commits into from
Jun 15, 2023
Merged

Gradle tweaks #4222

merged 14 commits into from
Jun 15, 2023

Conversation

3flex
Copy link
Contributor

@3flex 3flex commented Jun 2, 2023

Some Gradle plugin tweaks that can hopefully make it into 2.0 RC2.

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@3flex 3flex force-pushed the gradle-tweaks-2023-06-02 branch from 6ffadf1 to e14cb88 Compare June 3, 2023 04:09
@3flex 3flex force-pushed the gradle-tweaks-2023-06-02 branch from e14cb88 to 8fededc Compare June 3, 2023 05:03
@3flex 3flex force-pushed the gradle-tweaks-2023-06-02 branch from 8fededc to 8821713 Compare June 3, 2023 05:10
@3flex 3flex marked this pull request as ready for review June 3, 2023 05:11
project.plugins.withId("org.jetbrains.kotlin.js", kotlinPluginHandler)
project.plugins.withId("kotlin2js", kotlinPluginHandler)

project.plugins.withType(KotlinBasePlugin::class.java) {
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 should note that KotlinBasePlugin was only added in Kotlin 1.7.0, so if compatibility with older versions is needed, this should be reverted.

https://kotlinlang.org/docs/whatsnew17.html#updates-in-the-kotlin-gradle-plugin-api

Copy link
Collaborator

Choose a reason for hiding this comment

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

While I am fine with requiring Kotlin 1.7.0, could you split this change from the PR to merge the other Gradle plugin changes and we can discuss the Kotlin requirement in separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think its fine, I can't imagine folks upgrading SQLDelight to not also be upgrading kotlin

@AlecKazakova AlecKazakova merged commit 9012c42 into cashapp:master Jun 15, 2023
5 checks passed
@AlecKazakova
Copy link
Collaborator

Need to revert, this broke @shellderp 's build:

java.lang.NoClassDefFoundError: com/android/build/gradle/api/AndroidBasePlugin
        at app.cash.sqldelight.gradle.SqlDelightPlugin.apply(SqlDelightPlugin.kt:53)
        at app.cash.sqldelight.gradle.SqlDelightPlugin.apply(SqlDelightPlugin.kt:37)

AlecKazakova pushed a commit that referenced this pull request Jun 15, 2023
@AlecKazakova
Copy link
Collaborator

(feel free to reopen the changes with a fix, the android plugin is optional on the classpath which is the issue)

AlecKazakova pushed a commit that referenced this pull request Jun 15, 2023
@hfhbd
Copy link
Collaborator

hfhbd commented Jun 16, 2023

Biggest question: Why did the tests success? Don't we have tests without AGP?

@AlecKazakova
Copy link
Collaborator

i thought so too, I'm not sure

i wonder if tests specifically are getting it transitively pulled in because of all the magic we do for shading

@hfhbd
Copy link
Collaborator

hfhbd commented Jun 16, 2023

I took a look and I think, it is caused by this line:

I am already trying to fix the setup and getting failing tests.

@hfhbd hfhbd mentioned this pull request Jun 16, 2023
@3flex 3flex mentioned this pull request Jun 17, 2023
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.

None yet

4 participants