Closed Bug 1621322 Opened 5 years ago Closed 5 years ago

bare `mach lint --warnings` is mostly unusable due to rust clippy stuff

Categories

(Developer Infrastructure :: Lint and Formatting, task, P3)

Tracking

(firefox76 fixed)

RESOLVED FIXED
mozilla76
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?)

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?

(Edited: I messed up and ran on my fixed version, so it correctly found nothing.)

Sounds like there are a couple problems here..

  1. Submitting a Python-only patch to phabricator should not show any clippy results at all.
  2. 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 :).
  3. The clippy lint is very slow / resource intensive.
Type: task → defect
Priority: -- → P2

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.

Type: defect → task
Priority: P2 → P3

Bastien, could you update the message reviewbot displays to something like comment 1?

Flags: needinfo?(bastien)

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

Flags: needinfo?(bastien)

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.)

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.

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).

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 is hg status -r ("show only removed files"). I think this is what was intended?
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/831af60ac6bd mozversioncontrol: pre-existing bug fix, do not run git test under hg r=ahal https://hg.mozilla.org/integration/autoland/rev/8f82ceefe18a Implement `mach lint --revset=REV` r=ahal
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: