Skip to content

Extract the bulk of shared gradle logic into script plugins [ci full].#4194

Merged
rfk merged 1 commit intomainfrom
more-gradle-cleanups
Jul 20, 2021
Merged

Extract the bulk of shared gradle logic into script plugins [ci full].#4194
rfk merged 1 commit intomainfrom
more-gradle-cleanups

Conversation

@rfk
Copy link
Copy Markdown
Contributor

@rfk rfk commented Jun 9, 2021

Prior to this commit, the build.gradle for each individual
project contained copy-pasted logic for configuring Android
and protobuf. Some of them were even using slightly different
versions of the same dependencies.

This this commit, there are two new gradle scripts that encapsulate
this shared logic:

  • ./build-scripts/component-common.gradle, for basic Android
    and Kotlin setup.
  • ./build-scripts/protobof-common.gradle, for configuration specific
    to the protobuf plugin.

Hopefully this will make the logic easier to maintain going forward.

There may be gradle-related reasons that we shouldn't do this, so
I will reach out to some Android experts for a review. But even if this
makes our builds slightly slower, I think its a win overall for a team
of non-gradle-experts who need tomaintain this stuff.

@rfk rfk force-pushed the more-gradle-cleanups branch from 88be94a to 00626f8 Compare June 9, 2021 07:21
// }
// }
//
// However, it doesn't work when factored out into this script.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Specifically, if I try to do the previous snippet in this build script, gradle tries to look up org.jetbrains.... as a property on $project, which fails. Maybe there's some trick I don't know about to have it looked up as a global package name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

More concerningly, I don't know how to test whether this change has broken whatever AndroidX thing it was introduced to address; our ./gradlew test still passes and an a-c test run seems to be going OK so far. I'll try a Fenix build with locally-published components as well for completeness.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll try a Fenix build with locally-published components as well for completeness.

I can't build Fenix against latest a-c right now, looks like there might be a refactor in progress that needs updates in Fenix. Will try again later, but definitely want to do a full test run on Fenix before merging this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It took me a bit of fiddling around to find the right combo of branches, but I have successfully build Fenix against a local publish of appservices and a-c with these changes in place, and confirmed that sync- and accounts-related functionality is working as expected

testImplementation "org.mockito:mockito-core:$mockito_core_version"

androidTestImplementation "com.android.support.test:runner:$android_test_runner_version"
androidTestImplementation "com.android.support.test.espresso:$espresso_core_version"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A handful of our current components to not currently depend on all of these test dependencies, so this change will introduce a few potentially-spurious test dependencies in a few of our packages. That seems fine to me, but that may be because I wouldn't be the one paying the cost of those dependencies. Thoughts?

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #4194 (00626f8) into taskcluster-test-all-at-once (aa9766e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                      Coverage Diff                      @@
##           taskcluster-test-all-at-once    #4194   +/-   ##
=============================================================
  Coverage                         74.86%   74.86%           
=============================================================
  Files                                45       45           
  Lines                              4011     4011           
=============================================================
  Hits                               3003     3003           
  Misses                             1008     1008           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa9766e...00626f8. Read the comment docs.

@rfk rfk requested review from grigoryk, jhugman and mhammond June 9, 2021 07:32
@rfk
Copy link
Copy Markdown
Contributor Author

rfk commented Jun 9, 2021

@jhugman how do you feel about this from a Nimbus PoV?

Copy link
Copy Markdown
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This looks great to me, although I suggest you still wait for a review from someone who actually knows something about gradle

@rfk rfk force-pushed the taskcluster-test-all-at-once branch from aa9766e to 8d4fb69 Compare June 16, 2021 02:51
@rfk rfk force-pushed the more-gradle-cleanups branch 3 times, most recently from 9aaa8d5 to 0f173e1 Compare June 16, 2021 02:57
Copy link
Copy Markdown
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

FWIW: I'm very happy to see some order brought to these files. Thank you @rfk .

@rfk
Copy link
Copy Markdown
Contributor Author

rfk commented Jul 19, 2021

This is starting to get stale, and it really seems like an easy-to-reverse-if-we-hate-it change to me, so I'm going to rebase this and get it merged.

@rfk rfk force-pushed the taskcluster-test-all-at-once branch 2 times, most recently from 163a41d to e2be243 Compare July 19, 2021 03:42
Base automatically changed from taskcluster-test-all-at-once to main July 19, 2021 04:03
@rfk rfk force-pushed the more-gradle-cleanups branch 3 times, most recently from 6065443 to d27cb2c Compare July 19, 2021 04:20
@rfk
Copy link
Copy Markdown
Contributor Author

rfk commented Jul 19, 2021

(Actually, I still need to do a final test and build with Fenix before merging this, which I will do shortly)

@rfk rfk force-pushed the more-gradle-cleanups branch from d27cb2c to 12d82f0 Compare July 19, 2021 10:37
Prior to this commit, the `build.gradle` for each individual
project contained copy-pasted logic for configuring Android
and protobuf. Some of them were even using slightly different
versions of the same dependencies.

With this commit, there are two new gradle scripts that encapsulate
this shared logic:

* ./build-scripts/component-common.gradle, for basic Android
  and Kotlin setup.
* ./build-scripts/protobuf-common.gradle, for configuration specific
  to the protobuf plugin.

Hopefully this will make the logic easier to maintain going forward.
@rfk rfk force-pushed the more-gradle-cleanups branch from 12d82f0 to d633b2c Compare July 20, 2021 02:54
@rfk rfk merged commit c12b286 into main Jul 20, 2021
@rfk rfk deleted the more-gradle-cleanups branch July 20, 2021 03:56
rfk added a commit that referenced this pull request Jul 20, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 20, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 20, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 22, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 22, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 22, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 22, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 22, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 26, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
rfk added a commit that referenced this pull request Jul 26, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
lougeniaC64 pushed a commit that referenced this pull request Aug 23, 2021
This uses the new, cleaner factoring of our Android config from #4194
as an excuse to update some of our Android dependencies to bring them
inline with what's used in current android-components. It was much
easier than it has been in the past!
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.

4 participants