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.

Sign up for GitHub

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

fix: react android kotlin plugin version conflict #34255

Closed
wants to merge 4 commits into from
Closed

fix: react android kotlin plugin version conflict #34255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62e65ed
chore: added kotlin version to gradle.properties
hurali97 Jul 23, 2022
5dee391
fix: moved kotlin plugin to build script to use kotlin version
hurali97 Jul 23, 2022
94dd5b1
fix: added apply kotlin plugin
hurali97 Jul 31, 2022
b781cc2
Merge branch 'facebook:main' into react-android/kotlin-plugin
hurali97 Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ReactAndroid/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,15 @@
* LICENSE file in the root directory of this source tree.
*/

buildscript {
dependencies {
classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:${rootProject.hasProperty("kotlinVersion") ? rootProject.ext.kotlinVersion : KOTLIN_VERSION}"
}
}
Comment on lines +8 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will have unwanted side effects. Specifically mixing buildscript{} and plugins{} in Gradle is not advised.

It seems like a workaround that we could live with 🤔 But I would look for a solution that uses only plugins{} if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I looked, having dynamic version in plugins isn't quite possible. Since, we require to use the kotlinVersion from root level if that's present.

See here

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a note on why this is needed otherwise we'll forget about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the reason to go with this approach is:

use the kotlinVersion defined in top level build.gradle. If it's not defined, we use the default(1.6.10 as of now) defined in gradle.properties. We couldn't add dynamic version using the plugins DSL because of the constrained syntax, so we had to introduce the buildScript for this. See here

Should I add it as a comment in the build.gradle or are we good here?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're good here 👍
I'm looking into cleaning this up a bit in the future so this will hopefully go.


plugins {
id("com.android.library")
id("com.facebook.react")
id("org.jetbrains.kotlin.android") version "1.6.10"
id("maven-publish")
id("de.undercouch.download")
}
Expand Down Expand Up @@ -484,3 +489,5 @@ afterEvaluate {
}
}
}

apply plugin: "org.jetbrains.kotlin.android"
3 changes: 3 additions & 0 deletions ReactAndroid/gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,8 @@ FOLLY_VERSION=2021.07.22.00
GLOG_VERSION=0.3.5
LIBEVENT_VERSION=2.1.12

# Plugins Versions
KOTLIN_VERSION=1.6.10

android.useAndroidX=true
android.enableJetifier=true