Closed Bug 1403131 Opened 2 years ago Closed Last year

Run linters against mozharness scripts and configs

Categories

(Release Engineering :: Applications: MozharnessCore, enhancement, P1)

enhancement

Tracking

(firefox-esr60 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr60 --- fixed

People

(Reporter: rail, Assigned: tomprince, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [releaseduty])

Attachments

(2 files, 2 obsolete files)

In bug 1402955 we had a silly syntax issue which can be found by running flake8 on those files.

Enabling the linter is trivial, just need to add the corresponding directories to tools/lint/flake8.yml: https://dxr.mozilla.org/mozilla-central/source/tools/lint/flake8.yml?q=tools%2Flint%2Fflake8.yml&redirect_type=direct

The issue is that we need to fix this :)

✖ 1344 problems (1344 errors, 0 warnings)

They are mostly easy-to-fix PEP8 issues.

We can land the changes in chunks (per directory).

Sounds like a "good first bug"!
Mentor: rail
This is how I tested it:

1) clone mozilla-central:

  hg clone https://hg.mozilla.org/mozilla-central && cd mozilla-central

2) Run the following to verify that the linter works without any issues:

  ./mach lint -l flake8 testing/mozharness
 
3) modify tools/lint/flake8.yml and add "testing/mozharness/configs" after https://dxr.mozilla.org/mozilla-central/rev/33b7b8e81b4befcba503c0e48cd5370aeb715085/tools/lint/flake8.yml#18. Using "testing/mozharness/configs" limits the linter to that directory, "testing/mozharness" will enable everything.

4) re-run the linter to see the issues:

  ./mach lint -l flake8 testing/mozharness

The linter should print the issues that we should fix. We can probably relax some of the flake8 checks (like  max-line-length). This can be done per directory by adding ".flake8" files, see https://dxr.mozilla.org/mozilla-central/source/security/manager/.flake8 for example.
Hi, I can take this (I've completed a couple other flake8 bugs). If you assign to me I'll get started on this.
Assignee: nobody → stevea1
Thank you! Marking it as P1 (aka "actively worked on")
Priority: -- → P1
Whiteboard: [lang=python] → [lang=python][releaseduty]
Update: I've tallied up the number of changes per directory so I can start chunking up work. I'll use reasonable size increments to check in directories at a time.
I've posted the updates needed for the testing/mozharness/external_tools, mozfile, mozinfo, and scripts directories. Please let me know of any corrections needed.
Comment on attachment 8916460 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/187576/#review192598

Wow! This looks great! Can you rebase the patch against the inbound branch please before we push? I tried, but it requires manual conflict resolution. IIRC autoland wont' let you land patches with conflicts.
Attachment #8916460 - Flags: review?(rail)
Ok, I've rebased the patch, merge the conflicts and repushed for review. Let me know how it looks.
Comment on attachment 8916460 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/187576/#review193168

This is great. Thank you very much!

::: tools/lint/flake8.yml:19
(Diff revision 2)
>          - testing/marionette/client
>          - testing/marionette/harness
>          - testing/marionette/puppeteer
> -        - testing/mozbase
>          - testing/mochitest
> +        - testing/mozbase

Even tough mozbase is a different module, why not?! :)
Attachment #8916460 - Flags: review?(rail) → review+
testing/mozbase wasn't in alphabetical order, which was a little annoying, so I re-ordered it on the list!
Ah, I see. Thanks!
Hi Rail, what would recommend for next steps? Should I wait until these changes have made it into a build before submitting more directories, or just keep submitting?
Flags: needinfo?(rail)
Let's land this part by part. Otherwise we may end up fighting with conflicts (if we don't have one already!). Go ahead and try to autoland this! Let me know if you need a hand with it.
Flags: needinfo?(rail)
Sounds like a good plan. I haven't used autoland yet so I'll need a hand getting started with that.
Flags: needinfo?(rail)
* Rebase the patch again (sorry, I pushed something that conflicts with your patch). Should be relatively easy to resolve. 
* Push the change to reviewboard again. I'll approve it.
* Go to reviewboard https://reviewboard.mozilla.org/r/187576/diff/2#index_header
* There is Automation -> Land Commits, which should push the changes to the autoland branch

