Closed Bug 1542963 Opened 6 months ago Closed 5 months ago

Lint python/mozbuild/{mozbuild,mozpack}

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: Callek, Assigned: Callek)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

In preparing for python3 support in mozpack, I'm setting up python/mozbuild/mozbuild and python/mozbuild/mozpack for flake8 linting, in addition to setting future magic to make sure we don't break them.

I'm only setting unicode_literals on the mozbuild files imported from mozpack, and not all of mozbuild at this time.

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9edd2c2e074cf65777bb0d0c38781ad3beaa9fe7 (mostly since not all of this is covered by tests)

Type: defect → enhancement

Autoformat with Black against python/mozbuild/mozbuild

flake8 python/mozbuild/mozbuild

Depends on D26638

Now that we've run black, run './mach lint -l py2 -l flake8 -l shellcheck -l codespell -l yaml python/mozbuild/mozbuild/ --fix' in order to undo some black changes and get closer to making this folder able to be validated on every lint run

Depends on D26639

Fix flake8 issues in python/mozbuild/mozbuild/*, except for one I want an explicit review on

Depends on D26640

Fix undefined var 'defines' when use_defines_in_asflags is set.

Depends on D26641

Add 'from future import absolute_import' to all missing files in python/mozbuild/{mozbuild,mozpack}/**/*.py

Depends on D26642

use future print_function in all mozbuild/mozbuild and mozbuild/mozpack files

Depends on D26643

