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

Migrate kapt to ksp #2471

Merged
merged 16 commits into from
Mar 25, 2024
Merged

Migrate kapt to ksp #2471

merged 16 commits into from
Mar 25, 2024

Conversation

icrc-fdeniger
Copy link
Contributor

@icrc-fdeniger icrc-fdeniger commented Mar 12, 2024

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #2470

Description
To be polished.

  • move stdlib to 1.9.22 ( to be compatible with ksp 1.9.22)
  • use ksp instead of kapt

Alternative(s) considered
Have to run the demo in admin mode on windows but not possible due to our entreprise policy

Type
Bug fix

Screenshots (if applicable)

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

Copy link

google-cla bot commented Mar 12, 2024

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@icrc-fdeniger icrc-fdeniger changed the title Fix issue on windows with sqlite Fix issue on windows with sqlite and add configuration for FHIR Server Mar 12, 2024
@icrc-fdeniger icrc-fdeniger changed the title Fix issue on windows with sqlite and add configuration for FHIR Server DRAFT: Fix issue on windows with sqlite Mar 13, 2024
@icrc-fdeniger icrc-fdeniger changed the title DRAFT: Fix issue on windows with sqlite Fix issue on windows with sqlite Mar 13, 2024
@jingtang10 jingtang10 enabled auto-merge (squash) March 13, 2024 16:50
@jingtang10
Copy link
Collaborator

thanks for this pr this looks fine to me if tests pass

@jingtang10 jingtang10 changed the title Fix issue on windows with sqlite Migrate kapt to ksp Mar 13, 2024
@jingtang10
Copy link
Collaborator

@icrc-fdeniger i took the liberty of changing the title of the pr and added a commit that fixes the build - hopefully it'll get auto-merged once tests pass.

demo/build.gradle.kts Show resolved Hide resolved
engine/build.gradle.kts Outdated Show resolved Hide resolved
auto-merge was automatically disabled March 14, 2024 10:02

Head branch was pushed to by a user without write access

@icrc-fdeniger
Copy link
Contributor Author

push a new version with something similar to other plugins.
3 tests are failing with this error:
java.lang.IllegalStateException: Illegal connection pointer 4411. Current pointers for thread Thread[arch_disk_io_1 @coroutine#204,5,SDK 24] []
Will try to investigate today. don't know the root cause for now

@icrc-fdeniger
Copy link
Contributor Author

I fixed some unit tests:

  • NpmFileManagerTest: was failing on Windows due to File separator.
  • FhirSyncWorkerTest: I add to start in test mode and clean up FhirEngineProvider to let the test pass. Without this modification I get the error java.lang.IllegalStateException: Illegal connection pointer 4411

Some tests are still failing and I suppose it's due to my locale:

  • For instance QuestionnaireUiEspressoTest (expected: 2005-01-05T06:10 but was : 2005-01-05T18:10)

@MJ1998
Copy link
Collaborator

MJ1998 commented Mar 14, 2024

I believe the test failing in QuestionnaireUiEspressoTest is dateTimePicker_shouldSetAnswerWhenDateAndTimeAreFilled. It's a flaky test. Even this PR has this problem.
@jingtang10 Is someone working on this flaky test ?

Copy link
Collaborator

@MJ1998 MJ1998 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for testing these out on Windows.

@jingtang10 jingtang10 enabled auto-merge (squash) March 15, 2024 10:13
@jingtang10
Copy link
Collaborator

jingtang10 commented Mar 15, 2024

Some tests are still failing and I suppose it's due to my locale:

thanks for these! we shouldn't have tests that fail in different locales if that's the problem we should def fix it.

@jingtang10
Copy link
Collaborator

I believe the test failing in QuestionnaireUiEspressoTest is dateTimePicker_shouldSetAnswerWhenDateAndTimeAreFilled. It's a flaky test. Even this PR has this problem. @jingtang10 Is someone working on this flaky test ?

got ya - can you take a look at this?

@vorburger
Copy link
Member

FYI I have created #2485 re. the flaky QuestionnaireUiEspressoTest$dateTimePicker_shouldSetAnswerWhenDateAndTimeAreFilled test 12/24h problem.

@icrc-fdeniger
Copy link
Contributor Author

Hello, thanks for your answers. Is there another step to let this PR be accepted ? I see that Kokoro step is still pending but I can't have the details.

@vorburger
Copy link
Member

Hello, thanks for your answers. Is there another step to let this PR be accepted ?

I (only now, didn't look before) noticed that it already has 2 approvals and auto-merge, so it just needs a green build, then it will go in.

I see that Kokoro step is still pending but I can't have the details.

There were some problems which were just solved, see #2485 and #2488, I've tried to click Update branch to see if it helps.

@vorburger
Copy link
Member

@icrc-fdeniger following the merge of #2463 there is a small conflict - can you have a look and (manually) resolve that? Tx!

@jingtang10 jingtang10 merged commit d1d0086 into google:master Mar 25, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

demo application doesn't start on windows
5 participants