From ffae4b6ea76526d724dd70904438550873aa191a Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Mon, 24 Oct 2022 15:27:56 -0700 Subject: Add fixes for bpfmt issues Bug: 132785638 Test: Manually Change-Id: I4d95742af41074ec905ff859b8b6bd11caea8be0 --- rh/hooks.py | 13 ++++++++++--- rh/hooks_unittest.py | 2 ++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/rh/hooks.py b/rh/hooks.py index 7edda83..0e9ee40 100644 --- a/rh/hooks.py +++ b/rh/hooks.py @@ -352,15 +352,22 @@ def check_bpfmt(project, commit, _desc, diff, options=None): return None bpfmt = options.tool_path('bpfmt') - cmd = [bpfmt, '-l'] + options.args((), filtered) + bpfmt_options = options.args((), filtered) + cmd = [bpfmt, '-l'] + bpfmt_options ret = [] for d in filtered: data = rh.git.get_file_content(commit, d.file) result = _run(cmd, input=data) if result.stdout: + fixup_cmd = [bpfmt, '-w'] + if '-s' in bpfmt_options: + fixup_cmd.append('-s') + fixup_cmd.append(os.path.join(project.dir, d.file)) ret.append(rh.results.HookResult( - 'bpfmt', project, commit, error=result.stdout, - files=(d.file,))) + 'bpfmt', project, commit, + error=result.stdout, + files=(d.file,), + fixup_func=_fixup_func_caller(fixup_cmd))) return ret diff --git a/rh/hooks_unittest.py b/rh/hooks_unittest.py index ba5c5fa..c366e1f 100755 --- a/rh/hooks_unittest.py +++ b/rh/hooks_unittest.py @@ -370,6 +370,8 @@ class BuiltinHooksTests(unittest.TestCase): ret = rh.hooks.check_bpfmt( self.project, 'commit', 'desc', diff, options=self.options) self.assertIsNotNone(ret) + for result in ret: + self.assertIsNotNone(result.fixup_func) def test_checkpatch(self, mock_check, _mock_run): """Verify the checkpatch builtin hook.""" -- cgit v1.2.3 From 8cca2ee04a87f299b44c40e0cb8bbab40987c188 Mon Sep 17 00:00:00 2001 From: Sorin Basca Date: Thu, 20 Oct 2022 14:55:40 +0100 Subject: Move Python script from distutils to shutil Bug: 254537505 Test: google-java-format-diff.py -h Test: google-java-format-diff.py # force to reach modified line Change-Id: Ia232e4f5bbb9deab7d5eeb2a9ad2ad1689b80f60 --- tools/google-java-format.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/google-java-format.py b/tools/google-java-format.py index f951981..1553648 100755 --- a/tools/google-java-format.py +++ b/tools/google-java-format.py @@ -18,7 +18,7 @@ import argparse import os import sys -from distutils.spawn import find_executable +from shutil import which _path = os.path.realpath(__file__ + '/../..') if sys.path[0] != _path: @@ -64,7 +64,7 @@ def main(argv): # the parent dir up front and inject it into $PATH when launching it. # TODO: Pass the path in directly once this issue is resolved: # https://github.com/google/google-java-format/issues/108 - format_path = find_executable(opts.google_java_format) + format_path = which(opts.google_java_format) if not format_path: print( f'Unable to find google-java-format at: {opts.google_java_format}', -- cgit v1.2.3 From 787e6a349a829e46460352e93696ec72f28289f3 Mon Sep 17 00:00:00 2001 From: Sorin Basca Date: Thu, 20 Oct 2022 17:48:01 +0100 Subject: Using -b when calling google-java-format-diff Bug: 254537505 Test: upload from a project with the hook Change-Id: Ib26198fe65c5b8498af2ccf84953a591f5e7d571 --- tools/google-java-format.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/tools/google-java-format.py b/tools/google-java-format.py index 1553648..8a4f739 100755 --- a/tools/google-java-format.py +++ b/tools/google-java-format.py @@ -60,10 +60,6 @@ def main(argv): parser = get_parser() opts = parser.parse_args(argv) - # google-java-format-diff.py looks for google-java-format in $PATH, so find - # the parent dir up front and inject it into $PATH when launching it. - # TODO: Pass the path in directly once this issue is resolved: - # https://github.com/google/google-java-format/issues/108 format_path = which(opts.google_java_format) if not format_path: print( @@ -72,24 +68,19 @@ def main(argv): ) return 1 - extra_env = { - 'PATH': os.path.dirname(format_path) + os.pathsep + os.environ['PATH'], - } - # TODO: Delegate to the tool once this issue is resolved: # https://github.com/google/google-java-format/issues/107 diff_cmd = ['git', 'diff', '--no-ext-diff', '-U0', f'{opts.commit}^!'] diff_cmd.extend(['--'] + opts.files) diff = rh.utils.run(diff_cmd, capture_output=True).stdout - cmd = [opts.google_java_format_diff, '-p1', '--aosp'] + cmd = [opts.google_java_format_diff, '-p1', '--aosp', '-b', format_path] if opts.fix: cmd.extend(['-i']) if not opts.sort_imports: cmd.extend(['--skip-sorting-imports']) - stdout = rh.utils.run(cmd, input=diff, capture_output=True, - extra_env=extra_env).stdout + stdout = rh.utils.run(cmd, input=diff, capture_output=True).stdout if stdout: print('One or more files in your commit have Java formatting errors.') print(f'You can run: {sys.argv[0]} --fix {rh.shell.cmd_to_str(argv)}') -- cgit v1.2.3 From 3f7deee5b180403bc01dc66d57c64eb570643213 Mon Sep 17 00:00:00 2001 From: Zach Lee Date: Wed, 19 Oct 2022 19:49:07 +0000 Subject: Propagate google-java-format-diff.py's failures This adds error handling to expose failures in the script's dependencies. It also adds a `--verbose` flag to aid in debugging. Repo upload hooks will now properly understand that the fixing script has failed instead of passing silently. Bug: 254300364 Test: Added invalid flags for google-java-format in google-java-format-diff.py and verified that `repo upload` failed with expected output Change-Id: I88003dec942b220f112ba80875a6c027e6c2f5c7 --- tools/google-java-format.py | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/tools/google-java-format.py b/tools/google-java-format.py index 8a4f739..e25d781 100755 --- a/tools/google-java-format.py +++ b/tools/google-java-format.py @@ -28,8 +28,8 @@ del _path # We have to import our local modules after the sys.path tweak. We can't use # relative imports because this is an executable program, not a module. # pylint: disable=wrong-import-position -import rh.shell -import rh.utils +import rh.shell # pylint: disable=import-error +import rh.utils # pylint: disable=import-error def get_parser(): @@ -52,6 +52,8 @@ def get_parser(): parser.add_argument('files', nargs='*', help='If specified, only consider differences in ' 'these files.') + parser.add_argument('--verbose', action='store_true', + help='Explain what is being done.') return parser @@ -74,18 +76,45 @@ def main(argv): diff_cmd.extend(['--'] + opts.files) diff = rh.utils.run(diff_cmd, capture_output=True).stdout - cmd = [opts.google_java_format_diff, '-p1', '--aosp', '-b', format_path] + format_cmd = [ + opts.google_java_format_diff, + '-p1', + '--aosp', + '-b', + format_path, + ] if opts.fix: - cmd.extend(['-i']) + format_cmd.extend(['-i']) if not opts.sort_imports: - cmd.extend(['--skip-sorting-imports']) + format_cmd.extend(['--skip-sorting-imports']) - stdout = rh.utils.run(cmd, input=diff, capture_output=True).stdout - if stdout: + format_cmd_result = rh.utils.run( + format_cmd, input=diff, capture_output=True) + + if format_cmd_result.returncode != 0: + print("Failed due to non-zero exit code.") + if opts.verbose: + # print out the full command that was called, including pipes + print("Called:") + print(f" {' '.join(diff_cmd)} |") + print(f" {' '.join(format_cmd)}") + for line in format_cmd_result.stdout.splitlines(): + print(f"[captured stdout] {line}") + for line in format_cmd_result.stderr.splitlines(): + print(f"[captured stderr] {line}") + return format_cmd_result.returncode + if format_cmd_result.stdout: print('One or more files in your commit have Java formatting errors.') print(f'You can run: {sys.argv[0]} --fix {rh.shell.cmd_to_str(argv)}') return 1 - + if format_cmd_result.stderr: + # We need to use stderr to catch errors in google-java-format since we + # cannot listen for a non-zero error code until + # https://github.com/google/google-java-format/pull/848 is merged. + print("Errors have been captured in stderr.") + for line in format_cmd_result.stderr.splitlines(): + print(f"[captured stderr] {line}") + return 1 return 0 -- cgit v1.2.3