Closed Bug 1408365 Opened 7 years ago Closed 7 years ago

Enable flake8/py2/py3 linters on python/mozboot

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ahal, Assigned: stevea1, Mentored)

References

Details

(Whiteboard: [lang=py])

Attachments

(1 file)

Bug 1408365 - Enable flake8/py2/py3 linters on python/mozboot.
59 bytes, text/x-review-board-request
ahal
: review+
Details 
+++ This bug was initially created as a clone of Bug #1407763 +++

These linters can be enabled by removing 'python/mozboot' from the exclude section in tools/lint/py2.yml and tools/lint/py3.yml and adding 'python/mozboot' to the include section of tools/lint/flake8.yml.

You can then see all the errors by running:
./mach lint -l flake8 -l py2 -l py3 python/mozboot

Each of the errors will need to be fixed before this can land.
Turns out py3 was already enabled, so I enabled py2 and flake8 and corrected those errors. Looks good to me. Can you push to tryserver?
Comment on attachment 8918517 [details]
Bug 1408365 - Enable flake8/py2/py3 linters on python/mozboot.

https://reviewboard.mozilla.org/r/189380/#review194972

Thanks, this mostly looks good, just a couple suggestions for better fixes. Giving an r- for now because this doesn't enable the `py3` linter. We could definitely handle that in a follow-up though if there are a lot of issues.

::: python/mozboot/mozboot/base.py:674
(Diff revision 1)
> -                '--default-host', platform,
> -            ])
> +                                   '--default-host', platform, ]
> +                                  )

nit: let's leave the closing ] next to the ). E.g:

    ...call([rustup_init, '-y',
             '--default-toolchain', 'stable',
             '--default-host', platform])

::: python/mozboot/mozboot/osx.py:26
(Diff revision 1)
> -MACPORTS_URL = {'11': 'https://distfiles.macports.org/MacPorts/MacPorts-2.3.4-10.11-ElCapitan.pkg',
> -                '10': 'https://distfiles.macports.org/MacPorts/MacPorts-2.3.4-10.10-Yosemite.pkg',
> -                '9': 'https://distfiles.macports.org/MacPorts/MacPorts-2.3.4-10.9-Mavericks.pkg',
> -                '8': 'https://distfiles.macports.org/MacPorts/MacPorts-2.3.4-10.8-MountainLion.pkg',
> -                '7': 'https://distfiles.macports.org/MacPorts/MacPorts-2.3.4-10.7-Lion.pkg',
> -                '6': 'https://distfiles.macports.org/MacPorts/MacPorts-2.3.4-10.6-SnowLeopard.pkg', }
> +MACPORTS = 'https://distfiles.macports.org/MacPorts/'
> +MACPORTS_URL = {'11': MACPORTS+'MacPorts-2.3.4-10.11-ElCapitan.pkg',
> +                '10': MACPORTS+'MacPorts-2.3.4-10.10-Yosemite.pkg',
> +                '9': MACPORTS+'MacPorts-2.3.4-10.9-Mavericks.pkg',
> +                '8': MACPORTS+'MacPorts-2.3.4-10.8-MountainLion.pkg',
> +                '7': MACPORTS+'MacPorts-2.3.4-10.7-Lion.pkg',
> +                '6': MACPORTS+'MacPorts-2.3.4-10.6-SnowLeopard.pkg', }

Instead of making a variable, try something like this:

    MACPORTS_URL = {
        '11': 'https://...',
        '10': 'https://...',
        ...
    }

::: tools/lint/flake8.yml:13
(Diff revision 1)
>          - config/check_macroassembler_style.py
>          - config/mozunit.py
>          - layout/tools/reftest
>          - python/mach
>          - python/mach_commands.py
> +        - python/mozboot        

extra whitespace here (this should fail the `yaml` linter)

::: tools/lint/py2.yml
(Diff revision 1)
>          - nsprpub
>          - other-licenses
>          - probes/trace-gen.py
>          - python/devtools
>          - python/mach
> -        - python/mozboot

Looks like you forgot to enable the `py3` linter. We could tackle that separate though if you like.
Attachment #8918517 - Flags: review?(ahalberstadt) → review-
Thanks, I'll clean up per your suggestions.

Not sure if you saw my comment 2, but it looks like py3 is already enabled for all of the python directories (i.e. there's no python/mozboot to remove from the exclude list tools/lint/py3.yml). Or maybe I'm missing something?
Flags: needinfo?(ahalberstadt)
Oops I didn't notice, thanks for pointing that out. I'll flip the review flag and push to try after you've pushed the fixes up.
Flags: needinfo?(ahalberstadt)
Fixes pushed up, ready for another look.
Comment on attachment 8918517 [details]
Bug 1408365 - Enable flake8/py2/py3 linters on python/mozboot.

https://reviewboard.mozilla.org/r/189380/#review195400

Thanks! I'll do a try push.
Attachment #8918517 - Flags: review?(ahalberstadt) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=13e12aa8113402250e4f9ce4c921ecb4edc0010f

I talked to Pike about those L10n failures and he said it was due to a bad m-c base, so nothing to worry about. I think this is good to land.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1ee605f2cd9b -d 1083fb72eed4: rebasing 427021:1ee605f2cd9b "Bug 1408365 - Enable flake8/py2/py3 linters on python/mozboot. r=ahal" (tip)
merging python/mozboot/mozboot/android.py
merging python/mozboot/mozboot/base.py
warning: conflicts while merging python/mozboot/mozboot/android.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Hey Steve, looks like there are merge conflicts. Would you mind pulling the latest copy of mozilla-central, rebasing your patch on top and re-pushing to review?
Flags: needinfo?(stevea1)
Updated with rebased copy. Ready for another try.
Flags: needinfo?(stevea1)
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b7c1abb35a8
Enable flake8/py2/py3 linters on python/mozboot. r=ahal
https://hg.mozilla.org/mozilla-central/rev/2b7c1abb35a8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
My guess is that this ticket has broken import in |mach bootstrap|: see Bug 1410454 and reports from IRC, like:

<mib_mub6bj> Hi I'm trying to build firefox but I'm unable to get it to work , I'm getting an import error while running mach bootstrap.
https://pastebin.mozilla.org/9070718

which is

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ImportError: No module named stylo

  File "c:\mozilla-source\mozilla-central\python/mozboot/mozboot/mach_commands.py", line 32, in bootstrap
    bootstrapper.bootstrap()
  File "c:\mozilla-source\mozilla-central\python/mozboot\mozboot\bootstrap.py", line 372, in bootstrap
    self.instance.ensure_stylo_packages(state_dir, checkout_root)
  File "c:\mozilla-source\mozilla-central\python/mozboot\mozboot\mozillabuild.py", line 50, in ensure_stylo_packages
    import stylo
  File "c:\mozilla-source\mozilla-central\build/mach_bootstrap.py", line 353, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
gps: ahal: Steve: can we either:

- prove this isn't the responsible patch
- fix this
- back it out

very quickly?  Thanks!
Flags: needinfo?(stevea1)
Flags: needinfo?(gps)
Flags: needinfo?(ahalberstadt)
Those 'import stylo' and 'import android' statements need to be changed to 'from mozboot import android'. This wasn't caught by the py2 linter because they are defined in a function (and therefore not hit at import time). It wasn't caught by testing because mozboot doesn't have tests.

I'll post a fix over in bug 1410454 shortly.
Flags: needinfo?(stevea1)
Flags: needinfo?(gps)
Flags: needinfo?(ahalberstadt)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: