Fix #1617: Remove source code genrules #5702
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This requires a whole bunch of import updates, and a lot of simplifications in BUILD.bazel files. Some now unnecessary macros have also been removed, as well as one wiki update.
|
Going to skip self-reviewing on this PR since it's quite a large number of files, and the way I did this change relied heavily on automation and verifying builds (so I'm rather confident in its correctness). PTAL @adhiamboperes and feel free to delegate as needed since I know this is a large PR. |
Coverage ReportResultsNumber of files assessed: 359 Exempted coverageFiles exempted from coverage
|
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: 19 MiB (old), 19 MiB (new), 356 bytes (Removed) APK download size (estimated): 17 MiB (old), 17 MiB (new), 1181 bytes (Added) Method count: 260321 (old), 260321 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6830 (old), 6830 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 356 bytes (Removed) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 8 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 469 bytes (Removed) Method count: 115840 (old), 115840 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5798 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 8 bytes (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) APK download size (estimated): 10 MiB (old), 10 MiB (new), 236 bytes (Added) Method count: 115846 (old), 115846 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5798 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 36 bytes (Removed) Method count: 115846 (old), 115846 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5798 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 0 bytes (No change) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
adhiamboperes
requested changes
Feb 14, 2025
There was a problem hiding this comment.
I have reviewed about 50% of these files, and mostly look good. I ran the code locally and my observation was:
- It appears that the Bazel plugin does not handle conflict resolution well, e.g.
- when there are 2 versions of the same dependency, such as version 2.20 and 2.3.1, then it shows an error wherever classes of that dependency are used.
- There is a possible conflic when multiple R classes are available, and I wonder if it has something to do with how
DataBinderMapperImplhandles generated bindings, or perharps how the plugin interprets the bindings.
public List<DataBinderMapper> collectDependencies() {
ArrayList<DataBinderMapper> result = new ArrayList<DataBinderMapper>(5);
result.add(new org.oppia.android.app.databinding.DataBinderMapperImpl());
result.add(new org.oppia.android.app.databinding.adapters.DataBinderMapperImpl());
result.add(new org.oppia.android.app.ui.DataBinderMapperImpl());
result.add(new org.oppia.android.app.view.models.DataBinderMapperImpl());
result.add(new org.oppia.android.app.views.DataBinderMapperImpl());
return result;
}It seems that the generated R classes are prioritized based on the order of packages in the resulting list, such that the plugin can only resolve import org.oppia.android.app.databinding.R but not import org.oppia.android.app.ui.R, import org.oppia.android.app.view.models.R or import org.oppia.android.app.views.R.
- It is likely that
import org.oppia.android.app.ui.Rcan safely be replaced byorg.oppia.android.app.databinding.R
PTAL, thanks @BenHenning!
| ) | ||
|
|
||
| app_test( | ||
| oppia_android_test( |
There was a problem hiding this comment.
Noticed in build warnings that TextInputLayoutBindingAdaptersTest is missing a test target here. It seems I forgot to create one when I added the test suite in a previous PR.
There was a problem hiding this comment.
Good catch--I'll add this on the next pass for changes. I think I might've caught this in my modularization PR chain, though I'm actually surprised this wasn't caught by TestFileCheck. I'm assuming it might be because that script is only verifying against production Kotlin files and not Java. :) I suppose I could fix the check in this PR, but ideally we move away from Java long-term, anyway, so it may not actually be worth the effort.
I think I had some success migrating these Java files to Kotlin in the past, as well...
| @@ -40,6 +39,7 @@ import org.oppia.android.app.devoptions.DeveloperOptionsModule | |||
| import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule | |||
| import org.oppia.android.app.player.state.itemviewmodel.SplitScreenInteractionModule | |||
| import org.oppia.android.app.shim.ViewBindingShimModule | |||
| import org.oppia.android.app.test.R | |||
There was a problem hiding this comment.
Import does not appear to be used (greyed out in IDE, and test ran fine when removed). It seems that the test is using local package R file instead.
There was a problem hiding this comment.
Ditto all tests in this package.
There was a problem hiding this comment.
This is actually really surprising to me. I can try to remove the imports and re-run the tests. It may be the case that a LOT of code can technically rely on the locally produced R files (it's a really significant problem with android_library in my opinion as it adds a ton of unnecessary build complexity and time, but we don't have an obvious mitigation for now).
This is a great catch. My goal for the PR was reaching code parity with the actual build today, but I welcome simplifications like this.
Thanks @adhiamboperes! Usually, we should never have multiple versions ( I'm also not sure if the error you're describing is due to duplication--are you seeing an actual sync failure or broken symbols in Oppia Android code? Dependency management is much more complex in the Bazel world than Gradle since there are thousands of targets to build rather than ~6, so it's possible for multiple R files to exist with the same symbols but not be available for a given source file (if the corresponding Separately, do you want me to make changes before you finish reviewing the PR? I'm assuming it will be much easier to review incremental changes after a full review since then your marked review files will correctly diff against later changes. |
|
Unassigning @BenHenning since a re-review was requested. @BenHenning, please make sure you have addressed all review comments. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes #1617
This PR removes all
genrules (https://bazel.build/reference/be/general#genrule) that generate Kotlin or Java source code in the repository. This corresponds to almost every app/ directory test and production file, including the few Java files used for binding adapters. While not every file was being regenerated, the vast majority were.This change has fairly substantial implications:
.bazelprojectfile. This didn't work before because thegenrules would create a new file either with a different name or different path, so the plugin didn't know to associate the two during sync.SourceFileattribute will be stripped or not included in corresponding Dex files that are deployed with the app).genrules themselves ran very quickly, the bigger win is the large decrease in targets that need to be analyzed during a large build.Rlocally, from a common package, or from a very specific package (such as for tests). Databinding imports will face a similar issue. With symbolization in Android Studio, I'm hopeful this isn't too confusing to developers. In the long-term, we will try to remove the need for explicit imports entirely (and do away with databinding), but this sort of work will be on the order of years, not weeks or months, to complete. This will be the medium- to long-term state for development moving forward.This change also includes a few noteworthy simplifications:
app_testhas been removed entirely in favor of usingoppia_android_testdirectly. This is the direction we want to go in general for simpler test definitions, so it's nice to see this was actually possible with the removal oftest_with_resources. Using this for resource cases is a bit awkward right now due to needing to include theDataBinderMapperImpl.javabridge class for databinding, but hopefully this will be fixable in the future (android_local_test doesn't work with data-binding. #1683).app/BUILD.bazeldue to the extra resources-specific splits of different libraries being no longer necessary. The notion of migrated targets were also removed since they were actually never needed with how Bazel'sglobbehaves.test/andsharedTest/tests (rather than having two), which is cleaner.There was one wiki change needed since
app_testis no longer available to use, and it has been clarified to better explain how to set up a test for resources vs. not.For compatibility, this further cements Gradle no longer being able to be used for builds, but that's no longer a concern since we can't build with Gradle, anyway.
It's important to note that all of the updated import statements are reflecting the existing reality of the Bazel build configuration (i.e. these are exactly the import statements that Bazel was using via the
genruletargets). Android resource processing is very complex, and even more so in the Bazel world. This means a lot of theRsymbolic references are actually duplicated many times in the build pipeline, so there are otherRfiles that can be used. This was an approach I took a few months ago to try and remove thegenrules while maintaining Gradle compatibility. I didn't take that approach here for simplicity (it was more important to get this change in), but it's quite likely the import statements will see more iterations as we continue with modularization work. One useful goal may be to try and consolidate to local packageRfiles to avoid needing an import at all (and thus remove potential confusion that there are manyRfiles generated throughout the build graph), but there are other possibilities that can be explored as well.Essential Checklist
For UI-specific PRs only
N/A -- It's expected that this infrastructure-only change has no real impact on UIs, and this is assumed to be the case as per the passing tests.