Insert unicode_literals into python/mozbuild/mozpack and called mozbuild/* files.

Depends on D26644

Attachment #9056777 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - Autoformat with Black against python/mozbuild/mozbuild r=#build
Attachment #9056778 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - flake8 python/mozbuild/mozbuild r=#build
Attachment #9056779 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - run './mach lint ... --fix' on mozbuild/mozbuild, undoes some black changes. r=#build
Attachment #9056780 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - Fix most flake8 issues in python/mozbuild/mozbuild/* r=#build
Attachment #9056781 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - Fix undefined var 'defines' when use_defines_in_asflags is set. r=#build
Attachment #9056782 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - Add absolute_imports to mozbuild and mozpack. r=#build
Attachment #9056783 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - use print_function for mozbuild and mozpack. r=#build

First off, thank you for taking this on! I have a couple of concerns about this that I think we should take into account before accepting these patches:

First, what is the state of tooling to keep these changes up to date? What prevents code in tree from sliding back into a non-python3 amenable state?

Second we should be aware that there is a large amount of code that consumes this code, namely mach commands and to some extent mach bootstrap, that do not currently have extensive automated tests running against them. We should take care when making changes related to str/bytes/unicode, as the existing code is likely to be incorrect in some cases (bug 1542042 was a recent example). Additional manual testing may be prudent as a result.

(In reply to Chris Manchester (:chmanchester) from comment #12)

First off, thank you for taking this on! I have a couple of concerns about this that I think we should take into account before accepting these patches:

First, what is the state of tooling to keep these changes up to date? What prevents code in tree from sliding back into a non-python3 amenable state?

Both
./mach python-test --python 3
./mach lint -l flake8 -l py2 -l py3

would ideally catch most of this, although the tests for these files are only run as part of make check (the landable patches don't change that) and the linters don't enforce __future__ imports in any way. but I hope to enforce what I can via them for the short term.

Second we should be aware that there is a large amount of code that consumes this code, namely mach commands and to some extent mach bootstrap, that do not currently have extensive automated tests running against them. We should take care when making changes related to str/bytes/unicode, as the existing code is likely to be incorrect in some cases (bug 1542042 was a recent example). Additional manual testing may be prudent as a result.

This specific bug is going to tackle py2 only, and just do future imports.

The unicode/string/bytes bit is going to be the most likely to have issues, and is why I have a try run (that is currently broken on windows due to this specific issue!) to try and fetter it out, even though the make check tests seem to pass.

I hope to add python-test -p3 support to some of this as well, but will only [at this time] be doing the py3 bits that I need for an unrelated project, as part of a separate bug.

The theory being of course, that from __future__ import unicode_literals changes should account for a large chunk of issues here, and less any change I make for py3 (since the unicode/bytes split is part of the problem space)

Attachment #9056784 - Attachment description: Bug 1542963 - Lint python/mozbuild/{mozbuild,mozpack}. r=#build → Bug 1542963 - Insert unicode_literals into python/mozbuild/mozpack and called mozbuild/* files. r=#build

Hi Chris, there was some discussion in #build today re this set of patches do you have any insight to provide as build module owner?

Flags: needinfo?(cmanchester)

I'm going to suggest against the auto-formatting patches until we have a definite plan in place for that.

Does python 3 support depend on the code being lint clean? The relationship between those things is not clear from this bug.

Flags: needinfo?(cmanchester)

Depend directly, no.

But my own ability to push this forward does, as well as the existing patch set I have, so I want to see this land first. With our without black.

I don't really understand hesitation around not linting though.

4:05 PM <Callek_cloud9> chmanchester: lint clean made the code 10x easier to reason about and now of course much of my manual py3 patches depend on that programmatically

Flags: needinfo?(cmanchester)

I don't have any particular hesitation about linting. I'm trying to understand the context.

Flags: needinfo?(cmanchester)
Attachment #9056777 - Attachment is obsolete: true
Blocks: 1547730
Blocks: 1548259
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f69c7eeb6fd
flake8 python/mozbuild/mozbuild r=#build
https://hg.mozilla.org/integration/mozilla-inbound/rev/2715bac40d2f
run './mach lint ... --fix' on mozbuild/mozbuild, undoes some black changes. r=#build
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a714f491d5
Fix most flake8 issues in python/mozbuild/mozbuild/* r=#build
https://hg.mozilla.org/integration/mozilla-inbound/rev/14aa1bd254a4
Fix undefined var 'defines' when use_defines_in_asflags is set.  r=#build
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a84e0feabb
Add absolute_imports to mozbuild and mozpack. r=#build
https://hg.mozilla.org/integration/mozilla-inbound/rev/70fbe1a158ae
use print_function for mozbuild and mozpack. r=#build
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f3c10562df3
Insert unicode_literals into python/mozbuild/mozpack and called mozbuild/* files. r=#build
Attachment #9056786 - Attachment is obsolete: true
Attachment #9056785 - Attachment is obsolete: true
Backout by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee4b88439111
Backed out 27 changesets (bug 1542963, bug 1547730) on request from Callek for DWE fails. CLOSED TREE
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/3d88030030a1
Fix linting f8 failure. r=tomprince a=fix
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/c5c101d8326f
Backed out changeset 3d88030030a1 for conflicts on backing out Bug 1542963 . a=backout
Pushed by jwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfbd665029ba
flake8 python/mozbuild/mozbuild r=glandium
https://hg.mozilla.org/integration/autoland/rev/41e140939316
run './mach lint ... --fix' on mozbuild/mozbuild, undoes some black changes. r=glandium
https://hg.mozilla.org/integration/autoland/rev/bf11356a308c
Fix most flake8 issues in python/mozbuild/mozbuild/* r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/d6653dd3105a
Fix undefined var 'defines' when use_defines_in_asflags is set.  r=glandium
https://hg.mozilla.org/integration/autoland/rev/42d95aa48b7c
Add absolute_imports to mozbuild and mozpack. r=glandium
https://hg.mozilla.org/integration/autoland/rev/1b57b9bd7435
use print_function for mozbuild and mozpack. r=glandium
https://hg.mozilla.org/integration/autoland/rev/d799c2e1cd02
Insert unicode_literals into python/mozbuild/mozpack and called mozbuild/* files. r=firefox-build-system-reviewers,chmanchester
Flags: needinfo?(bugspam.Callek)
You need to log in before you can comment on or make changes to this bug.