Closed Bug 1637655 Opened 4 years ago Closed 4 years ago

Don't run mozlint taks for beta, release or esr

Categories

(Developer Infrastructure :: Lint and Formatting, task)

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

For most of the patches landing in beta, release or esr, we will run mozlint on them.

However:

  • during the review phase, linters are executed and should have been fixed by developers
  • as the patches should land in autoland/m-c before being uplifted, they already had linting running on them
  • esr is by definition an older code base, m-c is in general more green than esr, we won't decrease the quality
  • we run older tools on esr
  • I don't remember if it ever produces anything interesting in an uplift

Are you intending on disabling all or some of them?

Have you considered about handling of bitrot situations or custom uplifts where the patch had to be written differently?

Are you intending on disabling all or some of them?

All for now but we could enable a few

Have you considered about handling of bitrot situations or custom uplifts where the patch had to be written differently?

Did that ever happen ? Feels like rhetorical for now

(In reply to Sylvestre Ledru [:Sylvestre] from comment #2)

Have you considered about handling of bitrot situations or custom uplifts where the patch had to be written differently?

Did that ever happen ? Feels like rhetorical for now

Ever? Empirically, yes. Is it common now? Unknown. Ryan?

Flags: needinfo?(ryanvm)

I've definitely seen ESR uplifts hit linting issues due to differing rules between the branches.

Flags: needinfo?(ryanvm)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)

I've definitely seen ESR uplifts hit linting issues due to differing rules between the branches.

That's an interesting data point, too. I was asking how common it was for people to have to custom-write patches for uplifts nowadays. :)

Flags: needinfo?(ryanvm)

It's late in the ESR cycle, most patches need some form of rebasing at this point. Entirely different patches? Pretty uncommon I'd say.

Flags: needinfo?(ryanvm)

I think my biggest concern would be for JavaScript code at least. It is easy to make trivial mistakes and for them not to be noticed, and test coverage isn't always 100%. c++ / rust are better, as they would at least catch things like unused/undefined variables.

I don't have any data on this relative to uplifts/esr though.

One thing that might also be viable, is to disable prettier (aka eslint formatting) on those branches to cut down the run time and reduce potential for back-outs/re-lands due to formatting only changes. We'd probably want to check the effective rules that get disabled, but I think you could get away with it.

yeah, it doesn't have to be black/white. We could keep eslint and maybe flake8

I've definitely seen ESR uplifts hit linting issues due to differing rules between the branches.

We have different kind of lintings. undefined variable is a big deal, missing "," or too many spaces is something else :)

We keep eslint and yaml on all projects as they could be usefull during uplift processes.

Attachment #9148295 - Attachment is obsolete: true

So, i have been bikeshedding here.
According to a dashboard, we are spending a very small amount on this for all repo (less than US$ 4k since the beginning of 2020).

So, if one day, a linter catch an actual issue in m-r or esr, the early catching of the error will be worth all the money we had spend for it.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
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: