Closed Bug 1288425 Opened 4 years ago Closed 4 years ago

[mozlint] Flake8 will attempt to lint non-python files if you explicitly pass them in

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

STR:
mach lint -l flake8 tools/lint/eslint-formatter.js

If you explicitly pass in a file to flake8, it will disregard the 'filenames' config. I think this is correct behaviour on flake8's part, but we need to handle this in mozlint because this situation will be common when using the --rev/--workdir options (those will explicitly pass in every file that was touched, which probably include non-python files).

Ftr, I noticed that eslint will also spew out a warning (that we should avoid) if you explicitly pass it a .py file. So mozlint will need to be able to handle file extensions.

For now I'll add it to the existing LINTER dictionary. But I'm getting more and more convinced that I should come up with a better configuration system.
Some linters, such as flake8, will lint invalid file extensions if you explicitly pass them in. E.g,
|flake8 foobar.js| will result in flake8 attempting to lint a JS file. This is a problem because passing
in files explicitly is exactly what the --rev/--workdir options do. If a developer modifies a JS file
then runs |mach lint -l flake8 -w|, that JS file will get linted.

To prevent this, mozlint needs to handle file extensions instead of relying on the underlying linter to
do it. This patch adds an "extensions" config option to the LINTER dict, and will filter these files out
as part of the 'filterpaths' steps.

Review commit: https://reviewboard.mozilla.org/r/66054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66054/
Attachment #8773311 - Flags: review?(smacleod)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment on attachment 8773311 [details]
Bug 1288425 - Make sure we skip invalid extensions when linting with --rev or --workdir,

https://reviewboard.mozilla.org/r/66054/#review64164
Attachment #8773311 - Flags: review?(smacleod) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2141360b4137
Make sure we skip invalid extensions when linting with --rev or --workdir, r=smacleod
sorry had to backout your push for build bustage, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=1014985&repo=autoland#L30587
Flags: needinfo?(ahalberstadt)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f3e55cbfea9
Backed out changeset 2141360b4137 for build bustage
Sorry, bit of a perfect storm. These tests:
A) Pass for me locally with just this commit
B) Pass on try when included with another patch I wrote
C) Fail on try with just this commit alone

I was originally going to file a new bug for the other commit in the series, but given that this commit depends on it, I'll upload it here shortly instead.
Flags: needinfo?(ahalberstadt)
Depends on: 1289805
Nvm, mozreview doesn't like me re-submitting a new commit to the old review. So I'll wait for bug 1289805 to land before re-landing this (after proper try coverage of course!)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23e3bb6e8e93
Make sure we skip invalid extensions when linting with --rev or --workdir, r=smacleod
https://hg.mozilla.org/mozilla-central/rev/23e3bb6e8e93
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This is causing build oranges on c-c, due to mozlint test failures e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=44597&repo=comm-central#L28848-L29854

I suspect that's due to the path in https://hg.mozilla.org/mozilla-central/rev/23e3bb6e8e93#l3.14 (which might have to be mozilla/python/... for comm builds)?
Flags: needinfo?(ahalberstadt)
Sorry, yeah that looks like the reason. The mozlint tests definitely shouldn't depend on where they're located within the tree. I have a fix, and will file a new bug to clean this up.
Flags: needinfo?(ahalberstadt)
Depends on: 1297699
(In reply to Andrew Halberstadt [:ahal] from comment #12)
> Sorry, yeah that looks like the reason. The mozlint tests definitely
> shouldn't depend on where they're located within the tree. I have a fix, and
> will file a new bug to clean this up.

Thanks!
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.