bare `mach lint --warnings` is mostly unusable due to rust clippy stuff
Categories
(Developer Infrastructure :: Lint and Formatting, task, P3)
Tracking
(firefox76 fixed)
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(3 files)
I'm not sure if this is the intention or not, but here's my current workflow:
- Submit a python-only patch to phabricator. (https://phabricator.services.mozilla.com/D66171 is an example.)
- reviewbot gives me a pyflakes warning with the message to run
./mach lint --warnings path/to/file
- note that this pretty much always happens, which you could say is because I don't enable the format-source extension or whatever it's called, but that's still broken and causes major headaches for me (bug 1557535).
- I either have a bunch of files in the patch and don't want to figure out which ones to use, or I'm simply too lazy to cut & paste the single filename in the patch. So I run
./mach lint --warnings
with a naive expectation that it will run only what is necessary to recreate the error. ./mach lint
eats my machine, pegging the cores and compiling a whole bunch of code. Apparently it's to run rust-clippy, which wants to compile everything, including mozjs which involves recompiling the entire spidermonkey tree. I'll attach a process tree documenting this.
And in this case,
- file a bug complaining about it
- in the process of filing the bug, discover the
-w
flag which would enable me to avoid this situation part of the time. Though actually not, in this case, since I have no changes in my working directory. I really want to run against either the changes in outgoing patches, or changes in a single specified revision.
For the purpose of this bug, I think it would work to just skip the rust-clippy stuff if the only changes are to Python code? (Though maybe that's invalid if you're editing eg dom/bindings/Codegen.py?)
At any rate, it seems like there's fancy automation to run linting on the minimal subset of stuff necessary, and it's not working because of the rust-clippy integration. (Which I'm assuming does the same thing even if you have a fully built tree, because it's compiling its own copies of stuff?)
Assignee | ||
Comment 1•5 years ago
|
||
Ah. It looks like I really want to be running mach lint --warnings -o
. Could that be what is displayed in the reviewbot message, or at least an alternative?
Assignee | ||
Comment 2•5 years ago
|
||
(Edited: I messed up and ran on my fixed version, so it correctly found nothing.)
Comment 3•5 years ago
|
||
Sounds like there are a couple problems here..
- Submitting a Python-only patch to phabricator should not show any clippy results at all.
- The default of run every linter on every file is not great. See https://bugzilla.mozilla.org/show_bug.cgi?id=1369784 comment 4 and down. I decided not to make
--outgoing
the default, but would be open to reconsidering. But otherwise, running./mach lint
without qualifiers is expected to eat your machine (even without clippy). Actually typing that out makes me realize that I should re-open that bug :). - The clippy lint is very slow / resource intensive.
Updated•5 years ago
|
Comment 4•5 years ago
|
||
I think I misread comment 0 and the problem #1 isn't actually happening. In that case, this isn't a defect after all. Let's use this bug to track problem #3 and bug 1369784 to track problem #2.
Updated•5 years ago
|
Comment 5•5 years ago
|
||
Bastien, could you update the message reviewbot displays to something like comment 1?
Comment 6•5 years ago
|
||
I just made a PR to do that.
We also have an open issue to use an extra Taskcluster payload so that the analyzer can specify the command itself: https://github.com/mozilla/code-review/issues/339
Assignee | ||
Comment 7•5 years ago
|
||
Thanks!
Thinking about it, I think my 80% case is wanting to run lint on the files touched by a single patch (well, phabricator revision). And 80% of that 80%, I want to run it on the top patch in my mercurial draft patch stack. That's because usually what's happening is that I'm getting reviewbot feedback from phabricator, and I want to fix the issues locally. I could just look at what reviewbot reports, but generally I'll end up editing the code and wanting to run it multiple times, so I figure I may as well start out running locally.
If I'm touching a single file, then the suggested file at a time command is fine. But in practice that feels less common than editing several python files at once.
Outgoing (-o
) is less likely to be the right thing for me because especially with Python changes, I'm generally doing them on top of an existing stack of several unrelated changes. (This is specific to me, and does not argue for changing defaults for everyone unless this case is common.)
I'm not sure what I'm asking for. I would like to be able to specify a revision, or number of outgoing patches, or something. But that feels a little weird in a tool trying to be hg/git-neutral. I'm mostly just describing what I see as a common use case that doesn't exactly fit the current options: I get lint errors on a phabricator rev, and want to fix just the problems introduced in that rev, possibly with multiple edit/lint cycles.
(Though honestly, it's possible that linting the full outgoing stack is fine, since hg absorb
has a good chance of distributing the fixes among patches anyway.)
Assignee | ||
Comment 8•5 years ago
|
||
I'll also note, for whatever it's worth, that my standard behavior for hg-related tools I'm writing is to use the changes in the working directory if any. If the working dir is clean, then use the top draft patch. But I don't think this is a generally accepted default?
But for a fast lint, --outgoing --workdir
is probably as good or better.
Comment 9•5 years ago
|
||
Maybe what you want is a --rev=<revset>
option which means lint all files touched by revisions in this revset. Then you could do something like --rev . --workdir
. Note I'm unlikely to have time to implement this anytime soon. If you wanted to take a shot at it, here's where the integration for --outgoing/--workdir happens:
https://searchfox.org/mozilla-central/rev/278046367dab878316f60f0bd7f740cf73f3c447/python/mozlint/mozlint/roller.py#262
We'd likely need to add features to mozversioncontrol
as well.
Fwiw I too often want to lint only a subset of my outgoing patches, but --outgoing
is usually fast enough anyway that I don't care if extra things are linted. So for me personally it hasn't been an issue (especially with hg absorb
).
Assignee | ||
Comment 10•5 years ago
|
||
In the process, fixed a few bugs:
- the template eg
{file_adds % "\n{file}"}
produced a leading blank line,
which led to everything being linted. - 'd' was replaced with 'r' in diff_filters, but the replacement was discarded.
- as a result of the above,
hg status -d
was being used ("show only deleted (but tracked) files") and now it ishg status -r
("show only removed files"). I think this is what was intended?
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/831af60ac6bd
https://hg.mozilla.org/mozilla-central/rev/8f82ceefe18a
Updated•2 years ago
|
Description
•