Skip to content
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 #1736: Add Bazel Linter to the project #1745

Closed

Conversation

anandwana001
Copy link
Contributor

@anandwana001 anandwana001 commented Aug 29, 2020

Explanation

Fix #1736

This is the first part where we want to add linter for bazel.

How to test

  1. Run command brew install buildifier on your root - this will install the buildifier
  2. Go to oppia-android project root
  3. You can either run command for single files or all at once
 buildifier --lint=warn --mode=check --warnings=all oppia_android_test.bzl
 buildifier --lint=warn --mode=check --warnings=all BUILD.bazel
 buildifier --lint=warn --mode=check --warnings=all WORKSPACE
or 
 buildifier --lint=warn --mode=check --warnings=all oppia_android_test.bzl BUILD.bazel WORKSPACE
  1. If you want to see what auto-fix of this linter do
 buildifier --lint=fix --warnings=all oppia_android_test.bzl
 buildifier --lint=fix --warnings=all BUILD.bazel
 buildifier --lint=fix --warnings=all WORKSPACE
or 
 buildifier --lint=warn --mode=check --warnings=all oppia_android_test.bzl BUILD.bazel WORKSPACE

There are currently two types of issue which auto-fix cannot fix

  1. out-of-order-load - lexicographical order, mentioned in the comments below
  2. unsorted-dict-items -

How to review

Until now in this PR, I had fixed and added only root bazel files in checks. Before moving forward with the bazel files in modules, we need to clarify a few points which are a bit different way a linter fix the issues.

Now

./buildifier --lint=warn --mode=check --warnings=all BUILD.bazel WORKSPACE oppia_android_test.bzl

After First Review, I will add all bazel files to check

./buildifier --lint=warn --mode=check --warnings=all -r app data domain model testing utility BUILD.bazel WORKSPACE oppia_android_test.bzl

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.

@anandwana001 anandwana001 marked this pull request as draft August 29, 2020 23:05
WORKSPACE Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
@anandwana001 anandwana001 changed the title Fix #1736: Add Bazel Linter to the project Fix part of #1736: Add Bazel Linter to the project Aug 30, 2020
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anandwana001! This looks like a great start. Left some comments.

WORKSPACE Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
oppia_android_test.bzl Show resolved Hide resolved
oppia_android_test.bzl Show resolved Hide resolved
oppia_android_test.bzl Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@anandwana001 anandwana001 changed the title Fix part of #1736: Add Bazel Linter to the project Fix #1736: Add Bazel Linter to the project Aug 31, 2020
@anandwana001
Copy link
Contributor Author

As of now, I am trying to remove the package(default_visibility) and getting an error where the utility module has to access "//app:crashlytics", and "//app:crashlytics_deps". As "//app:crashlytics_deps", is an android_library, I can set visibility to public but cannot provide visibility to "//app:crashlytics",. This can be resolved by package(default_visibility=["//utility:__subpackages__"]).
Another error comes is root level BUILD not able to access app:manifest which is solved by using export_files.

@anandwana001
Copy link
Contributor Author

As of now, I am trying to remove the package(default_visibility) and getting an error where the utility module has to access "//app:crashlytics", and "//app:crashlytics_deps". As "//app:crashlytics_deps", is an android_library, I can set visibility to public but cannot provide visibility to "//app:crashlytics",. This can be resolved by package(default_visibility=["//utility:__subpackages__"]).
Another error comes is root level BUILD not able to access app:manifest which is solved by using export_files.

Not solved, another error with other manifest files.

# BUILD
# *.bzl

exec ./buildifier --lint=warn --mode=check --warnings=all -r app data domain model testing utility tools BUILD.bazel WORKSPACE oppia_android_test.bzl
Copy link
Contributor

@vinitamurthi vinitamurthi Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expects the buildifier path to be the current path right? When we introduce pre-push hooks, that may not be the case so let's add an install path variable and prefix this call with that

Copy link
Contributor

@aggarwalpulkit596 aggarwalpulkit596 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anandwana001 anandwana001 marked this pull request as ready for review September 28, 2020 09:07
@anandwana001 anandwana001 marked this pull request as draft October 12, 2020 05:03
@anandwana001
Copy link
Contributor Author

Closing this PR and continuing the work in different PR and will follow the reviews there, as there is a bunch of conflicts hereafter changing the package naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bazel Linter to the project
4 participants