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

Update Gradle Plugin to 7.4.2 + Fix Overflowing Poetry #257

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

davissuber
Copy link
Contributor

@davissuber davissuber commented Oct 27, 2023

This pull request does the following:

  • Update Android Gradle Plugin to 7.4.2 to stay someone consistent with other open source libraries like RIBs
    • Specify Java language to version 11 for Gradle
    • Miscellaneous build script fixes
  • Attempt to address java.lang.StackOverflowError due graph generation #255 by tidying up parameter type resolution - we were previously generating unresolved generic types first and then fixing them post generation - however, this 2-step process causes a stack overflow error if the unresolved generic type is self-referencing - this pull request merge the 2-step parameter resolution process into one to avoid the crash.

This commit is to keep the build somewhat in line with
RIBs and other uber open source projects
Thie commit prefers the slightly newer style of gradle plugin
specification
This avoids the infinite poetry in
uber#255
val resolvedParameterTypes = method.parameterTypes
while (i < size) {
val parameter = builder.parameters[i]
val type = javaToKotlinType(resolvedParameterTypes[i])
Copy link
Contributor Author

@davissuber davissuber Oct 27, 2023

Choose a reason for hiding this comment

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

This is the real type we want, but we can only get it from method (which has the enclosing generic context), but not methodElement (which has the unresolved T as type). Hence, we pass method.parameterTypes into the overriding function.

funBuilder.addParameter(
ParameterSpec.builder(it.name, it.type.typeName.toKTypeName()).build())
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 toKTypeName eventually creates an infinite loop somewhere in kotlin poet due to the unresolved type references itself in the generic type parameter.

interface Child {

@motif.Objects
abstract class Objects : ObjectComponent<Bar>
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 the key ingredient of the repro case, that an Objects inherits a function that contains a self-referencing generic parameter

@@ -26,8 +30,8 @@ android {
}

compileOptions {
sourceCompatibility JavaVersion.VERSION_1_8
targetCompatibility JavaVersion.VERSION_1_8
sourceCompatibility JavaVersion.VERSION_11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how useful these are, much of the code is already in kotlin.

kotlin: "org.jetbrains.kotlin:kotlin-gradle-plugin:${versions.kotlin}",
ksp: "com.google.devtools.ksp:com.google.devtools.ksp.gradle.plugin:${versions.ksp}",
dokka: "org.jetbrains.dokka:dokka-gradle-plugin:${versions.dokka}",
mavenPublish: 'com.vanniktech:gradle-maven-publish-plugin:0.18.0',
spotless: "com.diffplug.spotless:spotless-plugin-gradle:5.11.0",
shadow: "com.github.jengelman.gradle.plugins:shadow:6.1.0",
shadow: "com.github.johnrengelman:shadow:8.1.0",
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 looks questionable - but it is still the same library - upgrade is needed for the new gradle plugin version.

These modules don't seem to use kapt in the gradle.

More importantly, having those lines prevented dokka from completing
due to gradle complaining about task dependencies when trying to
publish the change to local maven
@davissuber
Copy link
Contributor Author

I have tested 88c9c0a by successfully building uber's 3 big apps with it.

@muandrew
Copy link

muandrew commented Nov 3, 2023

Thanks for checking internally also.
Note for future changes/contributors the changes to the build files can be done in another PR that can be quickly accepted while we get some time to review changes to the logic.

@muandrew muandrew merged commit 6a7863d into uber:main Nov 3, 2023
1 check passed
@davissuber davissuber deleted the ds-bump-gradle branch November 4, 2023 00:35
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