Closed Bug 1654103 Opened 6 months ago Closed 3 months ago

autoformat entire repo with black

Categories

(Firefox Build System :: General, task)

task

Tracking

(firefox84 fixed)

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: rstewart, Assigned: rstewart)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → rstewart
Blocks: 1672023

Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.

To produce this patch I did all of the following:

  1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.

  2. Run ./mach lint --linter black --fix

  3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.

  4. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).

Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.

To produce this patch I did all of the following:

  1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.

  2. Run ./mach lint --linter black --fix

  3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.

  4. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).

Attachment #9183034 - Attachment is obsolete: true
Pushed by rstewart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7558c8821a07
Standardize on Black for Python code in `mozilla-central`. r=remote-protocol-reviewers,marionette-reviewers,webdriver-reviewers,perftest-reviewers,devtools-backward-compat-reviewers,jgilbert,preferences-reviewers,sylvestre,maja_zf,webcompat-reviewers,denschub,ntim,whimboo,sparky

The fact that this had to be backed out for test bustage (effectively, harness bustage) doesn't give me much confidence in black as a reformatting tool.

Well, I wouldn't blame black here. The thing is that two individual strings have been updated, but the regex string hasn't been taken into that it will fail to parse the source file. Here the causing change:

-    return re.findall("__version__ = '([\d\.]+)'",
-                      read('marionette_driver', '__init__.py'), re.M)[0]
+    return re.findall(
+        "__version__ = '([\d\.]+)'", read("marionette_driver", "__init__.py"), re.M
+    )[0]

That will match __version__ = 'x.y.z', but note that this string has also been changed to double quotes.

Yeah, I don't think this is a proof that black isn't a good tool. We had similar issues with the clang-format move:
A bunch of scripts doing code parsing with some strong assumptions on the style used. Example bug 1499323

(In reply to Mike Hommey [:glandium] from comment #5)

The fact that this had to be backed out for test bustage (effectively, harness bustage) doesn't give me much confidence in black as a reformatting tool.

black checks that the reformatted code still produces a valid AST that is equivalent to the original.
Of course that won't catch instances where we parse Python with regexes.

Amazing. Okay, I'll make that one-line change then re-land.

Flags: needinfo?(rstewart)
Blocks: 1672798
Pushed by rstewart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7834b600201
Standardize on Black for Python code in `mozilla-central`. r=remote-protocol-reviewers,marionette-reviewers,webdriver-reviewers,perftest-reviewers,devtools-backward-compat-reviewers,jgilbert,preferences-reviewers,sylvestre,maja_zf,webcompat-reviewers,denschub,ntim,whimboo,sparky
Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.
Flags: needinfo?(rstewart)

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #12)

Backed out for conflicts when backing out bug 1518999.

Next time, please backout the smaller patch. It will be easier to rebase bug 1518999 than this one :)

(In reply to Sylvestre Ledru [:Sylvestre] from comment #13)

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #12)

Backed out for conflicts when backing out bug 1518999.

Next time, please backout the smaller patch. It will be easier to rebase bug 1518999 than this one :)

The problem was backing out a patch that landed before this one. This bug should land when the tree is closed and known to be clean. That would also reduce the risk of race condition between when the patch is updated and something pythonic landing.

(In reply to Mike Hommey [:glandium] from comment #14)

(In reply to Sylvestre Ledru [:Sylvestre] from comment #13)

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #12)

Backed out for conflicts when backing out bug 1518999.

Next time, please backout the smaller patch. It will be easier to rebase bug 1518999 than this one :)

The problem was backing out a patch that landed before this one. This bug should land when the tree is closed and known to be clean. That would also reduce the risk of race condition between when the patch is updated and something pythonic landing.

I agree. I'll engage the sheriffs on this.

Flags: needinfo?(rstewart)
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/994ae8e4833c
Standardize on Black for Python code in `mozilla-central`.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

Can we get an exclusion added to the HG blame/searchfox logic so this doesn't pollute every display? I think this would be like https://bugzilla.mozilla.org/show_bug.cgi?id=1633969.

Flags: needinfo?(rstewart)
Blocks: 1673690

(In reply to Nick Alexander :nalexander [he/him] from comment #18)

Can we get an exclusion added to the HG blame/searchfox logic so this doesn't pollute every display? I think this would be like https://bugzilla.mozilla.org/show_bug.cgi?id=1633969.

bug 1673690

Flags: needinfo?(rstewart)
Regressed by: 1673700
No longer regressed by: 1673700
Regressions: 1673700
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26356 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
Depends on: 1675099

ignore-this-changeset

Depends on D96609

Attachment #9186970 - Attachment is obsolete: true

Hey Ryan -- it's been suggested that we backport this reformat to ESR so that anyone who needs to backport a Python change won't encounter merge conflicts. Does that make sense to you? We figured you might have a better idea of whether the benefit would be worth it.

Flags: needinfo?(ryanvm)

Given that we've got about another year of support for ESR78, it's probably not a bad idea if it's not too much trouble.

Flags: needinfo?(ryanvm)
Regressions: 1687947
You need to log in before you can comment on or make changes to this bug.