Fix #2643: Introduce canonical Bazel library structure & migrate util accessibility package #2775

BenHenning · 2021-02-23T22:07:55Z

This is a replacement/continuation of #2668.

This PR does a few things:

It introduces a canonical structure for laying out packages with both production & test dependencies (see https://github.com/oppia/oppia-android/pull/2668/files#r577336935) using the accessibility package under the utility module as an example (& as part of fixing Create a BUILD file for accessibility/ #2643).
It introduces new top-level package groups in the root BUILD.bazel to simplify visibility management & to provide us with fine tune control (see https://docs.bazel.build/versions/master/be/functions.html#package_group).
It refactors the codebase to more consistently switch between prod & test modules for the accessibility checker to ensure dependencies are properly hooked up.
It does some other minor cleanups in the new package.
It introduces a way to exclude migrated packages (using a glob).

One thing to note: long-term it's expected that modules will only be referenced in test dependencies & the root app binary targets. However, at the moment production modules need to be included in the main app library due to being tightly coupled with the Dagger component classes. This will continue to be the case until #1720 is finished (since then build graph inclusion of a module predicates whether it's included in the generated component).

Includes introducing new top-level package groups to simpify visibility management. This also establishes a pattern for excluding migrated production files.

BenHenning · 2021-02-23T22:08:08Z

This replaces #2668.

BenHenning · 2021-02-23T22:13:00Z

NB: I am expecting some Bazel failures, so they will need to be fixed in a follow-up commit once the run finishes.

rt4914

Approving for files for which I am the code-owner. Thanks.

FareesHussain

@BenHenning Added some suggestions and comments, PTAL.

FareesHussain · 2021-02-24T12:44:34Z

utility/BUILD.bazel

@@ -7,10 +7,15 @@ load("@dagger//:workspace_defs.bzl", "dagger_rules")
 load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")
 load("//utility:utility_test.bzl", "utility_test")

+MIGRATED_PROD_FILES = glob(["src/main/java/org/oppia/android/util/android/**/*.kt"])


We need to exclude the files inside accessibility

I guess you need to add a kdoc here similar to app/BUILD.bazel.

Suggested change

MIGRATED_PROD_FILES = glob(["src/main/java/org/oppia/android/util/android/**/*.kt"])

MIGRATED_PROD_FILES = glob(["src/main/java/org/oppia/android/util/accessibility/**/*.kt"])

Ah. I'm not sure how I missed this--thanks! Will update it locally a bit later and make sure everything's working as expected.

FareesHussain · 2021-02-24T12:54:39Z

utility/src/main/java/org/oppia/android/util/accessibility/AccessibilityCheckerImpl.kt

+  private val accessibilityManager by lazy {
+    context.getSystemService(Context.ACCESSIBILITY_SERVICE) as AccessibilityManager
+  }
+
+  override fun isScreenReaderEnabled(): Boolean {
+    return accessibilityManager.isEnabled
+  }


I guess you've missed adding the Kdocs for these two functions.

I see just one function: isScreenReaderEnabled. Am I maybe missing something?

Also, per https://developer.android.com/kotlin/style-guide#exception_overrides KDocs aren't needed on overrides. In general, we only add them in places where they add value, and method KDocs should always be describing the API introduced by a method. This fits better in the interface or superclass as that's outlining the API implementations need to follow.

FareesHussain · 2021-02-24T13:16:59Z

BUILD.bazel

+package_group(
+    name = "oppia_api_visibility",
+    packages = [
+        "//...",


Isn't this similar to oppia_binary_visibility below?

if not what packages does each cover, when do we need to use oppia_api_visibility & oppia_binary_visibility

Hmm I tried making this clear in the documentation, but maybe it could use a bit more work. We'll generally be using API visibility unless something should only be accessible to the binary. I split them out to make the visibility groups themselves more explicit & to provide more control.

FareesHussain · 2021-02-24T13:21:36Z

utility/src/main/java/org/oppia/android/util/accessibility/BUILD.bazel

+    srcs = [
+        ":AccessibilityCheckerImpl.kt",
+    ],
+    visibility = ["//visibility:private"],


Do we need to specify this as it is private by default?

Nope--good point. I'll remove this when I address the other code-based changes.

FareesHussain

LGTM, thanks.

anandwana001

Approving for files for which I am the code-owner. Thanks.

fsharpasharp · 2021-02-26T16:48:42Z

BUILD.bazel

@@ -1,4 +1,50 @@
 # TODO(#1532): Rename file to 'BUILD' post-Gradle.


Will we be replacing all instances of //visibility:public with one of these package groups?

No, I don't think so. We may want to replace all instances with a package group, but the ones here are specifically corresponding to the types of libraries introduced in this PR.

FareesHussain · 2021-02-27T16:01:36Z

utility/src/main/java/org/oppia/android/util/accessibility/BUILD.bazel

+kt_android_library(
+    name = "impl",
+    srcs = [
+        ":AccessibilityCheckerImpl.kt",


Why use :<file_name> here

oppia-android/app/src/main/java/org/oppia/android/app/viewmodel/BUILD.bazel

Line 21 in 72bc897

"ObservableViewModel.kt",

Here I've used only file name

Mostly auto-complete. I believe they're equivalent, but it's less code-smell to omit the ':'. Will do that in a follow-up commit.

FareesHussain · 2021-02-27T16:34:55Z

app/BUILD.bazel

@@ -533,6 +532,7 @@ kt_android_library(
        "//third_party:androidx_databinding_databinding-common",
        "//third_party:androidx_databinding_databinding-runtime",
        "//third_party:circularimageview_circular_image_view",
+        "//utility/src/main/java/org/oppia/android/util/accessibility",


I've missed this earlier.

Why are we adding the deps in app/BUILD.bazel
and why not utility/BUILD.bazel

Because this is where the dependency is needed. It's not used by anything in utility, and to some extent, that's expected since utility is a collection of utility libraries to be used elsewhere in the codebase.

Does that mean if there are any packages, which are directly used in the app module then we can directly add this in the app/BUILD.bazel?

but I think it would be better if we can pass it through utility/BUILD.bazel

Yes, that's how it should be. We're planning to get rid of all the top-level BUILD files--libraries should always depend as directly on the thing they need as possible. Occasionally we may introduce libraries that export parts of their implementation (which can be useful when versioning APIs), but that's something that would come after everything is modularized.

Happy to chat about this further, and I'm sure @fsharpasharp can provide some insight here, too.

Addresses review comment.

BenHenning · 2021-03-05T03:53:07Z

I think all of your comments are now addressed @FareesHussain. PTAL & resolve if they are satisfactorily addressed.

FareesHussain

LGTM, thanks.

BenHenning · 2021-03-06T02:08:09Z

Thanks! Looking at failures.

BenHenning · 2021-03-06T02:08:58Z

Hmm, Bazel failures seem more of the incorrect affected targets computation. Gonna see if I can repro locally & actually fix the script.

Gradle failure seems like a weird kapt flake I haven't seen before.

BenHenning · 2021-03-06T04:52:02Z

Re-running tests to unblock down-stream work. Would like to try and investigate that Bazel issue, though. It's been eluding me...

BenHenning · 2021-03-07T02:17:06Z

Aha, it appears the Gradle failures are real. Have some fixes to do.

… util accessibility package (oppia#2775) * Refactor util a11y package with prod/test modules. * Hook up new accessibility module. Includes introducing new top-level package groups to simpify visibility management. This also establishes a pattern for excluding migrated production files. * Lint fixes. * Address reviewer comments. * Linter fix. * Use direct file names. Addresses review comment.

… migrate util accessibility package (oppia#2775)" This reverts commit a89c609.

… util accessibility package (oppia#2775) * Refactor util a11y package with prod/test modules. * Hook up new accessibility module. Includes introducing new top-level package groups to simpify visibility management. This also establishes a pattern for excluding migrated production files. * Lint fixes. * Address reviewer comments. * Linter fix. * Use direct file names. Addresses review comment.

BenHenning added 3 commits February 23, 2021 13:07

Refactor util a11y package with prod/test modules.

ac22335

Hook up new accessibility module.

b7ea826

Includes introducing new top-level package groups to simpify visibility management. This also establishes a pattern for excluding migrated production files.

Lint fixes.

ac7e61c

BenHenning requested review from anandwana001 and rt4914 as code owners February 23, 2021 22:07

BenHenning requested review from fsharpasharp and FareesHussain February 23, 2021 22:08

BenHenning assigned rt4914, fsharpasharp, anandwana001 and FareesHussain Feb 23, 2021

BenHenning mentioned this pull request Feb 23, 2021

Fix #2643: Added BUILD.bazel in accessibility package #2668

Closed

8 tasks

BenHenning mentioned this pull request Feb 23, 2021

Create a BUILD file for accessibility/ #2643

Closed

rt4914 approved these changes Feb 24, 2021

View reviewed changes

rt4914 removed their assignment Feb 24, 2021

FareesHussain suggested changes Feb 24, 2021

View reviewed changes

FareesHussain assigned BenHenning and unassigned FareesHussain Feb 24, 2021

BenHenning added 3 commits February 24, 2021 18:31

Merge branch 'develop' into introduce-canonical-bazel-library-structure

9e62837

Address reviewer comments.

8a7309c

Linter fix.

b6600ab

BenHenning assigned FareesHussain and unassigned BenHenning Feb 25, 2021

FareesHussain approved these changes Feb 25, 2021

View reviewed changes

FareesHussain removed their assignment Feb 25, 2021

anandwana001 approved these changes Feb 25, 2021

View reviewed changes

anandwana001 removed their assignment Feb 25, 2021

fsharpasharp reviewed Feb 26, 2021

View reviewed changes

fsharpasharp approved these changes Feb 26, 2021

View reviewed changes

FareesHussain reviewed Feb 27, 2021

View reviewed changes

FareesHussain assigned BenHenning and unassigned fsharpasharp Feb 27, 2021

BenHenning mentioned this pull request Mar 2, 2021

Fix #2657: Added BUILD.bazel in util/system #2795

Merged

8 tasks

BenHenning added 2 commits March 4, 2021 19:49

Merge branch 'develop' into introduce-canonical-bazel-library-structure

255f932

Use direct file names.

f64098c

Addresses review comment.

BenHenning assigned FareesHussain and unassigned BenHenning Mar 5, 2021

FareesHussain approved these changes Mar 5, 2021

View reviewed changes

FareesHussain assigned BenHenning and unassigned FareesHussain Mar 5, 2021

BenHenning enabled auto-merge (squash) March 6, 2021 04:52

BenHenning mentioned this pull request Mar 7, 2021

Verify Bazel CI test changes are working as expected #2691

Closed

8 tasks

BenHenning added 2 commits March 6, 2021 18:40

Fix new test module list.

b326d42

Merge branch 'develop' into introduce-canonical-bazel-library-structure

969f46a

BenHenning merged commit 76dc13d into develop Mar 7, 2021

BenHenning deleted the introduce-canonical-bazel-library-structure branch March 7, 2021 03:09

techjd added a commit to techjd/oppia-android that referenced this pull request Mar 9, 2021

Revert "Fix oppia#2643: Introduce canonical Bazel library structure &…

b6233da

… migrate util accessibility package (oppia#2775)" This reverts commit a89c609.

techjd added a commit to techjd/oppia-android that referenced this pull request Mar 9, 2021

Revert "Fix oppia#2643: Introduce canonical Bazel library structure &…

bc0f4da

… migrate util accessibility package (oppia#2775)" This reverts commit a89c609.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2643: Introduce canonical Bazel library structure & migrate util accessibility package #2775

Fix #2643: Introduce canonical Bazel library structure & migrate util accessibility package #2775

BenHenning commented Feb 23, 2021 •

edited

Loading

BenHenning commented Feb 23, 2021

BenHenning commented Feb 23, 2021

rt4914 left a comment

FareesHussain left a comment

FareesHussain Feb 24, 2021

BenHenning Feb 25, 2021

BenHenning Feb 25, 2021

FareesHussain Feb 24, 2021

BenHenning Feb 25, 2021

FareesHussain Feb 25, 2021

FareesHussain Feb 24, 2021

BenHenning Feb 25, 2021

FareesHussain Feb 24, 2021

BenHenning Feb 25, 2021

BenHenning Feb 25, 2021

FareesHussain left a comment

anandwana001 left a comment

fsharpasharp Feb 26, 2021

BenHenning Mar 2, 2021

FareesHussain Feb 27, 2021

BenHenning Mar 2, 2021

BenHenning Mar 5, 2021

FareesHussain Feb 27, 2021

BenHenning Mar 2, 2021

FareesHussain Mar 2, 2021

BenHenning Mar 5, 2021

BenHenning commented Mar 5, 2021

FareesHussain left a comment

BenHenning commented Mar 6, 2021

BenHenning commented Mar 6, 2021 •

edited

Loading

BenHenning commented Mar 6, 2021

BenHenning commented Mar 7, 2021

	MIGRATED_PROD_FILES = glob(["src/main/java/org/oppia/android/util/android/*/.kt"])
	MIGRATED_PROD_FILES = glob(["src/main/java/org/oppia/android/util/accessibility/*/.kt"])

		@@ -1,4 +1,50 @@
		# TODO(#1532): Rename file to 'BUILD' post-Gradle.

Fix #2643: Introduce canonical Bazel library structure & migrate util accessibility package #2775

Fix #2643: Introduce canonical Bazel library structure & migrate util accessibility package #2775

Conversation

BenHenning commented Feb 23, 2021 • edited Loading

BenHenning commented Feb 23, 2021

BenHenning commented Feb 23, 2021

rt4914 left a comment

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

anandwana001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning commented Mar 5, 2021

FareesHussain left a comment

Choose a reason for hiding this comment

BenHenning commented Mar 6, 2021

BenHenning commented Mar 6, 2021 • edited Loading

BenHenning commented Mar 6, 2021

BenHenning commented Mar 7, 2021

BenHenning commented Feb 23, 2021 •

edited

Loading

BenHenning commented Mar 6, 2021 •

edited

Loading