FNX-14449 ⁃ Gradle configuration avoidance #13268
Conversation
NotWoods
force-pushed
the
configuration-avoidance
branch
3 times, most recently
from
1f88418
to
b3d19b2
Compare
Codecov Report
@@ Coverage Diff @@
## master #13268 +/- ##
============================================
+ Coverage 28.35% 28.36% +0.01%
Complexity 1033 1033
============================================
Files 418 418
Lines 16826 16826
Branches 2190 2190
============================================
+ Hits 4771 4773 +2
+ Misses 11710 11708 -2
Partials 345 345
Continue to review full report at Codecov.
|
data-sync-user
changed the title
|
@mcomella you were recently looking at task parallelization, is this something you could review? |
|
@NotWoods Do you happen to know what the savings are here or how a similar change has impacted android-components? |
|
We implemented similar changes in AC.
|
There was a problem hiding this comment.
I learned a lot from this PR. :) I have a few open questions before I can r+ though.
I'm a little concerned that it's easy to cause configuration avoidance to break (from eagerly realizing a few tasks or using the wrong APIs) but I guess that's better than not having configuration avoidance at all. I'm also concerned that our current code is written incorrectly (e.g. the buildTranslationArray task) such that moving to configuration avoidance might break it.
If I made this change, I probably wouldn't have been too careful testing so I figured I should ask: should we do more thorough testing on the tasks that we're changing and their integration into the build lifecycle?
app/build.gradle
Outdated
| @@ -735,5 +738,7 @@ tasks.register("updateCookiesExtensionVersion", Copy) { task -> | |||
| updateExtensionVersion(task, 'src/main/assets/extensions/cookies') | |||
| } | |||
|
|
|||
| preBuild.dependsOn updateAdsExtensionVersion | |||
| preBuild.dependsOn updateCookiesExtensionVersion | |||
| preBuild.configure { | |||
There was a problem hiding this comment.
What's preBuild? Does this run every time we execute a gradle command? Or is it smart and only executes before Android compilation jobs?
There was a problem hiding this comment.
I'm not sure where preBuild is set up. I tried to keep the logic consistent by hooking into configure, but we can also keep this line as is for now.
app/build.gradle
Outdated
| } | ||
| } | ||
|
|
||
| task buildTranslationArray { | ||
| tasks.register('buildTranslationArray') { |
There was a problem hiding this comment.
I don't see this called from anywhere and it doesn't set dependencies but it seems to correctly fill the SUPPORTED_LOCALE_ARRAY: do you know why this works and why it will continue to work after lazy configuration?
I'm wondering if this has been running in the configuration phase this whole time (e.g. the code is not in doLast) and lazily registering it will prevent it from working.
There was a problem hiding this comment.
I think you're correct and the array is being populated as a side effect. I'll revert this line and file an issue.
NotWoods
force-pushed
the
configuration-avoidance
branch
from
b3d19b2
to
a392428
Compare
There was a problem hiding this comment.
lgtm. The tasks we've updated are all low risk – helper tasks, static analysis, etc. – expect for our change to the tests that adds additional logging output, which I think is used for TC parsing, i.e. no major compile task was touched. I verified it was working correctly locally, rebased on master, so I think this is good to land.
No description provided.