Skip to content

Conversation

@barbeau
Copy link
Collaborator

@barbeau barbeau commented Jun 15, 2021

This is Option 1 to fix #800 as mentioned in #800 (comment).

Option 2 is presented in PR #894.

@arriolac What are your thoughts on this vs. Option 2?

This is certainly a simpler fix that keeps the old configuration, but the potential downside is reduced performance. I don't know what the real-world impact on this project is.

Only one of these two PRs should be merged.


Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
  • Will this cause breaking changes to existing Java or Kotlin integrations? If so, ensure the commit has a BREAKING CHANGE footer so when this change is integrated a major version update is triggered. See: https://www.conventionalcommits.org/en/v1.0.0/

Fixes #800 🦕

@barbeau barbeau requested a review from arriolac June 15, 2021 19:34
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 15, 2021
@barbeau barbeau changed the title fix: Remove configureondemand fix: Remove Gradle configureondemand Jun 15, 2021
Copy link
Contributor

@arriolac arriolac left a comment

Choose a reason for hiding this comment

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

I'd prefer this fix given its simplicity. But for thoroughness, I'm curious how much this affects performance. Perhaps it might be good to run the following and compare execution speeds ./gradlew library:assembleDebug? Given that this project is fairly small already I imagine the deltas would be small.

@barbeau
Copy link
Collaborator Author

barbeau commented Jun 16, 2021

Perhaps it might be good to run the following and compare execution speeds ./gradlew library:assembleDebug? Given that this project is fairly small already I imagine the deltas would be small.

I ran ./gradlew clean, then closed the terminal, re-opened it, and ran ./gradlew library:assembleDebug three times for each configuration.

You're right, the difference in build time looks negligible:

Before (s) After (s)
5 7
6 6
5 6

@barbeau barbeau marked this pull request as ready for review June 23, 2021 22:04
@barbeau
Copy link
Collaborator Author

barbeau commented Jun 23, 2021

@arriolac This is ready for review when you have a chance

Copy link
Contributor

@arriolac arriolac 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 4b0b5db into main Jun 24, 2021
@barbeau barbeau deleted the sean/remove-configure-on-demand branch June 24, 2021 13:34
googlemaps-bot pushed a commit that referenced this pull request Jun 24, 2021
## [2.2.4](v2.2.3...v2.2.4) (2021-06-24)

### Bug Fixes

* Remove configureondemand ([#895](#895)) ([4b0b5db](4b0b5db)), closes [#800](#800)
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 2.2.4 🎉

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

cla: yes This human has signed the Contributor License Agreement. released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build fails with Android Gradle Plugin 4.1.0

3 participants