1273634 - Create a flake8 lint job

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Andrew Halberstadt [:ahal]]

A mozlint integration for flake8 is about to land. We should create a lint task for it.

Note, I have a bug on file to get eslint based on mozlint as well. But until that happens, this task will be slightly different from the eslint one.

Comment 1

[Andrew Halberstadt [:ahal]]

Hey Mossop, I'd like to add a flake8 job, and I think it probably makes sense to have a single image for all lint jobs. I noticed the eslint job is using the node-4.2 image from dockerhub, so I have two options:

1) Switch it to e.g the official ubuntu image (or something) and install npm/node into it
2) Install the flake8 dependencies directly into the existing node-4.2 image

1 feels cleaner, but 2 could be easier. Is there anything else in the official node image that is required (apart from node/npm/eslint packages) that might cause problems? I'm leaning towards 1) atm.

Comment 2

[Dave Townsend [:mossop]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> Hey Mossop, I'd like to add a flake8 job, and I think it probably makes
> sense to have a single image for all lint jobs. I noticed the eslint job is
> using the node-4.2 image from dockerhub, so I have two options:
> 
> 1) Switch it to e.g the official ubuntu image (or something) and install
> npm/node into it
> 2) Install the flake8 dependencies directly into the existing node-4.2 image
> 
> 1 feels cleaner, but 2 could be easier. Is there anything else in the
> official node image that is required (apart from node/npm/eslint packages)
> that might cause problems? I'm leaning towards 1) atm.

There shouldn't be anything specific, I think I just went with the node image because it already had most of what we wanted. You should double-check which node version the ubuntu image has, you may need to add node's package repository to get the right version

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1273634 - Use a generic ubuntu image as a base for the lint image, r?dustin

The ES job uses an image based on the official node image. While this was convenient
for eslint, it is a bit less convenient for other things. I want to use this image
for all lint jobs, and switching the base to a generic ubuntu image seems a bit cleaner.

I chose 16.04 for no good reason other than it is the most recent, and we might as well.
Node v4.4.5 and taskcluster-vcs have been uploaded to tooltool.

Review commit: https://reviewboard.mozilla.org/r/56060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56060/

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1273634 - Add flake8 dependencies to the lint image, r?dustin

This adds flake8 dependencies. Note that ubuntu 16.04 repos include pip > 8.0
which has peep merged into it, so there's no need to install peep separately.

I also ran into a locale issue which was causing a UnicodeDecodeError anytime
python tried to print a unicode character. The "locale-gen/dpkg-reconfigure locales"
fix the problem.

Review commit: https://reviewboard.mozilla.org/r/56076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56076/

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1273634 - [mozlint] Add a treeherder formatter, r?jgraham

This is a really simple and ugly formatter that is compatible with
treeherder's error highlighting mechanism. It is designed to be identical
to the current eslint output on treeherder.

Once bug 1276486 is fixed, we can make this look a little nicer. But for
now it gets the job done.

Review commit: https://reviewboard.mozilla.org/r/56078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56078/

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin

Enables flake8 linting! To start, only these directories are actually linted:
- python/mozlint
- tools/lint

To enable new directories, add them to the 'include' directive at the bottom of:
tools/lint/flake8.lint

Edit topsrcdir/.flake8 to modify global configuration. Add a new .flake8 to a
subdirectory to override the global. The current configuration is more or less
just the default and we should tweak it to our needs.

Review commit: https://reviewboard.mozilla.org/r/56080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56080/

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8757671 [details]
MozReview Request: Bug 1273634 - Add flake8 dependencies to the lint image, r?dustin

https://reviewboard.mozilla.org/r/56076/#review52736

I think MozReview is lying to me?  All I see is a diff to system-setup.sh, which would mean this is still using the node image..  It's cool that we don't have to download peep anymore, though!

Based on your bugzilla comment, it's worth noting that pulling from The Internet is OK in a docker image build, as long as there is some verification of the download (so, peep, or for node comparing the sha256 to a fixed value) and a reasonable expectation that the file will persist somewhere on the internet for a long time.  If a docker image build fails because someone server is temporarily down, that's OK -- we have backup docker images around that we can fall back to while retrying and/or looking for another URL.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