If the menu is not active for some reason, I can do that for you.
Flags: needinfo?(rail)
Ok, I rebased and reposted. Looks like Land Commits is greyed out for me (I don't have scm_level_3 access) though.
Comment on attachment 8916460 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/187576/#review193706
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 38ab52e77c12 -d 7290aaeb8074: rebasing 425288:38ab52e77c12 "Bug 1403131 - Run linters against mozharness scripts and configs.  r=rail" (tip)
merging tools/lint/flake8.yml
warning: conflicts while merging tools/lint/flake8.yml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: leave-open
Ouch, someone touched it again! :)
I would like to help with this one as well if it's no trouble. First time for me :)
I am not aware which files have been already done and submitted for review. How do people usually deal with this and avoid working on the same problems?
(In reply to Kristiyan from comment #22)
> I would like to help with this one as well if it's no trouble. First time
> for me :)

Hi Kristiyan, thanks for the offer but I have this one covered I'd say. Take a look at Bugs Ahoy (https://www.joshmatthews.net/bugsahoy/?unowned=1&simple=1) as there are plenty of unassigned bugs waiting for you to work on!
(In reply to Rail Aliiev [:rail] ⌚️ET from comment #21)
> Ouch, someone touched it again! :)

Ok, I think I've worked out the rebase issues if you want to send to tryserver again...
Hi Rail, I wasn't sure what the next step was to move this along...
Flags: needinfo?(rail)
We need to rebase it again, sorry... Autoland failed to do so. :/
Flags: needinfo?(rail)
Attachment #8916460 - Attachment is obsolete: true
Hi Rail, wanted to see if you could try again with the rebased and updated files.
Flags: needinfo?(rail)
The diff looks a bit weird on reviewboard. Let me pull them together and see if I can push them manually.
Flags: needinfo?(rail)
Pushed by raliiev@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa6cfe0199c
Run linters against mozharness scripts and configs.  r=rail
Steve, I pushed your patch to inbound https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6fa6cfe0199ca95e3a37f35f59ba047849e2ae80 \o/

I had to made a couple of changes though.

1) Apparently we refresh the contents of the external_tools directory from time to time by importing files. Someone has done this recently, so the linter won't pass. I thought that it'd be easier for everyone to ignore this directory, so I removed it from the config and reverted your changes (sorry).

2) There were a couple issues introduced since the patch, so I fixed them.
Backed out for mass reftest failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/32faa962a6056f8c774a4bc8af9fc4f8b6b18498

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6fa6cfe0199ca95e3a37f35f59ba047849e2ae80&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=138895006&repo=mozilla-inbound

