Closed Bug 1449035 Opened 3 years ago Closed 3 years ago

Codespell: silent warnings about binary errors

Categories

(Firefox Build System :: Source Code Analysis, enhancement, P5)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1444048 +++
To silent:

Unable to match regex against output: WARNING: Binary file: /home/sylvestre/dev/mozilla/mozilla-central.hg/tools/lint/python/compat.pyc

Moving to --quiet-level=7 silent them.
Comment on attachment 8962533 [details]
Bug 1449035 - Codespell: move to quiet-level=7 to also ignore the binary

https://reviewboard.mozilla.org/r/231328/#review237078

I think there's a different issue here:

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/tools/lint/codespell.yml#23

pyc isn't listed, so if it is being matched, there's a bug in the lint code.

Generally I think we need to make sure we exclude (as we don't need to be testing these files anyway), rather than turn down the warnings.
Attachment #8962533 - Flags: review?(standard8)
Comment on attachment 8962533 [details]
Bug 1449035 - Codespell: move to quiet-level=7 to also ignore the binary

https://reviewboard.mozilla.org/r/231328/#review237084
Attachment #8962533 - Flags: review-
Mark, in the initial implementation, I thought that the list of extensions was managed by mozlint itself and not by the checkers themselves.
Now, I know that it isn't the case. 

That means that the list https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/tools/lint/codespell.yml#23 is unused.

Now, codespell doesn't give an option to limit the extensions being checked. Only to ignore a few.

I see a few options here:
1) We add quiet-level=7 and remove the extensions list to avoid 

2) Maintain a list like --skip=*.dic,*.png,*.pyc,*.bmp,*.jpg,*.ico,*.icns,dsstore

3) Implement the feature in codespell upstream
Flags: needinfo?(standard8)
Press "Save changes" too quickly.

Please let me know what you think!
This is showing a lot of useless warnings in the CI:
https://treeherder.mozilla.org/logviewer.html#?job_id=178014892&repo=autoland&lineNumber=247
Flags: needinfo?(ahalberstadt)
Correct, if you pass in a directory, mozlint will just forward that directory to the underlying linter. This is mainly because otherwise mozlint would have to convert that directory into a list of files and pass the entire thing in (which will exceed the maximum command length on most operating systems if linting the entire tree). If you pass in files or globs, then mozlint *will* use that extension list to pre-filter.

Of those 3 options, I think I'd prefer 1 or 3. There is another option though. We could convert directories into lists of files and do our own filtering. There are utilities in-tree to facilitate this, and the py2/py3 linters actually already do this:
https://searchfox.org/mozilla-central/source/tools/lint/python/compat.py#71

Once we have our own pre-filtered list of files, we can either pass them into codespell explicitly (being careful not to exceed the maximum command line length), or instead of calling codespell via a subprocess, we could call into its internals directly (as it is written in python).
Flags: needinfo?(ahalberstadt)
For now, I propose that we do 1) for now and 3) later.
Assignee: nobody → sledru
Summary: Codespell: ignore more stuff → Codespell: silent warnings about binary errors
Comment on attachment 8962533 [details]
Bug 1449035 - Codespell: move to quiet-level=7 to also ignore the binary

https://reviewboard.mozilla.org/r/231328/#review249686
Attachment #8962533 - Flags: review?(ahalberstadt) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca08a480132a
Codespell: move to quiet-level=7 to also ignore the binary r=ahal
https://hg.mozilla.org/mozilla-central/rev/ca08a480132a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.