https://reviewboard.mozilla.org/r/56076/#review52736

Oh, of course, then I look at bug 1276409, next up in my queue, and that's doing the Dockerfile modifications :)

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8757671 [details]
MozReview Request: Bug 1273634 - Add flake8 dependencies to the lint image, r?dustin

https://reviewboard.mozilla.org/r/56076/#review52742

::: testing/docker/lint/system-setup.sh:22
(Diff revision 1)
> +apt_packages+=('sudo')
>  apt_packages+=('xz-utils')
>  
>  apt-get update
>  apt-get install -y ${apt_packages[@]}
> +locale-gen en_US.UTF-8

A comment would be helpful here so the next person knows what this is for

::: testing/docker/lint/system-setup.sh:70
(Diff revision 1)
> +
> +###
> +# Flake8 Setup
> +###
> +
> +cat >requirements.txt <<'EOF'

This is pretty cool :)

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8757671 [details]
MozReview Request: Bug 1273634 - Add flake8 dependencies to the lint image, r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56076/diff/1-2/

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8757672 [details]
MozReview Request: Bug 1273634 - [mozlint] Add a treeherder formatter, r?jgraham

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56078/diff/1-2/

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8757673 [details]
MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56080/diff/1-2/

Comment 13

[James Graham [:jgraham]]

Comment on attachment 8757672 [details]
MozReview Request: Bug 1273634 - [mozlint] Add a treeherder formatter, r?jgraham

https://reviewboard.mozilla.org/r/56078/#review53662

::: python/mozlint/mozlint/formatters/treeherder.py:25
(Diff revision 2)
> +        message = []
> +        for path, errors in sorted(result.iteritems()):
> +            for err in errors:
> +                assert isinstance(err, ResultContainer)
> +
> +                d = {s: getattr(err, s) for s in err.__slots__}

Seems like this should be moved to a method on ResultContainer rather than reimplemented in multiple places.

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8757671 [details]
MozReview Request: Bug 1273634 - Add flake8 dependencies to the lint image, r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56076/diff/2-3/

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8757672 [details]
MozReview Request: Bug 1273634 - [mozlint] Add a treeherder formatter, r?jgraham

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56078/diff/2-3/

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8757673 [details]
MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56080/diff/2-3/

Comment 17

[Greg Arndt [:garndt]]

Comment on attachment 8757673 [details]
MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin

https://reviewboard.mozilla.org/r/56080/#review53682

Comment 18

[Greg Arndt [:garndt]]

(In reply to Greg Arndt [:garndt] from comment #17)
> Comment on attachment 8757673 [details]
> MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin
> 
> https://reviewboard.mozilla.org/r/56080/#review53682

took a quick look at this and dont' see anything off about it.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3f4cbf0bfe
Add flake8 dependencies to the lint image, r=dustin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c48f3b04c9de
[mozlint] Add a treeherder formatter, r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f521a9d4d1f
[mozlint] Create a flake8 task, r=garndt

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c3f4cbf0bfe
https://hg.mozilla.org/mozilla-central/rev/c48f3b04c9de
https://hg.mozilla.org/mozilla-central/rev/3f521a9d4d1f

Comment 21

[Carsten Book [:Tomcat]]

we got a tree closure (tracked in bug 1277530) and per recommendation from garndt backing this changes out so we can reopen the trees

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/379f4fff66d5
Backed out changeset 3f521a9d4d1f 
https://hg.mozilla.org/mozilla-central/rev/d8fb9ee64c5c
Backed out changeset c48f3b04c9de 
https://hg.mozilla.org/mozilla-central/rev/92e0c73391e7
Backed out changeset 0c3f4cbf0bfe in order to reopen closed trees due to bug 1277530

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd123ca39e1f
Add flake8 dependencies to the lint image, r=dustin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ae9ac1a071
[mozlint] Add a treeherder formatter, r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb90089f9a8a
[mozlint] Create a flake8 task, r=dustin

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd123ca39e1f
https://hg.mozilla.org/mozilla-central/rev/e4ae9ac1a071
https://hg.mozilla.org/mozilla-central/rev/cb90089f9a8a