Closed Bug 1273634 Opened 4 years ago Closed 4 years ago

Create a flake8 lint job

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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.
Flags: needinfo?(dtownsend)
(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
Flags: needinfo?(dtownsend)
Depends on: 1276409
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/
Attachment #8757659 - Flags: review?(dustin)
Attachment #8757659 - Attachment is obsolete: true
Attachment #8757659 - Flags: review?(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/
Attachment #8757671 - Flags: review?(dustin)
Attachment #8757672 - Flags: review?(smacleod)
Attachment #8757673 - Flags: review?(smacleod)
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/
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 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.
Attachment #8757671 - Flags: review?(dustin)
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 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 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/
Attachment #8757672 - Attachment description: MozReview Request: Bug 1273634 - [mozlint] Add a treeherder formatter, r?smacleod → MozReview Request: Bug 1273634 - [mozlint] Add a treeherder formatter, r?jgraham
Attachment #8757673 - Attachment description: MozReview Request: Bug 1273634 - Create a mozlint flake8 task, r?smacleod → MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?jgraham
Attachment #8757672 - Flags: review?(smacleod) → review?(james)
Attachment #8757673 - Flags: review?(smacleod) → review?(james)
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 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/
Attachment #8757672 - Flags: review?(james) → review+
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 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/
Attachment #8757673 - Attachment description: MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?jgraham → MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin
Attachment #8757673 - Flags: review?(james) → review?(dustin)
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 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 on attachment 8757673 [details]
MozReview Request: Bug 1273634 - [mozlint] Create a flake8 task, r?dustin

https://reviewboard.mozilla.org/r/56080/#review53682
Attachment #8757673 - Flags: review+
(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.
Attachment #8757673 - Flags: review?(dustin)
Pushed by ahalberstadt@mozilla.com:
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
we got a tree closure (tracked in bug 1277530) and per recommendation from garndt backing this changes out so we can reopen the trees
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout by cbook@mozilla.com:
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
Pushed by cbook@mozilla.com:
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
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.