New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 886993 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Make generate_buildbot_json.py autoformat .pyl files

Project Member Reported by martiniss@chromium.org, Sep 19

Issue description

The script currently uses the ast python package to parse the .pyl files and warn the user if they are unsorted. The ast package gives us line number information, so theoretically the script could actually rewrite the files to be correctly formatted.
 
I think you'd end up having to recreate a bunch of work if you wanted to use ast;.

You'd probably be much better off calling out to yapf, which is available via depot_tools.
yapf is useful, and we probably want to use it. It doesn't sort keys though, which is part of why the code I wrote is useful. Unless yapf has some hidden sorting capabilities I don't know of.
Good point, yapf won't do that.

I would expect ast to lose any comments that the file might have.

I'm not aware of any tool that will do what you want. I actually have a side/20% project that probably would do it, but I really doubt we'd want to depend on that given that I don't have the time to maintain it.
I would also love something like this for all of our newfound infra configs, so if your side/20% project can handle those, too, perhaps it'd be worth us (not necessarily you specifically) investing a little time into it?
I had thought it would work like this:

The objects ast gives us have a lineno attribute. I assume that refers to the first line that the object is defined on. So, the plan is to run the existing logic, which finds out which keys are in the wrong place. Then, you map where each missing key is to the lines which it corresponds to, and swap around lines. Example:

1  {
2    # Some comment here
3    'foo': {
4      ...
5      ...
6    },
7
8    # Some comment here
9    'aa': {
10     ...
11   },
12 }


When we do the ast parsing, we get back an object for 'foo' and 'aa'. They both have line numbers. The lines that key encompass are everything from 'foo' to the next key; 2-8 inclusive. Then you can grab those lines and move them around.

You would mess up the comments, so you might have to do a pass on the actual file and figure out where comments live.

This is pretty specific to how we're using the .pyl files; I don't think it'd work in the general case, but it might work for us.
Happy to use your 20% project though, if it works well for this.
Cc: vadimsh@chromium.org estaab@chromium.org no...@chromium.org
The 20% thing is a parser generator / code formatter thing that in theory could be made to work for this (it's explicitly intended to be able to handle this sort of thing), but  I haven't actually done this for this format as a proof of concept (I've thought about it, but haven't done it).

That said, it's in a state that I don't think either of us would want to depend on at this point. We can talk about whether it'd make sense to invest in things here, but this is low enough priority that I doubt it's worth it, and you're probably better off hand-coding something.

As far as all of the other config files goes, that's a real problem, but I think the foundation thinking for this is that we want to move to a meta-config file based on skylark (aka python) that would then generate the textproto files. It looks like there is a formatter as part of the Bazel projects.
This is the Bazel formatter I found. I have no hands-on experience with it.

https://github.com/bazelbuild/buildtools/blob/master/buildifier/README.md
FYI I'm going to be uploading CLs to enforce sorting which will link to this bug.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7eb8b6179d4b482e2e4076e42f2a75dd9c074c97

commit 7eb8b6179d4b482e2e4076e42f2a75dd9c074c97
Author: Stephen Martinis <martiniss@chromium.org>
Date: Fri Sep 21 00:17:50 2018

generate_buildbot_json.py: Better handle printed lines

This CL changes the tests for generate_buildbot_json.py to capture all
printed lines. It requires tests to assert about the contents of these
printed lines, and then clear them.

It also adds a --verbose flag to the main script, which is passed when
checking the configs.

Also fixes a test for swarming mixins and how they interact with certain
classes of tests.

Bug: 886993
Change-Id: I047d74689d72f35bc7d19c8da338c070a8a5d9a5
Reviewed-on: https://chromium-review.googlesource.com/1237239
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593015}
[modify] https://crrev.com/7eb8b6179d4b482e2e4076e42f2a75dd9c074c97/testing/buildbot/PRESUBMIT.py
[modify] https://crrev.com/7eb8b6179d4b482e2e4076e42f2a75dd9c074c97/testing/buildbot/generate_buildbot_json.py
[modify] https://crrev.com/7eb8b6179d4b482e2e4076e42f2a75dd9c074c97/testing/buildbot/generate_buildbot_json_unittest.py

Sign in to add a comment