[task 2017-10-23T14:12:29.796Z] 14:12:29    FATAL - Uncaught exception: Traceback (most recent call last):
[task 2017-10-23T14:12:29.797Z] 14:12:29    FATAL -   File "/builds/worker/workspace/mozharness/mozharness/base/script.py", line 2059, in run
[task 2017-10-23T14:12:29.798Z] 14:12:29    FATAL -     self.run_action(action)
[task 2017-10-23T14:12:29.799Z] 14:12:29    FATAL -   File "/builds/worker/workspace/mozharness/mozharness/base/script.py", line 1998, in run_action
[task 2017-10-23T14:12:29.799Z] 14:12:29    FATAL -     self._possibly_run_method(method_name, error_if_missing=True)
[task 2017-10-23T14:12:29.800Z] 14:12:29    FATAL -   File "/builds/worker/workspace/mozharness/mozharness/base/script.py", line 1938, in _possibly_run_method
[task 2017-10-23T14:12:29.801Z] 14:12:29    FATAL -     return getattr(self, method_name)()
[task 2017-10-23T14:12:29.801Z] 14:12:29    FATAL -   File "/builds/worker/workspace/mozharness/scripts/desktop_unittest.py", line 672, in run_tests
[task 2017-10-23T14:12:29.802Z] 14:12:29    FATAL -     if not self._run_category_suites(category):
[task 2017-10-23T14:12:29.803Z] 14:12:29    FATAL -   File "/builds/worker/workspace/mozharness/scripts/desktop_unittest.py", line 737, in _run_category_suites
[task 2017-10-23T14:12:29.803Z] 14:12:29    FATAL -     os.path.join(dirs["abs_reftest_dir"], "output.py")))
[task 2017-10-23T14:12:29.804Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/tests/reftest/output.py", line 9, in <module>
[task 2017-10-23T14:12:29.805Z] 14:12:29    FATAL -     from mozrunner.utils import get_stack_fixer_function
[task 2017-10-23T14:12:29.806Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozrunner/__init__.py", line 5, in <module>
[task 2017-10-23T14:12:29.806Z] 14:12:29    FATAL -     from .cli import *
[task 2017-10-23T14:12:29.807Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozrunner/cli.py", line 11, in <module>
[task 2017-10-23T14:12:29.808Z] 14:12:29    FATAL -     from .runners import runners
[task 2017-10-23T14:12:29.808Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozrunner/runners.py", line 11, in <module>
[task 2017-10-23T14:12:29.809Z] 14:12:29    FATAL -     from .base import DeviceRunner, GeckoRuntimeRunner, FennecRunner
[task 2017-10-23T14:12:29.810Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozrunner/base/__init__.py", line 1, in <module>
[task 2017-10-23T14:12:29.811Z] 14:12:29    FATAL -     from .runner import BaseRunner
[task 2017-10-23T14:12:29.811Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozrunner/base/runner.py", line 15, in <module>
[task 2017-10-23T14:12:29.812Z] 14:12:29    FATAL -     import mozcrash
[task 2017-10-23T14:12:29.813Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozcrash/__init__.py", line 10, in <module>
[task 2017-10-23T14:12:29.813Z] 14:12:29    FATAL -     from mozcrash import *
[task 2017-10-23T14:12:29.814Z] 14:12:29    FATAL -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/mozcrash/mozcrash.py", line 385, in <module>
[task 2017-10-23T14:12:29.815Z] 14:12:29    FATAL -     if mozinfo.isWin:
[task 2017-10-23T14:12:29.815Z] 14:12:29    FATAL - AttributeError: 'module' object has no attribute 'isWin'
[task 2017-10-23T14:12:29.816Z] 14:12:29    FATAL - Running post_fatal callback...
[task 2017-10-23T14:12:29.817Z] 14:12:29    FATAL - Exiting -1
Flags: needinfo?(stevea1)
I can make these corrections to 'testing/mozharness/mozfile/mozfile.py' and 'testing/mozharness/mozinfo/__init__.py'. But what's the best way for me to avoid sending the external_tools files that were changed when I check in again?
Flags: needinfo?(stevea1)
I'd say, don't bother fixing the external tools directory. Let it live it's own life. :)

You can take the push (https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa6cfe0199c) as the base and add fixes on top of it.
Comment on attachment 8920907 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/191856/#review197260
Attachment #8920907 - Flags: review?(rail)
(In reply to Rail Aliiev [:rail] ⌚️ET from comment #38)
> 
> You can take the push
> (https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa6cfe0199c) as the
> base and add fixes on top of it.

Ok. Can you help me understand the specific hg command(s) to take this "as the base"? I want to make sure I update my copy correctly.
Flags: needinfo?(rail)
Sure. I would start by re-applying the same version:

hg export 6fa6cfe0199c | hg import --no-commit -

At this point you'll have 6fa6cfe0199c applied to your working directory but, not commited.

When you are happy with the fixes, you can commit. The commit will include both the base patch and your fixes.
Flags: needinfo?(rail)
Makes sense. Stuck on this though:
hg export 6fa6cfe0199c | hg import --no-commit -
abort: unknown revision '6fa6cfe0199c'!
Flags: needinfo?(rail)
6fa6cfe0199c is on inbound only. If your base your work on mozilla central, that you can use something like this:

hg import --no-commit https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/6fa6cfe0199c
Flags: needinfo?(rail)
The flake8 complained about this:

./mach lint -l flake8 testing/mozharness

/testing/mozharness/mozfile/__init__.py
  5:1  error  'from mozfile import *' used; unable to detect undefined names  F403 (flake8)
  5:1  error  'mozfile.*' imported but unused                                 F401 (flake8)

Removing it last time caused the excitement in comment 35, so this time I'm using the # noqa flag on that line to have flake8 ignore it.
Comment on attachment 8920907 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/191856/#review197446

::: testing/mozharness/mozinfo/__init__.py:54
(Diff revision 2)
>     * :attr:`processor`
>     * :attr:`version`
>  
>  """
>  
>  import mozinfo

Can you use the same approach as in `testing/mozharness/mozfile/__init__.py` here too? I bet this time it will fail here.
Attachment #8920907 - Flags: review?(rail)
Comment on attachment 8920907 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/191856/#review197446

I've added the # noqa flag to 'import mozinfo' statement.
Pushed by raliiev@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f30519845d3
Run linters against mozharness scripts and configs. r=rail
I pushed it with a slightly changed mozinfo/__init__.py: https://hg.mozilla.org/integration/mozilla-inbound/diff/3f30519845d3/testing/mozharness/mozinfo/__init__.py

Sorry I wasn't clear when I asked to use "the same approach".

Hopefully it sticks this time! :)

Thank you very much!
Attachment #8920907 - Flags: review?(rail) → review+
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #51)
> https://hg.mozilla.org/mozilla-central/rev/3f30519845d3

Yes!
Attachment #8920907 - Attachment is obsolete: true
Ok, the 2nd batch of changes (testing/mozharness/mozharness/mozilla) is ready to try.
Flags: needinfo?(rail)
On thing to note (in case I mishandled this) - for the file testing/mozharness/mozharness/mozilla/testing/talos.py, the linter caught this:
  614:13   error  undefined name 'parser'              F821 (flake8)

So I removed that line from the code:
    
    def _artifact_perf_data(self, dest):
        src = os.path.join(self.query_abs_dirs()['abs_work_dir'], 'local.json')
        try:
            shutil.copyfile(src, dest)
        except:
            self.critical("Error copying results %s to upload dir %s" % (src, dest))
            parser.update_worst_log_and_tbpl_levels(CRITICAL, TBPL_FAILURE)   <---------removed
Comment on attachment 8923141 [details]
Bug 1403131 - Run linters against mozharness scripts and configs.

https://reviewboard.mozilla.org/r/194326/#review199846

Sweet! I'm going to land this asap! Thank you very much!
Attachment #8923141 - Flags: review?(rail) → review+
Pushed by raliiev@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/349b9517cb9b
Run linters against mozharness scripts and configs.  r=rail
Flags: needinfo?(rail)
I would like to help as well, I have worked with Linter before and it seems like a good firs one for me :). 
Could you please update me in what order you are doing them so I know from where I can begin?
Whiteboard: [lang=python][releaseduty] → [releaseduty]
Steve do you want to update this to current tip and fix the issues described in c#58, or are you no longer interested?
Flags: needinfo?(stevea1)
Yes, I'll see if I can take another run at this.
Flags: needinfo?(stevea1)
Found in triaging. Do we still need work in this or can we close it?
Flags: needinfo?(bugspam.Callek)
Assignee: stevea1 → nobody
I've removed myself as assignee, in case this isn't closed and someone else wants to take it.
Everything but the tests have been linted by flake8 now.
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(bugspam.Callek)
Resolution: --- → FIXED
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/3d742a851920
[mozharness] Run lint against tests; r=mtabara
Assignee: nobody → mozilla
You need to log in before you can comment on or make changes to this bug.