-
Notifications
You must be signed in to change notification settings - Fork 533
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
Fix part of #1719, part of #3709: Add build stats CI workflow #4092
Conversation
This script primarily focuses on high-level differences, including: - Sizes (file + download) - New/old files, configurations, resource changes - Method counts
This workflow is set up to build multiple binaries for both develop & the local branch, then analyze and generate reports that are uploaded as artifacts and added as a comment to the PR body. This doesn't make a guarantee about exact differences since GitHub uses a detached HEAD for the commit that may not exactly match. This also introduces a new workflow for stats (that will initially only contain optional workflows since they may be very expensive/time consuming).
These allow some SDK properties to be shared across different dependencies.
This updates the script to no longer require an Android SDK path to be passed in since it now directly pulls in AAPT2 and directly performs the work of apkanalyzer using its internal library rather than calling through to the CLI library (since it's not exposed via android_sdk_repository). This also updates the report output to compute full comparisons for all APKs (rather than just the universal APK) and include it as part of the full summary (so that only 2 files are ever written regardless of the number of profiles being compared). This adds placeholder tests & documentation for all new utilities. This commit includes substantial changes to the build system: - A new dependency on https://github.com/oppia/archive-patcher - A split between production, test, and script Maven dependencies (where the license tools have been updated to only operate on production dependencies since those are the ones being shipped to users) - Two versions of Guava are now being pulled in since the scripts require a JRE version of Guava rather than the Android version used in production & test classes - rules_kotlin has been upgraded to beta 3 (from alpha 2), but the Kotlin version itself is stuck on 1.4 (to ensure interoperability with previous builds). While this change doesn't upgrade Kotlin, it does allow us to leverage better tooling per changes in rules_kotlin, including enforcing strict deps.
Also, since I forgot to add these notes in the previous commit: the executor has also been changed in this PR to support applications which block on stdout/stderr being consumed.
Conflicts: third_party/maven_prod_install.json
Remove duplicated workflow.
I'm not entirely sure why this is needed yet.
comment_body |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevUniversal APKAPK file size: 13 MiB (old), 13 MiB (new), -98039 bytes (difference) APK download size (estimated): 12 MiB (old), 12 MiB (new), -95809 bytes (difference) Method count: 166952 (old), 166635 (new), -317 (difference) Features: 2 (old), 2 (new), 0 (difference) Permissions: 6 (old), 6 (new), 0 (difference) Resources: 4907 (old), 4907 (new), 0 (difference)
Lesson assets: 71 (old), 71 (new), 0 (difference) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 13 MiB (old), 13 MiB (new), -98039 bytes (difference) Configuration hdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (difference) Configuration ldpiAPK file size: 54 KiB (old), 54 KiB (new), 0 bytes (difference) Configuration mdpiAPK file size: 51 KiB (old), 51 KiB (new), 0 bytes (difference) Configuration tvdpiAPK file size: 99 KiB (old), 99 KiB (new), 0 bytes (difference) Configuration xhdpiAPK file size: 64 KiB (old), 64 KiB (new), 0 bytes (difference) Configuration xxhdpiAPK file size: 74 KiB (old), 74 KiB (new), 0 bytes (difference) Configuration xxxhdpiAPK file size: 77 KiB (old), 77 KiB (new), 0 bytes (difference) AlphaUniversal APKAPK file size: 7973 KiB (old), 7954 KiB (new), -19603 bytes (difference) APK download size (estimated): 6883 KiB (old), 6864 KiB (new), -18911 bytes (difference) Method count: 71192 (old), 70932 (new), -260 (difference) Features: 2 (old), 2 (new), 0 (difference) Permissions: 6 (old), 6 (new), 0 (difference) Resources: 4064 (old), 4064 (new), 0 (difference)
Lesson assets: 71 (old), 71 (new), 0 (difference) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 7743 KiB (old), 7724 KiB (new), -19599 bytes (difference) Configuration hdpiAPK file size: 51 KiB (old), 51 KiB (new), 0 bytes (difference) Configuration ldpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (difference) Configuration mdpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (difference) Configuration tvdpiAPK file size: 87 KiB (old), 87 KiB (new), 0 bytes (difference) Configuration xhdpiAPK file size: 58 KiB (old), 58 KiB (new), 0 bytes (difference) Configuration xxhdpiAPK file size: 67 KiB (old), 67 KiB (new), 0 bytes (difference) Configuration xxxhdpiAPK file size: 70 KiB (old), 70 KiB (new), 0 bytes (difference) Alpha_kitkatUniversal APKAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) APK download size (estimated): 12 MiB (old), 12 MiB (new), -60161 bytes (difference) Method count: 167864 (old), 167373 (new), -491 (difference) Features: 2 (old), 2 (new), 0 (difference) Permissions: 6 (old), 6 (new), 0 (difference) Resources: 4907 (old), 4907 (new), 0 (difference)
Lesson assets: 71 (old), 71 (new), 0 (difference) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 13 MiB (old), 13 MiB (new), -61275 bytes (difference) Configuration hdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (difference) Configuration ldpiAPK file size: 54 KiB (old), 54 KiB (new), 0 bytes (difference) Configuration mdpiAPK file size: 51 KiB (old), 51 KiB (new), 0 bytes (difference) Configuration tvdpiAPK file size: 99 KiB (old), 99 KiB (new), 0 bytes (difference) Configuration xhdpiAPK file size: 64 KiB (old), 64 KiB (new), 0 bytes (difference) Configuration xxhdpiAPK file size: 74 KiB (old), 74 KiB (new), 0 bytes (difference) Configuration xxxhdpiAPK file size: 77 KiB (old), 77 KiB (new), 0 bytes (difference) Configuration standalone-hdpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) Configuration standalone-ldpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) Configuration standalone-mdpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) Configuration standalone-tvdpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) Configuration standalone-xhdpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) Configuration standalone-xxhdpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) Configuration standalone-xxxhdpiAPK file size: 13 MiB (old), 13 MiB (new), -61279 bytes (difference) |
The current comment is a bit too long, so adding additional collapsing per-flavor.
This reverts the split prod/test deps since it results in one version issues. This commit also fixes the nice byte formatting in the report not working correctly for negative sizes.
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 13 MiB (old), 13 MiB (new), -44 KiB (difference) APK download size (estimated): 12 MiB (old), 12 MiB (new), -44 KiB (difference) Method count: 166952 (old), 167601 (new), 649 (difference) Features: 2 (old), 2 (new), 0 (difference) Permissions: 6 (old), 6 (new), 0 (difference) Resources: 4907 (old), 4907 (new), 0 (difference)
Lesson assets: 71 (old), 71 (new), 0 (difference) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 13 MiB (old), 13 MiB (new), -44 KiB (difference) Configuration hdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (difference) Configuration ldpiAPK file size: 54 KiB (old), 54 KiB (new), 0 bytes (difference) Configuration mdpiAPK file size: 51 KiB (old), 51 KiB (new), 0 bytes (difference) Configuration tvdpiAPK file size: 99 KiB (old), 99 KiB (new), 0 bytes (difference) Configuration xhdpiAPK file size: 64 KiB (old), 64 KiB (new), 0 bytes (difference) Configuration xxhdpiAPK file size: 74 KiB (old), 74 KiB (new), 0 bytes (difference) Configuration xxxhdpiAPK file size: 77 KiB (old), 77 KiB (new), 0 bytes (difference) AlphaExpand to see flavor specificsUniversal APKAPK file size: 7973 KiB (old), 7977 KiB (new), 3413 bytes (difference) APK download size (estimated): 6883 KiB (old), 6887 KiB (new), 4007 bytes (difference) Method count: 71192 (old), 71244 (new), 52 (difference) Features: 2 (old), 2 (new), 0 (difference) Permissions: 6 (old), 6 (new), 0 (difference) Resources: 4064 (old), 4064 (new), 0 (difference)
Lesson assets: 71 (old), 71 (new), 0 (difference) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 7743 KiB (old), 7746 KiB (new), 3417 bytes (difference) Configuration hdpiAPK file size: 51 KiB (old), 51 KiB (new), 0 bytes (difference) Configuration ldpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (difference) Configuration mdpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (difference) Configuration tvdpiAPK file size: 87 KiB (old), 87 KiB (new), 0 bytes (difference) Configuration xhdpiAPK file size: 58 KiB (old), 58 KiB (new), 0 bytes (difference) Configuration xxhdpiAPK file size: 67 KiB (old), 67 KiB (new), 0 bytes (difference) Configuration xxxhdpiAPK file size: 70 KiB (old), 70 KiB (new), 0 bytes (difference) Alpha_kitkatExpand to see flavor specificsUniversal APKAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) APK download size (estimated): 12 MiB (old), 12 MiB (new), 26 KiB (difference) Method count: 167864 (old), 168335 (new), 471 (difference) Features: 2 (old), 2 (new), 0 (difference) Permissions: 6 (old), 6 (new), 0 (difference) Resources: 4907 (old), 4907 (new), 0 (difference)
Lesson assets: 71 (old), 71 (new), 0 (difference) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration hdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (difference) Configuration ldpiAPK file size: 54 KiB (old), 54 KiB (new), 0 bytes (difference) Configuration mdpiAPK file size: 51 KiB (old), 51 KiB (new), 0 bytes (difference) Configuration tvdpiAPK file size: 99 KiB (old), 99 KiB (new), 0 bytes (difference) Configuration xhdpiAPK file size: 64 KiB (old), 64 KiB (new), 0 bytes (difference) Configuration xxhdpiAPK file size: 74 KiB (old), 74 KiB (new), 0 bytes (difference) Configuration xxxhdpiAPK file size: 77 KiB (old), 77 KiB (new), 0 bytes (difference) Configuration standalone-hdpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration standalone-ldpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration standalone-mdpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration standalone-tvdpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration standalone-xhdpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration standalone-xxhdpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) Configuration standalone-xxxhdpiAPK file size: 13 MiB (old), 13 MiB (new), 23 KiB (difference) |
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Message so that this PR is not considered stale. I'm not actively working on it yet, but I don't want it to be closed. |
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Still planning to revive this (hopefully sometime next week). |
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Add a config to allow warnings-as-errors to be disabled for local development to make things easier. The default is still to treat them as errors (and that will be the build configuration for CI, so PRs shouldn't be mergeable with Kotlin build warnings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed latest merge changes--merge was clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed upstream merge (was clean).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-reviewed latest changes (merge). They actually didn't seem 100% correct, but the actual WORKSPACE file is correct. I haven't seem that sort of thing come up before; it's a bit strange.
Enabling auto-merge since there haven't been changes, the branch is up-to-date, and everything is resolved. Note that the new workflow should kick in starting tomorrow after the PR is merged; I will keep an eye out to make sure it actually runs as expected. |
Explanation
Fixes part of #1719 and #3709
This PR introduces a new script & CI workflow for computing build stats to compare both AABs and universal APKs between develop and the changes in a given PR, as part of fixing #1719 (though this PR doesn't cover everything outlined in that PR). This information is then detailed and uploaded as a CI build artifact, and summarized & posted as a comment in the PR. Some details included in the summary report:
The script supports computing differences for multiple "profiles" at the same time, and the CI workflow has been set up to compute four:
This workflow will be optional since it's very expensive to run (it has to assemble 8 builds, 6 of which are Proguarded). It also doesn't really need to be run in order to approve a PR, though reviewers may insist on waiting for large or suspicious changes
(such as PRs introducing new dependencies) to ensure the actual affected changes are as expected.
In order to mitigate this expense, the CI workflow runs on a scheduled cron job off of develop across all open PRs and checks them in a group. It runs at most once per day (based on https://github.com/orgs/community/discussions/55768#discussioncomment-5941720) so multiple changes to a PR will be picked up with a single check in the next cron run. Currently, it will run even for a PR that hasn't changed since the last run (but this is something that can be improved in the future if it needs to be). It's being scheduled for 2:30am (02:30) UTC which seems to have a few specific benefits:
The example output from these workflows can be observed in a few places:
Beyond that, implementing this utility involved several significant changes to various systems, including the build graph:
This PR is introducing a few new dependencies that, in turn, pull in a bunch of transitive dependencies. These are all due to the new
apkanalyzer
dependency. While it will affect licenses for this specific PR, once third-party dependencies for scripts are cleaned up in a downstream PR they will be moved out (since they are script-only dependencies).Separately, also note that the AAPT2 utility requires stdout to be processed continuously in order for the process to finish. This was one of the primary reasons CommandExecutorImpl was reworked in #4929.
For testing: most of the changes in this PR have been extensively manually tested. However, the new utilities are lacking significant automated tests. Since this utility is a nice-to-have for the rest of the Bazel PR chain, it's being prioritized to be merged in spite of lacking code coverage. #4971 has been filed to track adding these missing tests in the long-term.
Essential Checklist
For UI-specific PRs only
N/A -- This only affects CI workflows & the build system. Technically, some dependency changes in the build system could have UI effects, but there should be no such changes in this PR.