-
Notifications
You must be signed in to change notification settings - Fork 535
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 #2643: Introduce canonical Bazel library structure & migrate util accessibility package #2775
Changes from 3 commits
ac22335
b7ea826
ac7e61c
9e62837
8a7309c
b6600ab
255f932
f64098c
b326d42
969f46a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,50 @@ | ||
# TODO(#1532): Rename file to 'BUILD' post-Gradle. | ||
|
||
# Corresponds to being accessible to all Oppia targets. This should be used for production APIs & | ||
# modules that may be used both in production targets and in tests. | ||
package_group( | ||
name = "oppia_api_visibility", | ||
packages = [ | ||
"//...", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this similar to if not what packages does each cover, when do we need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
], | ||
) | ||
|
||
# Corresponds to being accessible to the Oppia binary. This should only be used by production-facing | ||
# modules that must be included in the top-level binary in order for the app to build. | ||
package_group( | ||
name = "oppia_binary_visibility", | ||
packages = [ | ||
"//", | ||
], | ||
) | ||
|
||
# Corresponds to being accessible to Oppia tests. This should be used by fakes & test-only modules | ||
# that can be included in tests for custom state arrangement or to satisfy upstream/downstream | ||
# dependency requirements for components whose production implementations cannot be used in test | ||
# environments. | ||
# TODO(#2773): Remove the open visibility access granted here & instead restrict this access to only | ||
# test targets. | ||
package_group( | ||
name = "oppia_testing_visibility", | ||
packages = [ | ||
"//app/...", | ||
"//data/...", | ||
"//domain/...", | ||
"//testing/...", | ||
"//utility/...", | ||
], | ||
) | ||
|
||
# Special visibility group specific to prod modules. This provides access to the module for both the | ||
# Oppia binary & tests. | ||
package_group( | ||
name = "oppia_prod_module_visibility", | ||
includes = [ | ||
":oppia_binary_visibility", | ||
":oppia_testing_visibility", | ||
], | ||
) | ||
|
||
# TODO(#1640): Move binary manifest to top-level package post-Gradle. | ||
android_binary( | ||
name = "oppia", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -533,6 +533,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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've missed this earlier. Why are we adding the deps in app/BUILD.bazel There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
], | ||
) | ||
|
||
|
@@ -632,6 +633,7 @@ kt_android_library( | |
"//third_party:com_google_android_flexbox", | ||
"//third_party:javax_annotation_javax_annotation-api_jar", | ||
"//utility", | ||
"//utility/src/main/java/org/oppia/android/util/accessibility:prod_module", | ||
], | ||
) | ||
|
||
|
@@ -697,6 +699,8 @@ TEST_DEPS = [ | |
"//third_party:org_robolectric_annotations", | ||
"//third_party:robolectric_android-all", | ||
"//utility", | ||
"//utility/src/main/java/org/oppia/android/util/accessibility", | ||
"//utility/src/main/java/org/oppia/android/util/accessibility:test_module", | ||
] | ||
|
||
# App module tests. | ||
|
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.
Will we be replacing all instances of //visibility:public with one of these package groups?
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.
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.