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

chore: Update Compose libraries and Compiler. #171

Merged
merged 1 commit into from Jul 12, 2022
Merged

Conversation

arriolac
Copy link
Member

@arriolac arriolac commented Jul 8, 2022

Compose now supports independent versioning: https://android-developers.googleblog.com/2022/06/independent-versioning-of-Jetpack-Compose-libraries.html

  • Updates compose compiler to the stable version (1.2.0)
  • Updates compose libraries to the latest 1.2.0 rc (1.2.0-rc03)
  • Updates target/compile SDK to 32

@barbeau
Copy link
Contributor

barbeau commented Jul 11, 2022

Looks like Lint is now flagging an error in Polygon.kt:
https://github.com/googlemaps/android-maps-compose/runs/7285245949?check_suite_focus=true#step:5:445

Lint found 1 error and 3 warnings. First failure:
/home/runner/work/android-maps-compose/android-maps-compose/maps-compose/src/main/java/com/google/maps/android/compose/Polygon.kt:79: Error: Call requires API level 24 (current min is 21): java.lang.Iterable#forEach [NewApi]
holes.forEach {

Seems like it's this problem where it's using Java 8 forEach instead of Kotlin:
https://stackoverflow.com/a/44752239/937715

I can confirm if I change the code to:

                for (h in holes) {
                    addHole(h)
                }

...and then run ./gradlew :maps-compose:lintDebug I no longer get the lint error.

@arriolac Do you want to fix this issue in this PR or should we address it separately?

@arriolac
Copy link
Member Author

arriolac commented Jul 11, 2022

Will address that in the same PR—I think there's a different forEach function that is part of kotlin collections that we can use here. Thanks @barbeau

@arriolac
Copy link
Member Author

Hmm looks like there's a separate error now? Will dig into.

@barbeau
Copy link
Contributor

barbeau commented Jul 11, 2022

I think it's (at least) the same error still:
https://github.com/googlemaps/android-maps-compose/runs/7290216852?check_suite_focus=true#step:5:481

Lint found 1 error and 3 warnings. First failure:
Fix the issues identified by lint, or add the following to your build script to proceed with errors:
/home/runner/work/android-maps-compose/android-maps-compose/maps-compose/src/main/java/com/google/maps/android/compose/Polygon.kt:79: Error: Call requires API level 24 (current min is 21): java.lang.Iterable#forEach [NewApi]
...
holes.forEach { hole ->

Locally I tried:

                holes.forEach { hole ->
                    addHole(hole)
                }

...as well and got the same Lint error as the original code. The for (h in holes) { version works though - I didn't dig any further beyond that.

Change-Id: I449263cbbea517218521ca002d2bb1ec23bd08c7
@arriolac
Copy link
Member Author

arriolac commented Jul 12, 2022

@barbeau looks like some of the MapInColumnTests are failing on this version bump. Since this won't cut a release, should we go ahead and merge this and address the failing tests separately? I'll file an issue.

com.google.maps.android.compose.MapInColumnTests > testScrollColumn_MapCameraRemainsSame[test(AVD) - 10] FAILED 
	java.lang.AssertionError: Assert failed: The component is displayed!
	at androidx.compose.ui.test.AssertionsKt.assertIsNotDisplayed(Assertions.kt:51)

@barbeau
Copy link
Contributor

barbeau commented Jul 12, 2022

@arriolac Agreed, let's merge this and address the test failures separately. FWIW, when running this PR locally all the tests pass. I saw test failures on CI in PR https://github.com/googlemaps/android-maps-compose/runs/7283340731 (even trying multiple runs) that also passed locally, although after the PR was merged CI passed. So it seems like the tests are a bit flaky in general.

Copy link
Contributor

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

LGTM

@barbeau barbeau merged commit 2f7223c into main Jul 12, 2022
@barbeau barbeau deleted the chris/compose/update branch July 12, 2022 18:12
@arriolac arriolac mentioned this pull request Jul 12, 2022
@arriolac
Copy link
Member Author

@arriolac Agreed, let's merge this and address the test failures separately. FWIW, when running this PR locally all the tests pass. I saw test failures on CI in PR https://github.com/googlemaps/android-maps-compose/runs/7283340731 (even trying multiple runs) that also passed locally, although after the PR was merged CI passed. So it seems like the tests are a bit flaky in general.

Yeah it looks isolated to CI. I'm able to see tests pass locally as well

@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 2.5.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants