Fixes #2881: Creating BUILD.bazel for statusbar utility #2959

Arjupta · 2021-03-21T20:57:11Z

Explanation

Fixes #2881 by creating a BUILD.bazel file

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.

Fixes oppia#2881 by creating a BUILD.bazel file

anandwana001 · 2021-03-22T08:22:53Z

@Arjupta Please complete all the installation steps - https://github.com/oppia/oppia-android/wiki#installation

Arjupta · 2021-03-22T10:16:24Z

@Arjupta Please complete all the installation steps - https://github.com/oppia/oppia-android/wiki#installation

I have completed the setup again, but it isn't formatting the Bazel files as needed. Any hints ?

anandwana001 · 2021-03-23T04:22:48Z

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

+    visibility = ["//:oppia_api_visibility"],
+    deps = [
+        "//third_party:androidx_appcompat_appcompat",
+    ]


Suggested change

]

],

As for now, this will fix it. I will create a bazel lint guide in our wiki this week.

Also, by the time you can setup this, if you are not on windows - https://formulae.brew.sh/formula-linux/buildifier

I also used buildifier earlier, but it was just mentioning that there is a some lint failure in both the BUILD.bazel files I have changed. It was not actually pointing out the cause of failure.

buildifier has a different type of output while sending error, like #reformat. Will try to cover all possible cases in wiki page.
by the time, if you want to check what command we run in checks, you can look at this script - https://github.com/oppia/oppia-android/blob/develop/scripts/buildifier_lint_check.sh

@Arjupta you can run buildifier on the file itself to let buildifier format the file.

anandwana001

bazel lint successfully pass

fsharpasharp · 2021-03-31T12:36:14Z

utility/BUILD.bazel

@@ -51,6 +52,7 @@ kt_android_library(
        "//third_party:com_google_guava_guava",
        "//third_party:org_jetbrains_kotlinx_kotlinx-coroutines-core",
        "//utility/src/main/java/org/oppia/android/util/datetime:date_time_util",
+        "//utility/src/main/java/org/oppia/android/util/statusbar:status_bar_color",


Could you try moving this dependency directly to the app module? I don't see any intradependency on this library in the utility module.

fsharpasharp · 2021-04-06T18:58:05Z

Hi @Arjupta, are you still working on this? Let me know if you need any clarification on my part.

Arjupta · 2021-04-07T00:07:25Z

Hi @Arjupta, are you still working on this? Let me know if you need any clarification on my part.

Yes, please explain a bit, why a dependency from utility module must be taken up to the app module.

fsharpasharp · 2021-04-07T10:18:56Z

Ultimately, we will be removing the utility library. So whichever package needs the statusbar utility will directly add that to its list of dependencies. Eventually we will also be modularizing the app module, but by adding this dependency to the app library, we remove one layer of indirection.

Right now it looks like this

statusbar <- utility <- app

we can remove one layer and let app depend on statusbar

statusbar <- app

Arjupta · 2021-04-16T15:20:02Z

Ultimately, we will be removing the utility library. So whichever package needs the statusbar utility will directly add that to its list of dependencies. Eventually we will also be modularizing the app module, but by adding this dependency to the app library, we remove one layer of indirection.

Right now it looks like this

statusbar <- utility <- app

we can remove one layer and let app depend on statusbar

statusbar <- app

Pardon for the delay, @fsharpasharp I have been busy with GSOC proposal all this time.

I understand what you are trying to do finally. But where do we include statusBarUtility in the app module. It is used by several activities like onboarding, profile, walkthrough. Also in this branch all these files don't have their own BUILD.bazel . What do you suggest to do?

fsharpasharp · 2021-04-16T15:28:46Z

Ultimately, we will be removing the utility library. So whichever package needs the statusbar utility will directly add that to its list of dependencies. Eventually we will also be modularizing the app module, but by adding this dependency to the app library, we remove one layer of indirection.
Right now it looks like this
statusbar <- utility <- app
we can remove one layer and let app depend on statusbar
statusbar <- app

Pardon for the delay, @fsharpasharp I have been busy with GSOC proposal all this time.

I understand what you are trying to do finally. But where do we include statusBarUtility in the app module. It is used by several activities like onboarding, profile, walkthrough. Also in this branch all these files don't have their own BUILD.bazel . What do you suggest to do?

Yes, for now we'll just add the dependency to the app library of app/BUILD.bazel, but eventually we will also create BUILD.bazel files for each package.

Arjupta · 2021-04-16T19:00:02Z

Ultimately, we will be removing the utility library. So whichever package needs the statusbar utility will directly add that to its list of dependencies. Eventually we will also be modularizing the app module, but by adding this dependency to the app library, we remove one layer of indirection.
Right now it looks like this
statusbar <- utility <- app
we can remove one layer and let app depend on statusbar
statusbar <- app

Pardon for the delay, @fsharpasharp I have been busy with GSOC proposal all this time.
I understand what you are trying to do finally. But where do we include statusBarUtility in the app module. It is used by several activities like onboarding, profile, walkthrough. Also in this branch all these files don't have their own BUILD.bazel . What do you suggest to do?

Yes, for now we'll just add the dependency to the app library of app/BUILD.bazel, but eventually we will also create BUILD.bazel files for each package.

Will we need to make a new library for this? I wasn't able to find a place where this status bar utility can be added as a dependecy. This is because status bar utility is only used by Presenters in our codebase and we don't have any separate library for presenters i guess.

fsharpasharp · 2021-04-16T19:30:20Z

Ultimately, we will be removing the utility library. So whichever package needs the statusbar utility will directly add that to its list of dependencies. Eventually we will also be modularizing the app module, but by adding this dependency to the app library, we remove one layer of indirection.
Right now it looks like this
statusbar <- utility <- app
we can remove one layer and let app depend on statusbar
statusbar <- app

Pardon for the delay, @fsharpasharp I have been busy with GSOC proposal all this time.
I understand what you are trying to do finally. But where do we include statusBarUtility in the app module. It is used by several activities like onboarding, profile, walkthrough. Also in this branch all these files don't have their own BUILD.bazel . What do you suggest to do?

Yes, for now we'll just add the dependency to the app library of app/BUILD.bazel, but eventually we will also create BUILD.bazel files for each package.

Will we need to make a new library for this? I wasn't able to find a place where this status bar utility can be added as a dependecy. This is because status bar utility is only used by Presenters in our codebase and we don't have any separate library for presenters i guess.

Does it work when you add the dependency to this library?

kt_android_library(
    name = "app",
    srcs = APP_FILES,
    custom_package = "org.oppia.android.app.ui",
    enable_data_binding = 1,
    manifest = "src/main/AppAndroidManifest.xml",
    visibility = ["//visibility:public"],
    deps = [
        ":binding_adapters",
        ":dagger",
        ":databinding_resources",
        ":resources",
        ":view_models",
        ":views",
        "//data/src/main/java/org/oppia/android/data/backends/gae:prod_module",
        "//model",
        "//third_party:androidx_databinding_databinding-adapters",
        "//third_party:androidx_databinding_databinding-common",
        "//third_party:androidx_databinding_databinding-runtime",
        "//third_party:androidx_lifecycle_lifecycle-extensions",
        "//third_party:androidx_lifecycle_lifecycle-livedata-core",
        "//third_party:androidx_lifecycle_lifecycle-livedata-ktx",
        "//third_party:androidx_multidex_multidex",
        "//third_party:androidx_viewpager2_viewpager2",
        "//third_party:androidx_viewpager_viewpager",
        "//third_party:androidx_work_work-runtime-ktx",
        "//third_party:com_caverock_androidsvg-aar",
        "//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",
        "//utility/src/main/java/org/oppia/android/util/extensions:bundle_extensions",
    ],
)

Arjupta · 2021-04-20T05:24:39Z

Commited this change. @fsharpasharp PTAL

fsharpasharp · 2021-04-21T13:50:52Z

utility/BUILD.bazel

@@ -16,7 +16,6 @@ MIGRATED_PROD_FILES = glob([
    "src/main/java/org/oppia/android/util/profile/*.kt",
    "src/main/java/org/oppia/android/util/datetime/*.kt",
    "src/main/java/org/oppia/android/util/system/*.kt",
-    "src/main/java/org/oppia/android/util/statusbar/*.kt"


Keep this line.

Arjupta · 2021-04-24T06:51:33Z

@fsharpasharp PTAL

rt4914

In this I will have to differ to @fsharpasharp and @BenHenning

BenHenning

Thanks @Arjupta! Just had one comment.

Also, @rt4914 codeowners is interesting here. I think I need to move the Bazel file ownership to be at the end of the codeowners file so that local directory ownership is overwritten (at least until the team's comfort with Bazel is increased).

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

Arjupta · 2021-04-28T09:23:37Z

@BenHenning PTAL. Also the check which failed right now was not failing in the previous commit, the only change that was done in the latest commit was to change the desctiption of BUILD.bazel.

BenHenning

Thanks @Arjupta. I agree that the failure is unrelated, though it's surprising. This is the first time I've noticed a flake in ExplorationActivityTest. We'll need to keep an eye out for this.

Fixes oppia#2881: Creating BUILD.bazel for statusbar utility

e8b2ede

Fixes oppia#2881 by creating a BUILD.bazel file

Arjupta requested a review from rt4914 as a code owner March 21, 2021 20:57

Adding the path of statusbar in module level BUILD.bazel

04a06a7

Arjupta requested a review from BenHenning as a code owner March 21, 2021 23:15

anandwana001 suggested changes Mar 23, 2021

View reviewed changes

anandwana001 assigned Arjupta Mar 23, 2021

Arjupta added 2 commits March 23, 2021 17:29

Failing lint check

96142ad

Merge branch 'develop' into status-bar-bazel-build

4f43c62

anandwana001 approved these changes Mar 23, 2021

View reviewed changes

Arjupta assigned fsharpasharp Mar 29, 2021

fsharpasharp suggested changes Mar 31, 2021

View reviewed changes

Arjupta removed their assignment Apr 16, 2021

Arjupta requested a review from fsharpasharp April 16, 2021 15:20

Arjupta added 2 commits April 20, 2021 01:32

Taking the status bar utility directly to the app module

da21735

Resolving conflicts

9c57b4a

Arjupta requested review from fsharpasharp and removed request for fsharpasharp April 21, 2021 12:45

fsharpasharp reviewed Apr 21, 2021

View reviewed changes

Arjupta added 2 commits April 21, 2021 20:20

suggested changes

fe34450

Merge branch 'develop' into status-bar-bazel-build

89ca1ef

lint issue solved

9d1fa48

Arjupta requested a review from fsharpasharp April 24, 2021 06:51

fsharpasharp approved these changes Apr 24, 2021

View reviewed changes

fsharpasharp assigned BenHenning and rt4914 Apr 24, 2021

rt4914 approved these changes Apr 26, 2021

View reviewed changes

rt4914 removed their assignment Apr 26, 2021

BenHenning reviewed Apr 27, 2021

View reviewed changes

utility/src/main/java/org/oppia/android/util/statusbar/BUILD.bazel Outdated Show resolved Hide resolved

BenHenning assigned Arjupta and unassigned BenHenning Apr 27, 2021

BenHenning mentioned this pull request Apr 27, 2021

Move Bazel ownership to end of codeowners file #3120

Merged

nit change

b0b4cc3

Arjupta assigned BenHenning and unassigned Arjupta Apr 28, 2021

BenHenning approved these changes Apr 29, 2021

View reviewed changes

BenHenning merged commit aac8348 into oppia:develop Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2881: Creating BUILD.bazel for statusbar utility #2959

Fixes #2881: Creating BUILD.bazel for statusbar utility #2959

Arjupta commented Mar 21, 2021

anandwana001 commented Mar 22, 2021

Arjupta commented Mar 22, 2021

anandwana001 Mar 23, 2021

anandwana001 Mar 23, 2021

anandwana001 Mar 23, 2021

Arjupta Mar 23, 2021

anandwana001 Mar 23, 2021

fsharpasharp Mar 31, 2021

anandwana001 left a comment

fsharpasharp Mar 31, 2021

fsharpasharp commented Apr 6, 2021

Arjupta commented Apr 7, 2021

fsharpasharp commented Apr 7, 2021

Arjupta commented Apr 16, 2021 •

edited

fsharpasharp commented Apr 16, 2021

Arjupta commented Apr 16, 2021

fsharpasharp commented Apr 16, 2021

Arjupta commented Apr 20, 2021

fsharpasharp Apr 21, 2021

Arjupta Apr 21, 2021

Arjupta commented Apr 24, 2021

rt4914 left a comment

BenHenning left a comment

Arjupta commented Apr 28, 2021

BenHenning left a comment

Fixes #2881: Creating BUILD.bazel for statusbar utility #2959

Fixes #2881: Creating BUILD.bazel for statusbar utility #2959

Conversation

Arjupta commented Mar 21, 2021

Explanation

Checklist

anandwana001 commented Mar 22, 2021

Arjupta commented Mar 22, 2021

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

anandwana001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsharpasharp commented Apr 6, 2021

Arjupta commented Apr 7, 2021

fsharpasharp commented Apr 7, 2021

Arjupta commented Apr 16, 2021 • edited

fsharpasharp commented Apr 16, 2021

Arjupta commented Apr 16, 2021

fsharpasharp commented Apr 16, 2021

Arjupta commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arjupta commented Apr 24, 2021

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Arjupta commented Apr 28, 2021

BenHenning left a comment

Choose a reason for hiding this comment

Arjupta commented Apr 16, 2021 •

edited