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

FareesHussain · 2021-02-09T10:39:16Z

Explanation

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

FareesHussain · 2021-02-09T10:49:37Z

@fsharpasharp I'm facing this error when I build the app

ERROR: /home/farees/dev/oppia/oppia-android/app/BUILD.bazel:620:19: KotlinCompile //app:app_kt { kt: 256, java: 0, srcjars: 0 } for armeabi-v7a failed (Exit 1)
error: /tmp/working9424917616707839429/_kotlinc/app-app_kt_jvm/generated_java_sources/org/oppia/android/app/application/DaggerApplicationComponent.java:695: error: cannot find symbol
import org.oppia.android.util.accessibility.AccessibilityModule_ProvideAccessibilityManagerFactory;

How can I solve this
I tried to add the all the targets in the app/BUILD.bazel (where //utility is used) but I get the same error

fsharpasharp · 2021-02-09T21:20:37Z

Right now I can't seem to build the project with Bazel, but I'll take a look as soon as I can build the project again.

FareesHussain · 2021-02-09T21:36:07Z

Right now I can't seem to build the project with Bazel, but I'll take a look as soon as I can built the project again.

Sure, thanks.
Adding @BenHenning too.

BenHenning

Thanks @FareesHussain! Left some thoughts.

BenHenning · 2021-02-10T02:31:07Z

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

+    visibility = ["//visibility:public"],
+)
+
+kt_android_library(


This should be marked as test only (e.g. with a testonly = True line) so that it can't be inadvertently included in production builds.

BenHenning · 2021-02-10T02:31:23Z

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

+        ":android_accessibility_manager",
+        ":custom_accessibility_manager",
+        artifact("com.google.dagger:dagger:2.27"),
+        artifact("javax.inject:javax.inject:1"),


Ditto here & below for the inject lines (see my comment in your other PR).

Will do this after #2648 (comment) is resolved

BenHenning · 2021-02-10T02:34:53Z

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

+)
+
+kt_android_library(
+    name = "custom_accessibility_manager",


This is the API, and the other two are the implementations for this package. I think we should instead set this up as:

kt_android_library( name = "accessibility", visibility = ["//visibility:public"], srcs = ["CustomAccessibilityManager.kt"], deps = [":impl"], ) kt_android_library( name = "impl", srcs = ["AndroidAccessibilityModule.kt","AccessibilityModule.kt"], deps = [...], ) kt_android_library( name = "testing", # Could also be called 'fake_accessibility_manager' visibility = ["//visibility:public"], srcs = ["FakeAccessibilityManager.kt"], deps = [...], )

This is the convention we should be using broadly since it clearly designates the public API as the package-level library, and properly privatizes the implementation & separates out the testing library.

For packages that have multiple public APIs (e.g. general utility packages with a bunch of separate things), in that case we would not have a package-level library and each group of utilities would be in their own packages.

@fsharpasharp FYI since we discussed this today.

Done, instead of accessibility I've used acc as it was getting refactored in app/BUILD.bazel. Will change it back after conformation.

BenHenning · 2021-02-10T02:35:33Z

utility/BUILD.bazel

+        "//utility/src/main/java/org/oppia/android/util/accessibility:accessibility_module",
+        "//utility/src/main/java/org/oppia/android/util/accessibility:android_accessibility_manager",
+        "//utility/src/main/java/org/oppia/android/util/accessibility:custom_accessibility_manager",
+        "//utility/src/main/java/org/oppia/android/util/accessibility:fake_accessibility_manager",


Here & below: this one should only be included in test deps, not production classes.

FareesHussain · 2021-02-10T16:17:55Z

@BenHenning PTAL, The error mentioned here #2668 (comment) is not resolved after adding the mentioned targets in app/BUILD.bazel.

BenHenning · 2021-02-11T02:26:52Z

@FareesHussain. First, the accessibility package should depend on impl (impl itself should be private). Naming it 'accessibility' shouldn't change the error.

Can you also please share the latest error after making the 2 changes above (e.g. updating impl to be private & changing the accessibility library name)? Please also push those changes so that I can cross-reference the error with your PR.

FareesHussain · 2021-02-14T22:29:37Z

Right now I have some problem with Bazel.
I Will update as soon as I fix that

… accessibility-build-bazel � Conflicts: � app/BUILD.bazel � utility/BUILD.bazel

FareesHussain · 2021-02-15T15:11:38Z

@BenHenning I made :impl private but there is some error related to this

BenHenning

Thanks @FareesHussain! Left some thoughts. Not de-assigning myself since I need to think further about how to lay out these files generally elsewhere in the codebase.

BenHenning · 2021-02-17T05:17:04Z

app/BUILD.bazel

@@ -683,6 +685,8 @@ TEST_DEPS = [
    "//third_party:org_robolectric_annotations",
    "//third_party:robolectric_android-all",
    "//utility",
+    "//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",


Suggested change

"//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",

"//utility/src/main/java/org/oppia/android/util/accessibility",

Should be sufficient.

BenHenning · 2021-02-17T05:18:13Z

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

@@ -0,0 +1,32 @@
+load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")


We should add a top-level documentation comment to this file explaining what sorts of files belong to it (e.g. General purposes utilities pertaining to the Android accessibility manager.).

BenHenning · 2021-02-17T05:18:46Z

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

+    ],
+    deps = [
+        ":accessibility",
+        artifact("com.google.dagger:dagger:2.27"),


As discussed in chat, prefer using the third party wrappers instead of artifact since they offer a bunch of benefits (see #2663 for specifics).

BenHenning · 2021-02-17T05:34:30Z

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

+)
+
+kt_android_library(
+    name = "impl",


Unfortunately, I think I missed the circular dependency that arises here when we try to make accessiblity depend on impl. There are a couple of different paths that we can take here, and I need to spend a bit more time thinking about this before proposing a specific solution. @fsharpasharp do you have a specific pattern that you think might work here?

How about introducing a third library that exports the interface and impl? None of the existing libraries will depend on it.

That might unfortunately undermine the value of keeping impl separate (since it then exposes the public classes in impl which should stay private as part of impl). I think we might need to use a similar pattern where we export the modules of the package & have those depend on impl, but I'm not super keen on this pattern since it has its own drawbacks (such as requiring dependencies on all modules in central places like the android_binary or test targets).

Okay, let's try the module + impl approach and see how that looks. @FareesHussain I suggest changing this to be:

kt_android_library( name = "accessibility", visibility = ["//visibility:public"], ... (source is interface only) ) kt_android_library( name = "impl", ... (private visibility, source is accessibility module + prod implementation) ) kt_android_library( name = "prod_module", visibility = ["//:oppia_binary_visibility"], ... (source is new prod module -- see below; depends on :impl) ) kt_android_library( name = "testing", visibility = ["//:oppia_testing_visibility"], testonly = True, ... (source is fake) ) kt_android_library( name = "test_module", visibility = ["//:oppia_testing_visibility"], testonly = True, ... (source is new test module -- see below; depends on :testing) )

You may also notice that there are both a test module & prod module. That's because I'm also suggesting that you make a functional change:

Rename AccessibilityModule to be AccessibilityManagerModule (to clarify it's just for providing the manager)

Introduce an AccessibilityProdModule that provides a binding of CustomAccessibilityManager to AndroidAccessibilityManager

Introduce an AccessibilityTestModule that provides a binding of CustomAccessibilityManager to FakeAccessibilityManager

Update all injections of AndroidAccessibilityModule to instead inject CustomAccessibilityManager so that it properly swaps out the accessibility version (you may be able to simplify some test bindings that are probably replacing the binding with the test version already)

Note also that the above requires introducing two new visibility groups: oppia_binary_visibility & oppia_testing_visibility to simplify visibility management. The former should work as-is, but the latter may not work until we split up the tests (in which case we should introduce the visibility such that it applies to all subpackages, and is later restricted to tests).

The nice thing about this package is that it actually includes all of the pieces we want to demonstrate in the pattern:

A public API

A production implementation that is kept private

A production module that should use the private implementation & be incorporated in the top-level binary (or in tests that want to use the production module)

A testing module that can optionally be used in tests

A fake testing library

If you're unsure of all this, let me know. I'll introduce a new PR that replaces this one in that case since it may be easier for me to demonstrate the new pattern more explicitly that way.

@fsharpasharp as well--what do you think of the above?

@BenHenning I'll follow up with your PR regarding this pattern.

BenHenning · 2021-02-17T05:34:47Z

utility/BUILD.bazel

@@ -45,6 +48,9 @@ TEST_DEPS = [
    "//third_party:org_jetbrains_kotlin_kotlin-test-junit",
    "//third_party:org_mockito_mockito-core",
    "//third_party:robolectric_android-all",
+    "//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",
+    "//utility/src/main/java/org/oppia/android/util/accessibility:impl",


impl should be private.

… accessibility-build-bazel

BenHenning · 2021-02-23T07:24:24Z

Thanks @FareesHussain. Will keep this assigned to me until I can duplicate it later in the week with the pattern described in #2668 (comment). Shouldn't take me long, so I might be able to get to it between meetings tomorrow or Wed.

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

This has been replaced #2775 so closing this PR now. Thanks for starting it @FareesHussain, it was a really helpful reference when creating #2775.

Added BUILD.bazel in accessibility package

21dad44

FareesHussain requested a review from fsharpasharp February 9, 2021 10:39

FareesHussain assigned fsharpasharp Feb 9, 2021

FareesHussain requested a review from BenHenning February 9, 2021 21:36

FareesHussain assigned BenHenning Feb 9, 2021

BenHenning requested changes Feb 10, 2021

View reviewed changes

BenHenning assigned FareesHussain and unassigned BenHenning Feb 10, 2021

FareesHussain added 2 commits February 10, 2021 21:10

Resolving requested changes

82083da

Adding targets in app/BUILD.bazel

c939ff5

FareesHussain assigned BenHenning and fsharpasharp and unassigned fsharpasharp and FareesHussain Feb 10, 2021

BenHenning assigned FareesHussain and unassigned BenHenning Feb 11, 2021

FareesHussain added 2 commits February 15, 2021 18:49

Merge branch 'develop' of https://github.com/oppia/oppia-android into…

98bb093

… accessibility-build-bazel � Conflicts: � app/BUILD.bazel � utility/BUILD.bazel

Resolving requested changes

8a8bead

FareesHussain assigned BenHenning and unassigned fsharpasharp Feb 15, 2021

FareesHussain removed their assignment Feb 15, 2021

BenHenning requested changes Feb 17, 2021

View reviewed changes

BenHenning assigned fsharpasharp Feb 17, 2021

BenHenning assigned FareesHussain Feb 17, 2021

FareesHussain added 2 commits February 17, 2021 12:23

Merge branch 'develop' of https://github.com/oppia/oppia-android into…

2575427

… accessibility-build-bazel

Resolving Requested changes

44aece9

BenHenning removed their assignment Feb 19, 2021

FareesHussain assigned BenHenning Feb 20, 2021

BenHenning mentioned this pull request Feb 23, 2021

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

Merged

BenHenning closed this Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

FareesHussain commented Feb 9, 2021

FareesHussain commented Feb 9, 2021

fsharpasharp commented Feb 9, 2021 •

edited

Loading

FareesHussain commented Feb 9, 2021

BenHenning left a comment

BenHenning Feb 10, 2021

FareesHussain Feb 10, 2021

BenHenning Feb 10, 2021

FareesHussain Feb 10, 2021

BenHenning Feb 10, 2021

FareesHussain Feb 10, 2021

BenHenning Feb 10, 2021

FareesHussain Feb 10, 2021

FareesHussain commented Feb 10, 2021

BenHenning commented Feb 11, 2021

FareesHussain commented Feb 14, 2021

FareesHussain commented Feb 15, 2021

BenHenning left a comment

BenHenning Feb 17, 2021

FareesHussain Feb 17, 2021

BenHenning Feb 17, 2021

FareesHussain Feb 17, 2021

BenHenning Feb 17, 2021

FareesHussain Feb 17, 2021

BenHenning Feb 17, 2021

fsharpasharp Feb 17, 2021 •

edited

Loading

BenHenning Feb 18, 2021

BenHenning Feb 19, 2021

BenHenning Feb 19, 2021

FareesHussain Feb 20, 2021

BenHenning Feb 17, 2021

FareesHussain Feb 17, 2021

BenHenning commented Feb 23, 2021

BenHenning commented Feb 23, 2021

	"//utility/src/main/java/org/oppia/android/util/accessibility:accessibility",
	"//utility/src/main/java/org/oppia/android/util/accessibility",

		@@ -0,0 +1,32 @@
		load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library")

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

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

Conversation

FareesHussain commented Feb 9, 2021

Explanation

Checklist

FareesHussain commented Feb 9, 2021

fsharpasharp commented Feb 9, 2021 • edited Loading

FareesHussain commented Feb 9, 2021

BenHenning 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

FareesHussain commented Feb 10, 2021

BenHenning commented Feb 11, 2021

FareesHussain commented Feb 14, 2021

FareesHussain commented Feb 15, 2021

BenHenning 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

fsharpasharp Feb 17, 2021 • edited Loading

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 Feb 23, 2021

BenHenning commented Feb 23, 2021

fsharpasharp commented Feb 9, 2021 •

edited

Loading

fsharpasharp Feb 17, 2021 •

edited

Loading