Adds KotlinPoet integration to Room Processing #137

gfreivasc · 2021-03-09T03:36:17Z

Proposed Changes

Adds write() method that accepts KotlinPoet's an instance of
FileSpec. Also adds KotlinPoet as an API dependency and an extension
function similar to that of JavaPoet.

Also verifies if processing backend is set for generating kotlin code
and fails for KotlinPoet if it isn't.

Testing

Test: ./gradlew test connectedCheck without benchmarks

Fixes

Fixes: b/182195680

googlebot · 2021-03-09T03:36:23Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-03-09T03:36:24Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

gfreivasc · 2021-03-09T03:38:18Z

@googlebot I signed it!

googlebot · 2021-03-09T03:38:21Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-03-09T03:38:21Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

yigit

overall lgtm, thanks!
Can you add a test to GeneratedCodeMatchTest to ensure we find these files properly as well.

https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:room/compiler-processing-testing/src/test/java/androidx/room/compiler/processing/util/GeneratedCodeMatchTest.kt

gfreivasc · 2021-03-09T13:35:34Z

Sure, working on it

gfreivasc · 2021-03-09T15:01:50Z

@yigit Is there anyway I can get the current generation path? I've stumbled upon two situations:

Java environment will not generate the Kotlin file, I could either ignore that scenario since it targets Java codebases (?).
Sources.kotlin() API isn't as simple as it does not take the qualified name, and internally the matcher expects the full path of the generated code instead of just the relative path, as seen on Source.kt#L132.

I am investigating what can be done about the latter but not sure how to handle the java scenario yet.

yigit · 2021-03-10T05:37:03Z

@yigit Is there anyway I can get the current generation path? I've stumbled upon two situations:

Java environment will not generate the Kotlin file, I could either ignore that scenario since it targets Java codebases (?).
We would need to solve it eventually but does not have to be in this CL

Sources.kotlin() API isn't as simple as it does not take the qualified name, and internally the matcher expects the full path of the generated code instead of just the relative path, as seen on Source.kt#L132.

I am investigating what can be done about the latter but not sure how to handle the java scenario yet.

We can maybe ignore kotlin code verification for now and instead throw an exception that it is not supported if an assertion receives a kotlin file w/ the java processor (to avoid false positives).
XTestInvocation has an isKsp property that can be used in the test side to avoid calling that method if desired.

I'm also not against changing Source.kotlin to require a qName but that would be inconsistent with how kotlin works so I'd rather don't do that for now. We can resolve it later.

gfreivasc · 2021-03-10T13:10:56Z

We can maybe ignore kotlin code verification for now and instead throw an exception that it is not supported if an assertion receives a kotlin file w/ the java processor (to avoid false positives).
XTestInvocation has an isKsp property that can be used in the test side to avoid calling that method if desired.

I've managed to run the tests by verifying if TestRunner's toString() is "java" and skipping the test if positive. This allows to validate for kapt which is expected to work.

I'm also not against changing Source.kotlin to require a qName but that would be inconsistent with how kotlin works so I'd rather don't do that for now. We can resolve it later.

