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 #2768: Removed voiceover_java_proto_lite from the list of exports in the model library #2999

Merged
merged 2 commits into from
Mar 28, 2021

Conversation

Karanjot-singh
Copy link
Contributor

@Karanjot-singh Karanjot-singh commented Mar 24, 2021

Explanation

Fixes #2768: Removed voiceover_java_proto_lite from the list of exports in the model library

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.

@Karanjot-singh Karanjot-singh marked this pull request as draft March 24, 2021 08:57
@Karanjot-singh
Copy link
Contributor Author

Karanjot-singh commented Mar 24, 2021

PTAL @prayutsu @fsharpasharp
I have added a few log screenshots. Kindly help addressing the queries

@fsharpasharp
Copy link
Contributor

PTAL @prayutsu @fsharpasharp
I have added a few log screenshots. Kindly help addressing the queries

Could you try removing the visibility field for voiceover_java_proto_lite? In general, for the BUILD linting you can try running

https://github.com/oppia/oppia-android/blob/develop/scripts/buildifier_lint_check.sh

locally. It might require some changes to output the specific issues. Let me check that.

@prayutsu
Copy link
Contributor

Actually, the problem with Bazel linter is it does not point out to the lines where the linter is failing, @anandwana001 Can you help us here?

@fsharpasharp
Copy link
Contributor

Actually, the problem with Bazel linter is it does not point out to the lines where the linter is failing, @anandwana001 Can you help us here?

We don't need to know the lines where it's failing, if we run the buildifier on the file to format the file for us. Run

https://github.com/oppia/oppia-android/blob/develop/scripts/buildifier_download.sh

Then use

./buildifier <path-to-BUILD.bazel-file>

@anandwana001
Copy link
Contributor

The command we use to lint check is

$buildifier_file_path --lint=warn --mode=check --warnings=-native-android,+out-of-order-load,+unsorted-dict-items <path-to-BUILD.bazel-file>

model/BUILD.bazel Outdated Show resolved Hide resolved
@anandwana001
Copy link
Contributor

Actually, the problem with Bazel linter is it does not point out to the lines where the linter is failing, @anandwana001 Can you help us here?

We don't need to know the lines where it's failing, if we run the buildifier on the file to format the file for us. Run

https://github.com/oppia/oppia-android/blob/develop/scripts/buildifier_download.sh

Then use

./buildifier <path-to-BUILD.bazel-file>

@fsharpasharp I am not sure if we can use the downloaded binary on local machine. As for me, I get some issues on mac. Let me know if it is working for you?
Steps:

  1. bash scripts/buildifier_download.sh
  2. bash scripts/buildifier_lint_check.sh

model/BUILD.bazel Outdated Show resolved Hide resolved
@fsharpasharp
Copy link
Contributor

fsharpasharp commented Mar 24, 2021

Actually, the problem with Bazel linter is it does not point out to the lines where the linter is failing, @anandwana001 Can you help us here?

We don't need to know the lines where it's failing, if we run the buildifier on the file to format the file for us. Run
https://github.com/oppia/oppia-android/blob/develop/scripts/buildifier_download.sh
Then use
./buildifier <path-to-BUILD.bazel-file>

@fsharpasharp I am not sure if we can use the downloaded binary on local machine. As for me, I get some issues on mac. Let me know if it is working for you?
Steps:

  1. bash scripts/buildifier_download.sh
  2. bash scripts/buildifier_lint_check.sh

I'm on Linux and it works for me. Just a sanity check, have you done

  • chmod +x scripts/buildifier_download.sh
  • chmod +x scripts/buildifier_lint_check.sh

What issues are you getting?

Edit:

buildifier_download.sh puts the buildifier binary in the current directory, but buildifier_lint_check.sh looks for the binary in

    buildifier_file_path="../oppia-android-tools/buildifier"

@anandwana001
Copy link
Contributor

Error I am getting:

********************************
Checking Bazel file formatting
********************************
scripts/buildifier_lint_check.sh: line 17: ../oppia-android-tools/buildifier: cannot execute binary file
********************************
Buildifier issue found.
Please fix the above issues.
********************************

If you see in .github/workflows/static_checks.yml

cd $HOME/oppia-android-tools
bash /home/runner/work/oppia-android/oppia-android/scripts/buildifier_download.sh

So, it will download the binary in ../oppia-android-tools/buildifier.

@fsharpasharp
Copy link
Contributor

Error I am getting:

********************************
Checking Bazel file formatting
********************************
scripts/buildifier_lint_check.sh: line 17: ../oppia-android-tools/buildifier: cannot execute binary file
********************************
Buildifier issue found.
Please fix the above issues.
********************************

If you see in .github/workflows/static_checks.yml

cd $HOME/oppia-android-tools
bash /home/runner/work/oppia-android/oppia-android/scripts/buildifier_download.sh

So, it will download the binary in ../oppia-android-tools/buildifier.

I see, I did update the buildifier_file_path in my local buildifier_lint_check.sh. We could update the buildifier_lint_check.sh to check for the buildifier binary in the repository root?

@anandwana001
Copy link
Contributor

It looks like it's working when the binary is in the root.

buildifier_file_path="../oppia-android-tools/buildifier" // not working
buildifier_file_path="buildifier" // working

@anandwana001
Copy link
Contributor

Filed an issue with an alternative solution. #3000

@fsharpasharp
Copy link
Contributor

Filed an issue with an alternative solution. #3000

That looks like a robust solution. Thanks for looking into it.

Copy link
Contributor

@fsharpasharp fsharpasharp 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 anandwana001 requested review from fsharpasharp and removed request for anandwana001 March 24, 2021 12:27
@Karanjot-singh
Copy link
Contributor Author

PTAL @BenHenning @vinitamurthi

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 @Karanjot-singh! LGTM.

@BenHenning
Copy link
Member

@anandwana001 passing this back to you since it appears you've requested changes.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM, the bazel linting.

@anandwana001 anandwana001 removed their assignment Mar 25, 2021
@Karanjot-singh
Copy link
Contributor Author

PTAL @vinitamurthi

@rt4914
Copy link
Contributor

rt4914 commented Mar 28, 2021

Merging this as the change is trivial and other reviewers has approved it.

@rt4914 rt4914 merged commit 8e46435 into oppia:develop Mar 28, 2021
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.

Remove :voiceover_java_proto_lite from the list of exports in the model library
7 participants