New issue
Advanced search Search tips

Issue 918755 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 12
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Make committing to toolchain-utils easier

Project Member Reported by g...@google.com, Jan 3

Issue description

Committing to toolchain-utils is a bit of a fragmented experience at the moment (we have some git hooks that're only run on direct pushes, and which depend on Google-internal code. We also don't have built-in pylint.) It would be nice if:

- We ran our current git hooks as a part of `repo upload`
- We use a Python formatter that's accessible from inside of the chroot (and, ideally, open source)
- We ran linting as a part of our hooks

This bug tracks that work.
 
RE formatting: Luis and I chatted offline, and came to the following conclusions:
- There exists pyformat githooks already
- It appears that Chromium and CrOS are moving toward yapf
- pyformat is basically a small wrapper around yapf at this point, modulo a few nice features (notably, quote fixing, and addition/removal of #! if it's warranted)

Pyformat has the annoyance of being Google-internal, and not immediately accessible from inside of the chroot. So, we think that moving to yapf is a good thing if we don't lose much functionality.

The #! checking is pretty simple to emulate, though consistent quoting is potentially more difficult (looks like pyformat uses 2to3lib; the sum total of the official docs for that is "The lib2to3 API should be considered unstable and may change drastically in the future.") So, the only feature we appear to actually miss out on is the tool fixing quotes for us.

Given that there's a feature request out for that at the moment (https://github.com/google/yapf/issues/399), and slightly inconsistent quotes aren't earth-shatteringly bad, I think we agree that missing out on that is OK for now.

tl;dr: we plan to move to yapf. CL to add a simple style: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/1392445

There are rumblings of a consistent-across-CrOS yapf config. If that happens, we'll move to that.
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/0e74b9bf195b55935e81f79ffcd1382a9a678715

commit 0e74b9bf195b55935e81f79ffcd1382a9a678715
Author: George Burgess IV <gbiv@google.com>
Date: Fri Jan 04 10:09:05 2019

toolchain-utils: add a top-level `yapf` file

yapf is a Python autoformatter, much like how clang-tidy is an
autoformatter for c/c++. It's available by default in the chroot, and
appears to be in use by our autotest friends.

I ran `yapf -i *.py`, and it only suggested a handful of changes from
our current style here. The only one I really noticed was that it
preferred

'foo %s' % (really_long_variable_name_one,
	    really_long_variable_name_two)

over

'foo %s' %
	(really_long_variable_name_one, really_long_variable_name_two)

...Which I think we do in some files, anyway.

This file magically applies to all `yapf` invocations inside of
toolchain-utils, so it's not something that people need to worry about.
Simply run `yapf -i file.py`, and it'll be automatically formatted to
conform to Chromium's style guidelines, which we try to follow here.

BUG= chromium:918755 
TEST=`yapf *.py` left files mostly untouched. `cros lint` is just as
     (un)happy after the format as it was before.

Change-Id: Ie09694d8466b398ca07f5c8544597c9a28c7abfe
Reviewed-on: https://chromium-review.googlesource.com/1392445
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Luis Lozano <llozano@chromium.org>

[add] https://crrev.com/0e74b9bf195b55935e81f79ffcd1382a9a678715/.style.yapf

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/c100127d900d69608ab715ddcfac1d783c7848ef

commit c100127d900d69608ab715ddcfac1d783c7848ef
Author: George Burgess IV <gbiv@google.com>
Date: Sat Jan 05 22:52:12 2019

toolchain-utils: swap git hooks to use yapf

This updates a few things:
- Migrates our git hooks to directly use yapf for formatting (which
  appears to be the direction that Chromium is heading in, and is
  available by default both inside and outside of the chroot: Chromium's
  depot_tools has a copy of it. pyformat is only available internally,
  and is basically a wrapper around yapf, except for a few things.
  Please see CL 1392445 for more),
- Splits out the format checks into its own script, so it's easier to
  potentially run from e.g. repo (don't quote me on that quite yet,
  though ;) )
- Removes the now-seemingly-unused tc_format script

