Merged
Conversation
c937d73 to
f8c4455
Compare
Contributor
|
can you post the result of the running this script on latest master here as well? |
Contributor
Author
|
@simon-mo Here's the output on latest master. Note that fixing these isn't just as trivial as making the proposed changes, since the semantics can be different. OutputSuggested patch (not correct in every situation) |
edoakes
reviewed
Jun 12, 2020
ci/travis/bazel.py
Outdated
Collaborator
There was a problem hiding this comment.
This file is pretty unintelligible to me. What is it doing?
Contributor
Author
There was a problem hiding this comment.
Sure. I'm currently using this script to run shellcheck on genrules. To do that it needs to call Bazel to output the genrule scripts after Bazel has preprocessed them. Bazel then outputs the scripts in its textproto format, which I parse here and then invoke shellcheck on. It'll help us catch issues like if someone writes rm -rf $$WORKSPACE_DIR/foo (forgetting to quote it)... which could wipe someone's files by accident.
The check-shell-scripts.sh script is responsible for checking all shell scripts, and it invokes bazel.py to handle the ones embedded in Bazel genrules.
But it's probably worth mentioning: I imagine this file will probably get more Bazel-related functionality later on (in fact there are already things that I'm hoping to migrate from Bash to Bazel using this once it's merged), since invoking Bazel and parsing its output are common subfunctionality for lots of scripts. It's worth keeping that in mind when reviewing. It'll become more useful as time goes on. (That's also why I kept the module name generic and also easy to import.)
I've refactored this file and migrated an existing Bash script (bazel-preclean.sh) to use its common functionality for better readability.
For reviewing, I think it's probably easier to understand if you follow the logic starting in main() (which would be reading bottom-up). Let me know if I should add any explanations along the way. I also highly recommend playing around with it (run ci/travis/check-shell-scripts.sh).
b6cc9f3 to
3f85ba9
Compare
barakmich
reviewed
Jul 28, 2020
| local name="shellcheck-v${shellcheck_version}" | ||
| if [ "${osname}" = linux ] || [ "${osname}" = darwin ]; then | ||
| sudo mkdir -p /usr/local/bin || true | ||
| curl -f -s -L "https://github.com/koalaman/shellcheck/releases/download/v${shellcheck_version}/${name}.${osname}.x86_64.tar.xz" | { |
Contributor
There was a problem hiding this comment.
if this is truly a Travis CI script, then travis already has shellcheck installable: koalaman/shellcheck#589
The less we can curl | sudo the better.
Contributor
Author
There was a problem hiding this comment.
We need 0.7.1 specifically. Travis has 0.7.0 which is too noisy to be useful. And people need to know which version to run locally too, otherwise they will get lint errors on CI that they don't get locally (which leads to a bad experience).
| fi | ||
|
|
||
| if shellcheck --shell=sh --format=diff - < /dev/null; then | ||
| if [ 0 -lt "${#bazel_files[@]}" ]; then |
Contributor
There was a problem hiding this comment.
Careful with string and integer comparisons in bash (and elsewhere)
Contributor
Author
There was a problem hiding this comment.
I am :-) this is correct!
ci/travis/bazel.py
Outdated
| return result | ||
|
|
||
|
|
||
| def shellcheck(bazel_aquery, *shellcheck_args): |
Contributor
There was a problem hiding this comment.
This file could really use some comments. What are you trying to do for shellcheck? What are some of the expected arguments?
Same with the other methods
Contributor
Author
There was a problem hiding this comment.
shellcheck_args is just arguments that get passed through to shellcheck. bazel_aquery is a Bazel aquery. I can try to mention these in comments.
| """Cleans up any genrule() outputs for the provided target(s). | ||
|
|
||
| This is useful for forcing genrule actions to re-run, because the _true_ | ||
| outputs of those actions can include a larger set of files (e.g. files |
Contributor
There was a problem hiding this comment.
If this is the case, the genrule is probably broken. Calling out where this happens in the BUILD.bazel would be helpful (so that it may at least be a caution, if not fixed, and preclean obviated)
Contributor
Author
There was a problem hiding this comment.
The genrules are indeed "broken", but that has always been the case, and is unrelated to this PR. The reason is that we want to put the generated files into the worktree and Bazel inherently doesn't support this. There's no way to avoid the precleaning short of revamping the entire build flow and avoiding modifying the worktree (which I'm not against if it's possible, but is orthogonal to this PR).
Contributor
Author
There was a problem hiding this comment.
This isn't new logic btw. It's equivalent to what was already in the other shell script I deleted. I'm just moving it from Bash to Python to deduplicate the textproto parsing logic since I had to solve that problem in here in Python too.
| # This flag formats individual files. --files *must* be the first command line | ||
| # arg to use this option. | ||
| if [[ "$1" == '--files' ]]; then | ||
| if [ "${1-}" == '--files' ]; then |
Contributor
There was a problem hiding this comment.
You probably want a getopt style loop here
Contributor
Author
There was a problem hiding this comment.
Separate PR :-) I'm not not trying to change the logic or refactor these here, just linting scripts.
|
|
||
| # Format all files, and print the diff to stdout for travis. | ||
| format_all() { | ||
| flake8 --inline-quotes '"' --no-avoid-escape --exclude=python/ray/core/generated/,streaming/python/generated,doc/source/conf.py,python/ray/cloudpickle/,python/ray/thirdparty_files/ --ignore=C408,E121,E123,E126,E226,E24,E704,W503,W504,W605 |
Contributor
There was a problem hiding this comment.
can we set up a flake8 config instead of copying these ignore flags in many different places?
Contributor
Author
There was a problem hiding this comment.
Possibly. Note that this isn't new logic though -- I just moved this line from the other file. I have no idea how to set up a flake8 config but it might be worth doing in a separate PR.
|
Test FAILed. |
|
Test FAILed. |
|
Test FAILed. |
edoakes
approved these changes
Jul 30, 2020
Collaborator
edoakes
left a comment
There was a problem hiding this comment.
LGTM. Can you check that bazel.py is checked by the python linter? At the very least manually running it would be acceptable.
Comment on lines
+204
to
+205
| i += 1 | ||
| result = 0 |
Collaborator
There was a problem hiding this comment.
Is this file checked by the linter? Should be a space here.
Contributor
Author
There was a problem hiding this comment.
It doesn't seem to complain locally for me for some reason (even though other deliberate changes do seem to make it complain), but I added a space now regardless.
6 tasks
|
Test FAILed. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Checks shell scripts for common errors.
Related issue number
#631
Checks
scripts/format.shto lint the changes in this PR.