I agree, and modified it to take the relative path instead of the absolute. All tests are passing (the CI looks to have broken but they're passing locally).

yigit

lgtm, just small nits in the test.

you can ignore presubmit, i think we broke playground again, it will work fine in AOSP if it is passing locally (just don't rebase until we fix playground:) )

yigit · 2021-03-10T18:31:34Z

...ssing-testing/src/test/java/androidx/room/compiler/processing/util/GeneratedCodeMatchTest.kt

+    @Test
+    fun successfulGeneratedKotlinCodeMatch() {
+        // java environment will not generate kotlin files
+        if (runTest.toString() == "java") return


Suggested change

if (runTest.toString() == "java") return

if (runTest.toString() == "java") throw AssumptionViolatedException()

so that test is reported as skippped instead of succeeded.

yigit · 2021-03-10T18:33:15Z

...ssing-testing/src/test/java/androidx/room/compiler/processing/util/GeneratedCodeMatchTest.kt

+    @Test
+    fun missingGeneratedKotlinCode_mismatch() {
+        // java environment will not generate kotlin files
+        if (runTest.toString() == "java") return


same here. eventually we should probaby change TestRunner instead of equals check but not important for this PR.
it also probably makes sense to move this to a helper function assumeCanGenerateKotlinCode that throws assumption violation.

yigit · 2021-03-10T18:44:12Z

lgtm, just small nits in the test.

you can ignore presubmit, i think we broke playground again, it will work fine in AOSP if it is passing locally (just don't rebase until we fix playground:) )

it should be fixed now: https://android-review.googlesource.com/c/platform/frameworks/support/+/1624289

gfreivasc · 2021-03-10T18:56:27Z

lgtm, just small nits in the test.
you can ignore presubmit, i think we broke playground again, it will work fine in AOSP if it is passing locally (just don't rebase until we fix playground:) )

it should be fixed now: https://android-review.googlesource.com/c/platform/frameworks/support/+/1624289

Ok, I will try rebasing now. I also applied precondition checks for running KotlinPoet on javaAP without kapt. I can remove those if you don't think that's applicable. Will push the changes once tests pass.

Edit: playground still broken to me after rebase

yigit

lgtm, thanks for adding the kapt parameter check as well.

if CL is fine, it should pass in AOSP which is what matters anyways. github might block it but if it is the only one, we'll skip.

Approving so that it gets mirrored into AOSP

yigit · 2021-03-11T05:31:02Z

the aosp build failed due to some unused imports.

Detected these failing tasks: [':room:room-compiler-processing:ktlint']
> Task :room:room-compiler-processing:ktlint
$SUPPORT/room/compiler-processing/src/main/java/androidx/room/compiler/processing/javac/JavacFiler.kt:22:1: Unused import

> Task :room:room-compiler-processing:ktlint
"checkstyle" report written to $OUT_DIR/androidx/room/room-compiler-processing/build/reports/ktlint/ktlint-checkstyle-report.xml

FAILURE: Build failed with an exception.

You can validate these locally as described here:
https://github.com/androidx/androidx/blob/androidx-main/CONTRIBUTING.md#validating-changes-locally

Looks like the lint changes are still breaking playground. Locally, you can workaround it by commenting the source sets in ../lint-checks/build.gradle.

Adds `write()` method that accepts KotlinPoet's an instance of `FileSpec`. Also adds KotlinPoet as an API dependency and an extension function similar to that of JavaPoet. Test: ./gradlew test connectedCheck without benchmarks Fixes: b/182195680

Modifies `KotlinSource` to take relative path instead of absolute path for testing, making it somewhat similar to how `JavaSource` takes qualified name.

Also adds tests for the changes and marks tests which skip backends it does not target with `AssumptionViolatedException`.

googlebot added the cla: no label Mar 9, 2021

googlebot added cla: yes and removed cla: no labels Mar 9, 2021

danysantiago requested a review from yigit March 9, 2021 04:37

yigit requested changes Mar 9, 2021

View reviewed changes

gfreivasc force-pushed the room-processing-kotlinpoet branch from ef89069 to 91f8f88 Compare March 9, 2021 15:52

gfreivasc requested a review from yigit March 9, 2021 16:29

gfreivasc force-pushed the room-processing-kotlinpoet branch from 91f8f88 to 3ed4fae Compare March 9, 2021 19:30

yigit reviewed Mar 10, 2021

View reviewed changes

gfreivasc force-pushed the room-processing-kotlinpoet branch from 3ed4fae to bcf7ef7 Compare March 10, 2021 19:04

yigit approved these changes Mar 11, 2021

View reviewed changes

gcvasconcelos-picpay added 4 commits March 11, 2021 11:13

Adds tests for KotlinPoet integration

8be3c95

Modifies `KotlinSource` to take relative path instead of absolute path for testing, making it somewhat similar to how `JavaSource` takes qualified name.

Adds javaAP precondition to KotlinPoet integration

f05bd16

Also adds tests for the changes and marks tests which skip backends it does not target with `AssumptionViolatedException`.

Removes unused import

96dc315

gfreivasc force-pushed the room-processing-kotlinpoet branch from bcf7ef7 to 96dc315 Compare March 11, 2021 14:54

copybara-service bot closed this in 4a23604 Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds KotlinPoet integration to Room Processing #137

Adds KotlinPoet integration to Room Processing #137

gfreivasc commented Mar 9, 2021 •

edited

googlebot commented Mar 9, 2021

google-cla bot commented Mar 9, 2021

gfreivasc commented Mar 9, 2021

googlebot commented Mar 9, 2021

google-cla bot commented Mar 9, 2021

yigit left a comment

gfreivasc commented Mar 9, 2021

gfreivasc commented Mar 9, 2021

yigit commented Mar 10, 2021

gfreivasc commented Mar 10, 2021 •

edited

yigit left a comment

yigit Mar 10, 2021

yigit Mar 10, 2021

yigit commented Mar 10, 2021

gfreivasc commented Mar 10, 2021 •

edited

yigit left a comment

yigit commented Mar 11, 2021

	if (runTest.toString() == "java") return
	if (runTest.toString() == "java") throw AssumptionViolatedException()

Adds KotlinPoet integration to Room Processing #137

Adds KotlinPoet integration to Room Processing #137

Conversation

gfreivasc commented Mar 9, 2021 • edited

Proposed Changes

Testing

Fixes

googlebot commented Mar 9, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Mar 9, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

gfreivasc commented Mar 9, 2021

googlebot commented Mar 9, 2021

google-cla bot commented Mar 9, 2021

yigit left a comment

Choose a reason for hiding this comment

gfreivasc commented Mar 9, 2021

gfreivasc commented Mar 9, 2021

yigit commented Mar 10, 2021

gfreivasc commented Mar 10, 2021 • edited

yigit left a comment

Choose a reason for hiding this comment

yigit Mar 10, 2021

Choose a reason for hiding this comment

yigit Mar 10, 2021

Choose a reason for hiding this comment

yigit commented Mar 10, 2021

gfreivasc commented Mar 10, 2021 • edited

yigit left a comment

Choose a reason for hiding this comment

yigit commented Mar 11, 2021

gfreivasc commented Mar 9, 2021 •

edited

gfreivasc commented Mar 10, 2021 •

edited

gfreivasc commented Mar 10, 2021 •

edited