BUG= chromium:918755 
TEST=- ./check-format on a poorly formatted Python file
     - ./check-format on a python file with a #! that shouldn't have had
       it
     - ./check-format on a python file without a #! that should have had
       it
     - ./check-format on toolchain-utils/*.py; results were overall
       reasonable
     All fixes it suggested worked as intended.
CQ-DEPEND=CL:1392445

Change-Id: Idabc2658c5dedcc70b6d3cc6c1acfd6c83eabc29
Reviewed-on: https://chromium-review.googlesource.com/1394285
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Luis Lozano <llozano@chromium.org>

[modify] https://crrev.com/c100127d900d69608ab715ddcfac1d783c7848ef/toolchain_utils_githooks/pre-push.real
[add] https://crrev.com/c100127d900d69608ab715ddcfac1d783c7848ef/toolchain_utils_githooks/check-format
[delete] https://crrev.com/0e74b9bf195b55935e81f79ffcd1382a9a678715/bin/tc_pyformat

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/e136e0a53ae27ca7ff6ad31e6b4f86fde7238410

commit e136e0a53ae27ca7ff6ad31e6b4f86fde7238410
Author: George Burgess IV <gbiv@google.com>
Date: Sun Jan 06 06:44:00 2019

toolchain-utils: add a `cros lint` git hook

This adds a git hook to run `cros lint` on all of the files we're
committing. It also adds a wrapper around both check-lint and
check-format that runs the two in parallel without making the output
interleave. Better names for check-style are appreciated.

BUG= chromium:918755 
TEST=Ran ./check-style on a few Python files. Got the expected
     output/lint errors/etc. Works both outside and inside of the
     chroot.
CQ-DEPEND=CL:1394285

Change-Id: I30c74662759f27abdfd663083d04002a05260bf7
Reviewed-on: https://chromium-review.googlesource.com/1395807
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>

[modify] https://crrev.com/e136e0a53ae27ca7ff6ad31e6b4f86fde7238410/toolchain_utils_githooks/pre-push.real
[add] https://crrev.com/e136e0a53ae27ca7ff6ad31e6b4f86fde7238410/toolchain_utils_githooks/check-style
[add] https://crrev.com/e136e0a53ae27ca7ff6ad31e6b4f86fde7238410/toolchain_utils_githooks/check-lint

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/34b4506112b52a32844bcababd96a894f1cae0c4

commit 34b4506112b52a32844bcababd96a894f1cae0c4
Author: George Burgess IV <gbiv@google.com>
Date: Sat Jan 12 22:04:58 2019

toolchain-utils: make repo do presubmit checks

This CL makes repo do our Python style checks on `repo upload`.

BUG= chromium:918755 
TEST=`repo upload .` with broken Python

Change-Id: I2744637a5c4c1b25f11f56354ef39e04fd2a100b
Reviewed-on: https://chromium-review.googlesource.com/1395808
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Caroline Tice <cmtice@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[add] https://crrev.com/34b4506112b52a32844bcababd96a894f1cae0c4/PRESUBMIT.cfg

Status: Fixed (was: Untriaged)
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 17 (5 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/56620aa44e75db5710896fedcf53b4ea51606612

commit 56620aa44e75db5710896fedcf53b4ea51606612
Author: George Burgess IV <gbiv@google.com>
Date: Thu Jan 17 20:18:22 2019

git-hooks: swap to `[[`; fix style nits

Per the comment in I101c4c8b142f0c8f78819779adcfd0b3b0e1d614, `[[` is
preferable to `test` where possible. Swap to that for consistency.

(`[[` isn't an actual command, so we have to stick to `test` in
status_to_tf)

BUG= chromium:918755 
TEST=check-style on a handful of Python files

Change-Id: Ic2256fadc570a5d3818918e49de45d641a21574a
Reviewed-on: https://chromium-review.googlesource.com/1415683
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/56620aa44e75db5710896fedcf53b4ea51606612/toolchain_utils_githooks/check-style
[modify] https://crrev.com/56620aa44e75db5710896fedcf53b4ea51606612/toolchain_utils_githooks/check-format

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 18 (4 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/ea973ec56fa436a801937111a6bd7218d1a1be1d

commit ea973ec56fa436a801937111a6bd7218d1a1be1d
Author: George Burgess IV <gbiv@google.com>
Date: Fri Jan 18 12:01:37 2019

git-hooks: support linting outside of the source tree

We appear to have workflows and committers that prefer to `push` from
outside of the CrOS source tree.

It's relatively straightforward to have fallbacks to support that, so I
don't see why not.

If you want to interact with this repository from outside of a CrOS
source tree, you now have two options:
- If you have a source tree somewhere on your machine, you can set
  CHROMEOS_ROOT_DIRECTORY to that path. If `check-lint` detects a usable
  `cros` in there, it'll function as usual.
- If you don't have a source tree on your machine, `check-lint` will
  fall back to pylint-only linting (with a warning). This isn't ideal
  (we miss shell linting, as well as a few other nits in Python,
  somehow), but should get us most of the way to happiness.

This also includes a refactor of `test` to `[[`

BUG= chromium:918755 
TEST=Ran check-lint on some python files in a checkout outside and
     inside of the source tree

Change-Id: I101c4c8b142f0c8f78819779adcfd0b3b0e1d614
Reviewed-on: https://chromium-review.googlesource.com/1412682
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Caroline Tice <cmtice@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/ea973ec56fa436a801937111a6bd7218d1a1be1d/toolchain_utils_githooks/check-lint

Sign in to add a comment