With the discussion on firefox-dev about enabling linting tools in ci,
and the work the mozreview team has been doing to enable linting bots,
I've been doing some thinking about how we can turn linting into a
self-serve mechanism.

I want to build a system that holds a registry of linters, and knows how
and when to apply them to the proper files in the tree.

First, I'd like to state some goals:

1. It should be dead simple to add a new linter
2. New linters should not require any coding to implement
3. Linters should be as cheap as possible to run
4. It should be possible to add 3rd party linters
5. There should be fine grained control over when/where a linter runs
6. It should be easy to audit/disable linters
7. The process to run linters is the same, whether it be in ci, in
mozreview or on a developer's machine

Eventually, I envision tens or even hundreds of linters, scattered
throughout the tree. Some running globally, some running only against
certain file types, and some running only in certain subdirectories.
There will be a mach command that can run all the files touched by a
given changeset through all the appropriate linters. Some linters will
also be enabled by default in ci, or on mozreview.


This implementation is largely inspired by mach. There would be 3 parts:

1. A mozlint core (equivalent to mach core)
2. A lint registry (equivalent to mach_bootstrap.py)
3. The linters themselves (equivalent to mach_commands.py)


Linters
-------

It is a non-goal to have complicated and expensive linters that
do many different things at once. For this reason, a linter would be
nothing more than a well defined data structure and file type (e.g a
yaml file with a .lint extension).

Each linter would have some basic metadata (i.e name, description), a
type, a payload and a set of rules. Here are some examples:

* StringLinter - payload is a string, fails if the string exists
* RegexLinter - payload is a regex, fails if the regex matches
* ExternalLinter - payload is a command, fails if the command returns
non-zero

The "external" type would allow arbitrary complexity (e.g hooking in 3rd
party tools), but make it enough of an inconvenience that it would only
be used if absolutely necessary. The goal is to keep linters as small
and fast as possible

Finally each linter would have a set of rules. Rules are things like
"ext:js,jsm", "path:browser/*", etc. Rules can be combined with logical
operators (OR, AND, etc). They govern when a linter gets run against a
particular file. Both rules and types are well defined.


Registry
--------

The registry is how the core finds all the various linters in the tree.
It doesn't need to be complex, possibly just a path per line in a file.
Though other options include storing it in moz.build, a special config
file or a manifestparser manifest. The main takeaway here is that
multiple registries can exist. E.g, there can be one for mozreview, one
for ci and a third for the mach command. There could even be user
created registries for people who just want to enforce a rule in their
own code without pushing it onto everyone else.


Core
----

The mozlint core will define the allowed types and allowed rules. It
will fail to parse any linters that do not adhere to the definition.
Each linter type will be implemented here, and passed the payload and
the list of files as arguments. Not much else to say here, other than
it'll need to run the linters in parallel to make them as efficient as
possible.

The final missing pieces are the consumers of mozlint. The consumers are
what will determine which files need to get linted. E.g, the build
system may spit out a list of all .cpp files, or mercurial might spit
out a list of all files touched by a certain changeset, or the list of
files might be manually typed out on the commandline via a mach command.
Mozlint will not concern itself about how this list gets generated.

And that's about it. I'm going to start tinkering around with this over
the weekend and see where it goes. I'm sure there are a lot of things
that I'm not thinking about. All feedback, both positive and negative,
is much appreciated!

Cheers,
Andrew

On 28/11/15 00:35, Andrew Halberstadt wrote:
> Each linter would have some basic metadata (i.e name, description), a
> type, a payload and a set of rules. Here are some examples:
>
> * StringLinter - payload is a string, fails if the string exists
> * RegexLinter - payload is a regex, fails if the regex matches
> * ExternalLinter - payload is a command, fails if the command returns
> non-zero

Do we have enough examples of non-external linters that it's worth the
extra complexity to support them? The overhead of making people write an
extra python wrapper when they just want to run a regexp on a passed
path seems minimal.

> Finally each linter would have a set of rules. Rules are things like
> "ext:js,jsm", "path:browser/*", etc. Rules can be combined with logical
> operators (OR, AND, etc). They govern when a linter gets run against a
> particular file. Both rules and types are well defined.

Do you have use cases that require implementing full boolean expression
here rather than just assuming that multiple instances of the same rule
are always in union and different rules are always in intersection?
Doing things this way would allow just expressing the rules as a dict of
lists:

{path: ["browser/*"],
  ext: [".js", ".jsm"]}

> Registry
> --------
>
> The registry is how the core finds all the various linters in the tree.
> It doesn't need to be complex, possibly just a path per line in a file.
> Though other options include storing it in moz.build, a special config
> file or a manifestparser manifest. The main takeaway here is that
> multiple registries can exist. E.g, there can be one for mozreview, one
> for ci and a third for the mach command. There could even be user
> created registries for people who just want to enforce a rule in their
> own code without pushing it onto everyone else.
>
>
> Core
> ----
>
> The mozlint core will define the allowed types and allowed rules. It
> will fail to parse any linters that do not adhere to the definition.
> Each linter type will be implemented here, and passed the payload and
> the list of files as arguments. Not much else to say here, other than
> it'll need to run the linters in parallel to make them as efficient as
> possible.
>
> The final missing pieces are the consumers of mozlint. The consumers are
> what will determine which files need to get linted. E.g, the build
> system may spit out a list of all .cpp files, or mercurial might spit
> out a list of all files touched by a certain changeset, or the list of
> files might be manually typed out on the commandline via a mach command.
> Mozlint will not concern itself about how this list gets generated.

FWIW I think I have at least one use case where the lint doesn't need a
list of files because it computes this internally (this is the wpt lint).

My general feeling is that this whole design feels rather heavyweight,
but it's hard to know what's really needed without some concrete use
cases that have to be supported. What do we know that we need? For example:

* eslint. External program that (I assume) takes the path to a single js
file.

* web-platform-tests lint. External program that runs on all files under
the web-platform-tests root.

* pyflakes. Various modes of operation. Not sure what the best setup for
our use is.

I'm sure there are others. But I don't know what, exactly. For
comparison servo has [1] for linting, although their use cases are
rather different.

[1] https://github.com/servo/servo/blob/master/python/tidy.py

Good points, reply inline.

On 28/11/15 06:59 AM, James Graham wrote:
> Do we have enough examples of non-external linters that it's worth
> the extra complexity to support them? The overhead of making people
> write an extra python wrapper when they just want to run a regexp on
> a passed path seems minimal.

I think for some people, as soon as they discover they have to write a
python script, that right there is enough of a deterrent (even if it is
*really easy* python). I'd really like to make it as dead simple to
write as possible, even if it means some extra complexity in core.

Another benefit of doing it this way, is we can optimize the most common
use cases and avoid poorly written linters that slow everything down.

> Do you have use cases that require implementing full boolean
> expression here rather than just assuming that multiple instances of
> the same rule are always in union and different rules are always in
> intersection? Doing things this way would allow just expressing the
> rules as a dict of lists:
>
> {path: ["browser/*"], ext: [".js", ".jsm"]}

Good point, that would probably be good enough.

> FWIW I think I have at least one use case where the lint doesn't
> need a list of files because it computes this internally (this is the
> wpt lint).
>
> My general feeling is that this whole design feels rather
> heavyweight, but it's hard to know what's really needed without some
> concrete use cases that have to be supported.

Yeah, I'm not 100% sure how this will work with all the external tools.
For things like wptlint, maybe they just run the full set of files any
time a single one of them is an input.. or maybe we write a wrapper that
discards the output from irrelevant files. I don't really have an answer
here. I think we'll have to just try and implement some of them, see
what problems we run into and re-iterate on the design if necessary.

That being said, 3rd party linters are actually *not* the main reason I
want to write this system (there are already tons of well established
ways to run them). The main thing for me is providing the ability to do
customized micro-lints. If a particular team decides that they no longer
want to use a particular API, they can write a micro-lint that enforces
it only in their part of the code base. Linters aren't only for
style/syntax errors, they can be used for preventing the use of
deprecated code, or preventing known bad patterns that are stylistically
and syntactically correct. I think this is a powerful ability to have.

I'm likely going to focus on this use case first, and the ability to
hook in 3rd party linters will be a "nice to have" at first. Or maybe
down the road, we realize it isn't feasible and this system ignores 3rd
party linters altogether.

Thanks for the feedback!
-Andrew

On Saturday, November 28, 2015 at 12:36:00 AM UTC, Andrew Halberstadt wrote:
> First, I'd like to state some goals:
>
> 1. It should be dead simple to add a new linter
> 2. New linters should not require any coding to implement
> 3. Linters should be as cheap as possible to run
> 4. It should be possible to add 3rd party linters
> 5. There should be fine grained control over when/where a linter runs
> 6. It should be easy to audit/disable linters
> 7. The process to run linters is the same, whether it be in ci, in
> mozreview or on a developer's machine

I think there's some missing goals here, although I might be misunderstanding some of your intentions.

What I think we really need as developers is editor integration - review/checkin time is reasonable, but to actually be corrected/reminded when writing the code is what really speeds things up (less test/code/test cycles).

A lot of third party linters have editor integration already, and I think that's where we should be focusing on hooking into.

For instance, I don't know if you would plan on integrating eslint into the python wrappers. Lets say we did - I assume that would mean we wouldn't use its normal config files. At that stage we'd wouldn't be able to use the existing editor integrations, and end up writing those ourselves. Hence we'd end up writing and maintaining our own versions, which seems a little of a waste when there's already good options out there.

Mark.

On 28/11/15 01:49 PM, mba...@mozilla.com wrote:
> On Saturday, November 28, 2015 at 12:36:00 AM UTC, Andrew

> Halberstadt wrote: I think there's some missing goals here

That's very likely :)

> although I might be misunderstanding some of your intentions.

My main intention is to give developers an easy way of ensuring or
preventing a certain pattern from ever being landed. It doesn't
necessarily have to be style related.

For example, there was a post to dev.platform awhile back with the title
"Stop using PL_strchr - use mozilla::Tokenizer". Assuming that there
really is never a valid time to use PL_strchr, we could add a new linter
for it and immediately guarantee that it will never land in the codebase
again (if someone uses it, it would turn the "lint" job orange and get
backed out).

One could also imagine a linter that ensures all new files have a
license block. (I'm not saying this is necessarily a good idea, just an
example). These "micro-lints" are the main benefit of this project, and
while I'd like to also incorporate 3rd party linters, that's more of a
secondary goal.

The other benefit of this proposal, is that it acts as a registry of all
existing linters that have either been created, or pulled in from 3rd
party sources. Consumers like mozreview, taskcluster and mach will all
have access to the same set of linters, and be able to run them in a
unified way.

> What I think we really need as developers is editor integration -
> review/checkin time is reasonable, but to actually be
> corrected/reminded when writing the code is what really speeds
> things up (less test/code/test cycles).
>
> A lot of third party linters have editor integration already, and I
> think that's where we should be focusing on hooking into.

I guess an editor plugin could be a potential consumer of this library.
But I think editor integration is somewhat tangential to the problem I'm
trying to solve. But I do agree it's also something we should focus on.

> For instance, I don't know if you would plan on integrating eslint
> into the python wrappers. Lets say we did - I assume that would mean
> we wouldn't use its normal config files. At that stage we'd wouldn't
> be able to use the existing editor integrations, and end up writing
> those ourselves. Hence we'd end up writing and maintaining our own
> versions, which seems a little of a waste when there's already good
> options out there.
>
> Mark.

I disagree that wrapping eslint would mean not being able to use it's
normal config files though. My idea was that there would be an
"external" type that just runs a command in a subprocess. We could make
said command identical to the existing |mach eslint|.

-Andrew

On 28/11/15 16:39, Andrew Halberstadt wrote:
> Good points, reply inline.

> I'm likely going to focus on this use case first, and the ability to
> hook in 3rd party linters will be a "nice to have" at first. Or maybe
> down the road, we realize it isn't feasible and this system ignores 3rd
> party linters altogether.

Hmm, the third-party linters thing seems really important to me. Writing
code lints as regexp is always a hack compared to actually parsing the
file, so it seems like this restriction excludes the most
useful/correct/complex lints.

In somewhat related news, if there's an easy way to block commits on
lints at the moment, I don't know it; I would *love* for the wpt lint to
run whenever testing/web-platforms/tests is touched by a commit; I
currently live in fear that we will break the lint when merging a patch
upstream, but I don't know how to prevent the problem at source.

On 28/11/15 03:46 PM, James Graham wrote:
> On 28/11/15 16:39, Andrew Halberstadt wrote:
>> Good points, reply inline.
>
>> I'm likely going to focus on this use case first, and the ability
>> to hook in 3rd party linters will be a "nice to have" at first. Or
>> maybe down the road, we realize it isn't feasible and this system
>> ignores 3rd party linters altogether.
>
> Hmm, the third-party linters thing seems really important to me.
> Writing code lints as regexp is always a hack compared to actually
> parsing the file, so it seems like this restriction excludes the most
>  useful/correct/complex lints.

Yeah, I hope we can get 3rd party lints going with this system too. I
guess we'll see how it goes.

> In somewhat related news, if there's an easy way to block commits on
>  lints at the moment, I don't know it; I would *love* for the wpt
> lint to run whenever testing/web-platforms/tests is touched by a
> commit; I currently live in fear that we will break the lint when
> merging a patch upstream, but I don't know how to prevent the problem
> at source.

Not at the moment, but this is part of what I'm trying to enable. I plan
on making a taskcluster job that uses this library to run all enabled
linters per-checkin.

The "rules" portion of the wpt linter could make that specific linter
run whenever a wpt file gets touched.

On Sat, Nov 28, 2015, at 11:39 AM, Andrew Halberstadt wrote:
> The main thing for me is providing the ability to do
> customized micro-lints. If a particular team decides that they no longer
> want to use a particular API, they can write a micro-lint that enforces
> it only in their part of the code base.

I share James' concerns about hacky micro-lints.  It seems preferable
for us to use one extensible linter per language and for the build
support code to be so small and unglamorous that we don't need to give
it its own cool name.  For example, I don't think we would want our own
configuration files apart from the absolutely required build system
glue.  Most linters already have configuration files of their own and
these go hand-in-hand with in-editor integration (which I agree with
:mbanner is of utmost importance).

In the case of JS, eslint is the clear winner.  It has an AST and allows
plugins with minimal effort (ex:
https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml used by
FxOS gaia) and it has built-in performance analysis
(http://eslint.org/docs/developer-guide/working-with-rules.html#per-rule-performance)
that allows us to figure out if specific lints are slowing us down.  It
also already supports configuration files that can build on other
configuration files via relative paths
(http://eslint.org/docs/user-guide/configuring).

It should be a goal to forbid JS linters other than eslint for code that
is not a check-in of an external project with its own authoritative
linter.  So we still would run
https://github.com/w3c/web-platform-tests/blob/master/docs/lint-tool.md
for the web-platform-tests.  (I won't claim to understand the build
system sufficiently, but that linter really seems to just want to be
invoked like "./lint" in the wpt dir, so it does seem pretty simple for
the build system with additional wrapper code.)

I don't hack much on C/C++ right now, but it's my impression that
clang/LLVM family of tools are pretty cool.  Maybe clang-tidy or
something similar could similarly fill this role?  There seems to be
extensibility per
http://clang.llvm.org/extra/clang-tidy/#writing-a-clang-tidy-check.

And so on, for other languages.  Python, webidl, other?

Andrew

On 28/11/15 05:29 PM, Andrew Sutherland wrote:
> I share James' concerns about hacky micro-lints.  It seems preferable
> for us to use one extensible linter per language and for the build
> support code to be so small and unglamorous that we don't need to
> give it its own cool name.  For example, I don't think we would want
> our own configuration files apart from the absolutely required build
> system glue.  Most linters already have configuration files of their
> own and these go hand-in-hand with in-editor integration (which I
> agree with :mbanner is of utmost importance).

Yeah, tools that can parse the language should always be preferred and
used whenever possible. But I believe there is also a place for
micro-lints. They're low barrier, cheap and easy. They aren't gated on a
couple of people who know how to write a plugin, and they get the job
done for very specific things. Fwiw, Facebook has micro-lints and
(according to Legneato) are very useful.

I'm not personally concerned whatsoever about which lints get used. I'm
more concerned about empowering teams and people to easily add whatever
lints they deem most appropriate for their own use cases.

-Andrew

On Sun, Nov 29, 2015, at 12:59 AM, Andrew Halberstadt wrote:
> But I believe there is also a place for
> micro-lints. They're low barrier, cheap and easy.
 

> I'm not personally concerned whatsoever about which lints get used. I'm
> more concerned about empowering teams and people to easily add whatever
> lints they deem most appropriate for their own use cases.

I think we can make it easy to add lints to eslint/clang-tidy/whatever
without adding a second layer of linting infrastructure (mozlint) that
doesn't get the same immediate tooling benefits for free.

The examples I've seen so far are "Stop using PL_strchr - use
mozilla::Tokenizer" that you cited and David Bruant's citing of use
"Array.prototype.map given Array.map is non-standard and meant to
disappear".  I imagine a lot of other micro-lints would be similar
deprecation markers.  It seems like we could have an
eslint-plugin-deprecated-usage plugin and
clang-tidy-plugin-deprecated-usage that have an easy-to-modify list of
deprecated APIs.  So when a new API becomes deprecated, it just gets
added to the list.  (There may be plugins along these lines already; I
did a quick skim, but a deeper investigation is required.)

These could have the advantage that they're smart from the get go and
avoid false positive problems.  For example, in the event that
mozilla::Tokenizer gets deprecated, it would be far preferable if the
linter is smart and understands the namespaces/include chain so that
only mozilla::Tokenizer generates lint failures and
coolbeansparser::Tokenizer does not.

Most specifically, it seems like a bigger win to spend the effort on
integrating existing open source linter ecosystems into our build
process and making them easier to use than it is to write a
Mozilla-specific universal linter.  For example, goals like "New linters
should not require any coding to implement" could be met by adding a
mach command:
* mach add-lint --language js --deprecate Array.map --use-instead
Array.prototype.map --doc-link a-link-to-a-mailing-list-thread
* mach add-lint --language cpp --deprecate PL_strchr --use-instead
mozilla::Tokenizer --doc-link a-different-list-thread-link

And these would know what configuration file to edit.  And maybe even
use any mach commands to help create a bug and upload the patch and all
that.

Andrew

On 29/11/15 07:58 PM, Andrew Sutherland wrote:
> I think we can make it easy to add lints to
> eslint/clang-tidy/whatever without adding a second layer of linting
> infrastructure (mozlint) that doesn't get the same immediate tooling
> benefits for free.

I still hope for eslint/clang-tidy/what-have-you to be easy to hook up
and used whenever possible. And those linters will still benefit from
this proposal in that tools like mach/mozreview/taskcluster will be able
to find and run them in the same way.

But those tools won't cover Makefiles/moz.build/configs/xul/etc.. And
there are many languages that are so insignificant in our code base (e.g
java, perl), that it isn't worth going through any effort to provide
linting support. Micro-lints provide a useful workaround for all these
cases.

> The examples I've seen so far are "Stop using PL_strchr - use
> mozilla::Tokenizer" that you cited and David Bruant's citing of use
> "Array.prototype.map given Array.map is non-standard and meant to
> disappear".  I imagine a lot of other micro-lints would be similar
> deprecation markers.  It seems like we could have an
> eslint-plugin-deprecated-usage plugin and
> clang-tidy-plugin-deprecated-usage that have an easy-to-modify list
> of deprecated APIs.  So when a new API becomes deprecated, it just
> gets added to the list.  (There may be plugins along these lines
> already; I did a quick skim, but a deeper investigation is
> required.)

If implemented properly, this proposal can actually cover that case. I
mentioned the notion of "types" in my original post. I could imagine an
"eslint-deprecation" type where the payload is a deprecation rule.
Mozlint would then be able to automatically hook that into our custom
"eslint-plugin-deprecated-usage" module.

> These could have the advantage that they're smart from the get go
> and avoid false positive problems.  For example, in the event that
> mozilla::Tokenizer gets deprecated, it would be far preferable if
> the linter is smart and understands the namespaces/include chain so
> that only mozilla::Tokenizer generates lint failures and
> coolbeansparser::Tokenizer does not.

Valid point. Though I'd expect a micro-lint that had any possibility of
a false positive to be fixed ASAP or removed altogether. I'd also expect
a micro-lint that can easily be replaced by an eslint rule to be
promptly r-'ed.

> Most specifically, it seems like a bigger win to spend the effort on
> integrating existing open source linter ecosystems into our build
> process and making them easier to use than it is to write a
> Mozilla-specific universal linter.

I think the effort/complexity of this proposal is being blown out of
proportion. I already have a prototype with working micro-lints that run
in parallel, and it's currently no more than 200 lines of code. In fact,
the reason I'm tackling them first is *because* they are so easy to get
going. (This is a spare time project, linting has nothing to do with any
of my goals or deliverables).

Besides, I don't want a Mozilla-specific universal linter. I want a
democratically federated list of linters that vary from one source dir
to another.

> For example, goals like "New linters should not require any coding to
> implement" could be met by adding a mach command: * mach add-lint
> --language js --deprecate Array.map --use-instead Array.prototype.map
> --doc-link a-link-to-a-mailing-list-thread * mach add-lint --language
> cpp --deprecate PL_strchr --use-instead mozilla::Tokenizer --doc-link
> a-different-list-thread-link
>
> And these would know what configuration file to edit.  And maybe
> even use any mach commands to help create a bug and upload the patch
> and all that.

I believe |mach add-lint| is more or less what I'm proposing here. For
every mach command, there is an underlying library that implements all
the magic. Some sort of library has to hold the registry, micro-lints or
not. This will be that library.

It seems to me like we're both arguing for many of the same things. The
only disagreement seems to be that you think micro-lints are worse than
no micro-lints, while I think they're useful in addition to more
intelligent ones.

Thanks for the feedback btw, I appreciate it. It's good to get this
perspective early on, it's not something I would have thought of.

-Andrew

On Sun, Nov 29, 2015, at 08:40 PM, Andrew Halberstadt wrote:
> I believe |mach add-lint| is more or less what I'm proposing here. For
> every mach command, there is an underlying library that implements all
> the magic. Some sort of library has to hold the registry, micro-lints or
> not. This will be that library.

If people writing JS code never have to be aware of mozlint, then
indeed, I think we're golden.  But if adding a JS linting rule means
learning a mozlint configuration file, or means that a lint error can
occur from a try run but wasn't found when the developer's editor ran
eslint, then not golden.

I'm on board for naive linters against languages where we don't have a
better solution as long as they are the only solution we provide for
that language/file type.

Feel free to skip everything below here if we're already golden:

My concern in general derives from developers in general already being a
bit FUDdy on linters and contributor fatigue from having to deal with
Mozilla-specific stuff that doesn't need to be Mozilla-specific.  On
FxOS Gaia, we adopted jslint, then jshint, and are currently migrating
to eslint.  A common attitude was that the earlier jslint/jshint lint
errors were nuisances, with most things just being syntactic nits or
otherwise harmless.  This has led to instances of them being ignored,
with real fancy eslint errors on important security checks ignored and
checked into the tree via the expected failure mechanism.

> I think the effort/complexity of this proposal is being blown out of
> proportion. I already have a prototype with working micro-lints that run
> in parallel, and it's currently no more than 200 lines of code. In fact,
> the reason I'm tackling them first is *because* they are so easy to get
> going. (This is a spare time project, linting has nothing to do with any
> of my goals or deliverables).

I'm not concerned so much about the complexity of the implementation
itself so much as the cognitive burden of its existence for developers
and the possibility of "there's N ways to do X" when there really only
needs to be 1.

Especially if the naive implementation is fundamentally unsound but need
not be.  For example, I expect JS template literals (which allow
embedding a series of JS expressions of arbitrary complexity and so
fundamentally exceed the capability of regular expressions to match)
would allow one to construct either false positives or false negatives
for any JS linter that does not include a full parse of the source file.
 Given how easy eslint already makes it to avoid false positives/false
negatives in this case and that there are ways that we can make it extra
easy to configure eslint rules, introducing an unsound mechanism seems
inadvisable since it will contribute to an attitude that the linters can
and should be ignored.

In effect, I assert that allowing developers to add unnecessarily
unsound lints to the tree so that they can avoid learning eslint, the
dominant linter for JS (and therefore a useful thing to learn), creates
a tragedy of the commons that hurts everyone.  If it's important to stop
people from using certain code idioms, it's important to do it in a
sound way that rules out known classes of false positives/negatives and
keeps people trusting the linters.

> It seems to me like we're both arguing for many of the same things. The
> only disagreement seems to be that you think micro-lints are worse than
> no micro-lints, while I think they're useful in addition to more
> intelligent ones.

Yes.  But I do believe it's possible for extra tooling to be a hindrance
even when it is indeed an improvement in some regards.  For example,
arguably the creation of the node-based
https://www.npmjs.com/package/mozilla-download when we already had
https://pypi.python.org/pypi/mozdownload was not helping things.  I know
I certainly was frustrated when mozilla-download broke for me several
times and I had to investigate its implementation and provide fixes,
whereas presumably if we had been using the more aggressively supported
mozdownload I never would have encountered the problems or could have
poked an existing/active maintainer or the existing contributor-base.

Andrew

Hello all,
I'd like to chime in as a partial representative for the Fennec front-end team.  I hope Mike Comella will also comment, since he's actually done work in this area: https://bugzilla.mozilla.org/show_bug.cgi?id=939350, https://bugzilla.mozilla.org/show_bug.cgi?id=1170804, etc.

I want to support a few points others have raised:

* making it possible to run third-party linters in automation provides 99% of the value;
* running existing third-party linters is more important than doing anything else;
* Mozilla-specific is destined to rot.

If all that comes of this is a way to run "gradle lint" in automation that doesn't require release engineering to be involved [1], Fennec team will consider it a huge win.

Nick

[1] I'm aware that this is supposed to be possible.  Mike and I have tried to achieve this and failed.  We don't have infinite time and energy to make this happen.

I'm having a few constructive comments, also some sceptical.

I have a general concern, in that a system like this could make it
really hard to understand what's "code that is OK".

I also think that rewording this project might help. Instead of calling
it "dozens or hundreds of linters", we might call it "one or two
linters, with many decentralized configurations".

As someone writing editor plugins for his own linter, here are a few
mistakes to not repeat:

- create an optional json output, that doesn't have any constructed
messages. In particular, "bad match", "good replacement", and source
position need to be explicit fields.

Which is one item of a few mistakes I've made.

Another point is that I'd create a linter that rejects linter
configurations that don't have a constructive alternative.
This is in particular important to make our code base more accessible.
If your linter-config only has a "no", it's just alienating people. (I'm
talking to you, pep8-underindented-line).
Your linter config should basically give everything you need to rewrite
the code fragment in question. And do so in a way that
editor-integration doesn't need to parse stdout, beyond a json().

I'd drop the idea of starting commands as part of a linting process.
That's slow, scales really badly, and all around is tough to get performant.
I think that linters of this kind really want to be in a different
infrastructure, being launched once, and given a set of files.

Axel

On 29/11/15 09:39 PM, Andrew Sutherland wrote:
> If people writing JS code never have to be aware of mozlint, then
> indeed, I think we're golden.  But if adding a JS linting rule means
> learning a mozlint configuration file, or means that a lint error can
> occur from a try run but wasn't found when the developer's editor ran
> eslint, then not golden.

I think we're golden. In this proposal, one of the "mozlints" would be
the equivalent of running |mach eslint|. So to change the rules, you'd
edit the same .eslint configuration files that would normally affect
|mach eslint|.

> In effect, I assert that allowing developers to add unnecessarily
> unsound lints to the tree so that they can avoid learning eslint, the
> dominant linter for JS (and therefore a useful thing to learn), creates
> a tragedy of the commons that hurts everyone.  If it's important to stop
> people from using certain code idioms, it's important to do it in a
> sound way that rules out known classes of false positives/negatives and
> keeps people trusting the linters.

I hear your concern. I guess this seems like it should be a policy
decision, rather than a technical one. From an automation tools
perspective, we get the biggest wins when we give developers a solid
foundation, and let them build their own tools and policies on top of it.

In other words what you say makes sense, and I agree with it.. And if
everyone else also agrees with you, that should be the policy. But I'd
like to give module owners and teams the opportunity to make their own call.

I sort of think of a linter as a test. Sometimes bad tests get checked
in that cause intermittents and false positives. Such tests normally get
backed out or disabled if they aren't fixed in a timely manner. And it's
up to module owners/teams to set policy on quality and how thoroughly
they get reviewed.

> Yes.  But I do believe it's possible for extra tooling to be a hindrance
> even when it is indeed an improvement in some regards.  For example,
> arguably the creation of the node-based
> https://www.npmjs.com/package/mozilla-download when we already had
> https://pypi.python.org/pypi/mozdownload was not helping things.  I know
> I certainly was frustrated when mozilla-download broke for me several
> times and I had to investigate its implementation and provide fixes,
> whereas presumably if we had been using the more aggressively supported
> mozdownload I never would have encountered the problems or could have
> poked an existing/active maintainer or the existing contributor-base.

I could argue that this proposal is trying to avoid that situation.
Exactly what I don't want, is for everyone to go off and implement their
own linters in their own special-snowflake way. Then it will be on our
team to try and corral them after the fact. I want to build the corral
before it gets out of hand.

On 30/11/15 07:18 AM, Axel Hecht wrote:
> I'm having a few constructive comments, also some sceptical.
>
> I have a general concern, in that a system like this could make it
> really hard to understand what's "code that is OK".

This will be a bit tricky. I think where linters are configured to run
will determine what is "ok" and what isn't. In my mind, there are the
following levels:

1. editor/mach - suggested, but not required
2. mozreview - strongly suggested, but possible to get around it
3. taskcluster/ci - must be adhered to, no exceptions

For a linter to run in level 3, it should be a hard requirement that
it's also enabled in both level 2 and level 1.

> I also think that rewording this project might help. Instead of
> calling it "dozens or hundreds of linters", we might call it "one or
>  two linters, with many decentralized configurations".

Yeah, that makes sense.

> As someone writing editor plugins for his own linter, here are a few
>  mistakes to not repeat:
>
> - create an optional json output, that doesn't have any constructed
> messages. In particular, "bad match", "good replacement", and source
>  position need to be explicit fields.

Good idea. I haven't gotten around to thinking about output much yet,
but I like the idea of requiring a "replacement hint". And yes, I agree
source position is a must.

> I'd drop the idea of starting commands as part of a linting process.
>  That's slow, scales really badly, and all around is tough to get
> performant. I think that linters of this kind really want to be in a
>  different infrastructure, being launched once, and given a set of
> files.

I agree, we don't want to launch a subprocess per file, that would be
really expensive. I was planning for the 'external' type to be launched
only once, with all files being passed in as a batch. Put another way,
|mach eslint| also launches a subprocess. This wouldn't be any different.

Thanks for the feedback!
-Andrew

On Sun, Nov 29, 2015 at 11:38 PM, <ncale...@gmail.com> wrote:

> Hello all,
>

> I'd like to chime in as a partial representative for the Fennec front-end
> team.  I hope Mike Comella will also comment, since he's actually done work
> in this area: https://bugzilla.mozilla.org/show_bug.cgi?id=939350,
> https://bugzilla.mozilla.org/show_bug.cgi?id=1170804, etc.
>
> I want to support a few points others have raised:
>
> * making it possible to run third-party linters in automation provides 99%
> of the value;
> * running existing third-party linters is more important than doing
> anything else;
> * Mozilla-specific is destined to rot.
>
> If all that comes of this is a way to run "gradle lint" in automation that
> doesn't require release engineering to be involved [1], Fennec team will
> consider it a huge win.
>
> Nick
>
> [1] I'm aware that this is supposed to be possible.  Mike and I have tried
> to achieve this and failed.  We don't have infinite time and energy to make
> this happen.
>

FWIW, I consider it an explicit goal to get to this point. Short term may
be that you need help integrating a linter but long term this should be
self service.

Lawrence

On Mon, Nov 30, 2015 at 9:30 AM, Andrew Halberstadt <
ahalbe...@mozilla.com> wrote:

> On 30/11/15 07:18 AM, Axel Hecht wrote:
>
>> I'm having a few constructive comments, also some sceptical.
>>
>> I have a general concern, in that a system like this could make it
>> really hard to understand what's "code that is OK".
>>
>
> This will be a bit tricky. I think where linters are configured to run
> will determine what is "ok" and what isn't. In my mind, there are the
> following levels:
>
> 1. editor/mach - suggested, but not required
> 2. mozreview - strongly suggested, but possible to get around it
> 3. taskcluster/ci - must be adhered to, no exceptions
>
> For a linter to run in level 3, it should be a hard requirement that
> it's also enabled in both level 2 and level 1.
>

Yes. And it should fail (as opposed to suggest) in those envs as well. We
want to identify patches that are going to bounce early. Preferably these
patches never land in the first place.

Lawrence

>
>
> I also think that rewording this project might help. Instead of
>> calling it "dozens or hundreds of linters", we might call it "one or
>>  two linters, with many decentralized configurations".
>>
>
> Yeah, that makes sense.
>
>
> As someone writing editor plugins for his own linter, here are a few
>>  mistakes to not repeat:
>>
>> - create an optional json output, that doesn't have any constructed
>> messages. In particular, "bad match", "good replacement", and source
>>  position need to be explicit fields.
>>
>
> Good idea. I haven't gotten around to thinking about output much yet,
> but I like the idea of requiring a "replacement hint". And yes, I agree
> source position is a must.
>
>
> I'd drop the idea of starting commands as part of a linting process.
>>  That's slow, scales really badly, and all around is tough to get
>> performant. I think that linters of this kind really want to be in a
>>  different infrastructure, being launched once, and given a set of
>> files.
>>
>
> I agree, we don't want to launch a subprocess per file, that would be
> really expensive. I was planning for the 'external' type to be launched
> only once, with all files being passed in as a batch. Put another way,
> |mach eslint| also launches a subprocess. This wouldn't be any different.
>
> Thanks for the feedback!
> -Andrew
>
>

>>> Cheers, Andrew
>>>
>>
>>
> _______________________________________________
> tools mailing list
> to...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/tools
>

First, giant +1.

Second, I only skimmed this thread, so apologies if I'm being redundant
with others.

Random thoughts...

As with everything, linter definitions and ideally the linter tools and
related code themselves would be checked into mozilla-central. Or at least
versions are pinned from tree. This makes everything much easier to run and
reproduce over time (which is important for things like linting beta
differently from central so you aren't chasing failures on beta due to
changes in the tool). We also want developers and linters in automation
(including MozReview) to have identical, deterministic behavior.

moz.build files should provide a sufficient platform for declaring linters.
The Files() context manager (
https://gecko.readthedocs.org/en/latest/build/buildsystem/files-metadata.html)
can be used. It was always my intent to somehow declare linters and their
policies from Files(). We may, however, want a universal file (like
build/templates.mozbuild) that declares a global set of linter policies so
they can be universally updated (hello universal code style) and easily
referenced by name. A benefit of Files() is that we can read the data
directly from hg.mozilla.org so random automation can examine a changeset
without having to check it out. See
https://hg.mozilla.org/mozilla-central/json-mozbuildinfo/a18630f9ab42.
Another benefit of moz.build is it is Python and you can do some crazy
operator overload foo to get the rules DSL you proposed.

You may want to look at and/or revive bug 939350.

We're trending towards a mach command for submitting patches for review. I
would absolutely love for that command to be able to execute cheap linters
before code hits the review tool: no sense submitting bad code if it can be
detected. We also want cheap linters to run as a commit hook or when saving
files in your editor. Also, MozReview would probably redundantly run all
linters just in case. Takeaway is there needs to be an API on linters for
matching results up to files and line numbers so MozReview, IDEs, etc can
report failures nicely.

Not all linters are cheap, so there will be linters that only automation
can realistically perform.

As for a registry of linters, I'd use the filesystem. Create a directory
with 1 file per linter. Files could be Python modules declaring linters
conforming to a base class/interface. Files could be JSON or YAML declaring
linters from simple types (e.g. regexp based linters), where a full Python
implementation isn't needed. Your registry is a Python class that looks at
and loads each file in a directory depending on the file type. A mach
command could create a new linter by writing a file from a base template.

Hey there,

Kind of late to the game but may I suggest adopting and already established format: https://github.com/epriestley/arclint-examples/blob/master/.arclint

`.arclint` file is used by Arcanist, the command line tool for Phabricator. It has been working for us quite well. We may choose to extend it too.

Thanks for the pointer! I was looking for something like this to use as
a guideline. Here's the source:
https://github.com/phacility/arcanist/tree/master/src/lint

And here's some documentation:
https://secure.phabricator.com/book/phabricator/article/arcanist_lint/

While I don't know if I want to copy that exactly, it will definitely be
useful for inspiration.

-Andrew

On Tue, Dec 1, 2015 at 5:28 PM, BYK <mad...@gmail.com> wrote:

> Hey there,
>
> Kind of late to the game but may I suggest adopting and already
> established format:
> https://github.com/epriestley/arclint-examples/blob/master/.arclint
>
> `.arclint` file is used by Arcanist, the command line tool for
> Phabricator. It has been working for us quite well. We may choose to extend
> it too.
>
>

We already have the logic in moz.build files for dealing with policy
inheritance, extracting the metadata directly from hg.mozilla.org, etc. I'd
rather build on top of that. It's not like we'll be able to reuse much code
for Phabricator anyway, since I'm assuming it's all in PHP.

It's more about being compatible with the tools already out there instead
of reusing code. Also we may be able to reuse the logic even if it is PHP.
> --
BYK

On 02/12/15 02:53 PM, Burak Yiğit Kaya wrote:
> It's more about being compatible with the tools already out there
> instead of reusing code. Also we may be able to reuse the logic even
> if it is PHP.

It looks like arc lint has a centralized repository for linters (aka,
they're all implemented inside the arc lint package). I'd like more of a
mach approach that offers a distributed registry-like system.

I think trying to maintain compatibility with it would likely slow us
down for negligible benefit. That's not to say we can't borrow concepts
from it though, so thanks for pointing it out!

-Andrew

On 2015-12-02 9:59 AM, Andrew Halberstadt wrote:

> On 01/12/15 08:28 PM, BYK wrote:
>> Hey there,
>>
>> Kind of late to the game but may I suggest adopting and already
>> established format:
>> https://github.com/epriestley/arclint-examples/blob/master/.arclint
>>
>> `.arclint` file is used by Arcanist, the command line tool for
>> Phabricator. It has been working for us quite well. We may choose to
>> extend it too.
>

> Thanks for the pointer! I was looking for something like this to use as
> a guideline. Here's the source:
> https://github.com/phacility/arcanist/tree/master/src/lint
>
> And here's some documentation:
> https://secure.phabricator.com/book/phabricator/article/arcanist_lint/
>
> While I don't know if I want to copy that exactly, it will definitely be
> useful for inspiration.

Wait, what's specifically wrong with it?  This looks like a great tool
that is already built and has happy users.  Why not try using it?  It
looks like a good cross langauge linter based on a quick read over its docs.

On 2015-12-02 1:49 PM, Gregory Szorc wrote:
 > On Tue, Dec 1, 2015 at 5:28 PM, BYK <mad...@gmail.com> wrote:
 >
 > We already have the logic in moz.build files for dealing with policy
 > inheritance, extracting the metadata directly from hg.mozilla.org,
etc. I'd
 > rather build on top of that.

I think the suggestion here is only to use the `arc lint` command, not
the rest of arc.  The .arclint format is very simple, maintaining the
same information is similar to .hgrc/.gitconfig in terms of difficulty.
  I don't think having the same information in a different format
(moz.build for example) is that much better or worse.  .arclint is more
readable thought since it captures all linting information in the same
place.

 > It's not like we'll be able to reuse much code
 > for Phabricator anyway, since I'm assuming it's all in PHP.

I find the argument against PHP very weak since we don't need to modify
the tool, and it seems like an eslint linter is being contributed:
<https://github.com/phacility/arcanist/pull/200>.

I can help where PHP knowledge is needed (for example, for upstreaming
an eslint backend.)  :-)

Cheers,
Ehsan

On Wed, Dec 2, 2015 at 8:32 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> On 2015-12-02 9:59 AM, Andrew Halberstadt wrote:
>
>> On 01/12/15 08:28 PM, BYK wrote:
>>
>>> Hey there,
>>>
>>> Kind of late to the game but may I suggest adopting and already
>>> established format:
>>> https://github.com/epriestley/arclint-examples/blob/master/.arclint
>>>
>>> `.arclint` file is used by Arcanist, the command line tool for
>>> Phabricator. It has been working for us quite well. We may choose to
>>> extend it too.
>>>
>>
>> Thanks for the pointer! I was looking for something like this to use as
>> a guideline. Here's the source:
>> https://github.com/phacility/arcanist/tree/master/src/lint
>>
>> And here's some documentation:
>> https://secure.phabricator.com/book/phabricator/article/arcanist_lint/
>>
>> While I don't know if I want to copy that exactly, it will definitely be
>> useful for inspiration.
>>
>
> Wait, what's specifically wrong with it?  This looks like a great tool
> that is already built and has happy users.  Why not try using it?  It looks
> like a good cross langauge linter based on a quick read over its docs.
>

This is a good point. The goal is to enable linting not to write a new
linting framework. I don't know the details of this specific tool but if
there is a tool that is close to what we need and gets us to where we need
to go that is something we should pursue.

Lawrence


>
> On 2015-12-02 1:49 PM, Gregory Szorc wrote:
> > On Tue, Dec 1, 2015 at 5:28 PM, BYK <mad...@gmail.com> wrote:
> >
> > We already have the logic in moz.build files for dealing with policy
> > inheritance, extracting the metadata directly from hg.mozilla.org, etc.
> I'd
> > rather build on top of that.
>
> I think the suggestion here is only to use the `arc lint` command, not the
> rest of arc.  The .arclint format is very simple, maintaining the same
> information is similar to .hgrc/.gitconfig in terms of difficulty.  I don't
> think having the same information in a different format (moz.build for
> example) is that much better or worse.  .arclint is more readable thought
> since it captures all linting information in the same place.
>
> > It's not like we'll be able to reuse much code
> > for Phabricator anyway, since I'm assuming it's all in PHP.
>
> I find the argument against PHP very weak since we don't need to modify
> the tool, and it seems like an eslint linter is being contributed: <
> https://github.com/phacility/arcanist/pull/200>.
>
> I can help where PHP knowledge is needed (for example, for upstreaming an
> eslint backend.)  :-)
>
> Cheers,
> Ehsan
>
> _______________________________________________
> tools mailing list
> to...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/tools
>

On 02/12/15 08:32 PM, Ehsan Akhgari wrote:
> On 2015-12-02 9:59 AM, Andrew Halberstadt wrote:
>> On 01/12/15 08:28 PM, BYK wrote:
>>> Hey there,
>>>
>>> Kind of late to the game but may I suggest adopting and already
>>> established format:
>>> https://github.com/epriestley/arclint-examples/blob/master/.arclint
>>>
>>>
>>>
>>>
>>>
>>>
`.arclint` file is used by Arcanist, the command line tool for
>>> Phabricator. It has been working for us quite well. We may
>>> choose to extend it too.
>>
>> Thanks for the pointer! I was looking for something like this to
>> use as a guideline. Here's the source:
>> https://github.com/phacility/arcanist/tree/master/src/lint
>>
>> And here's some documentation:
>> https://secure.phabricator.com/book/phabricator/article/arcanist_lint/
>>
>>
>>
>>
>>
>>
While I don't know if I want to copy that exactly, it will definitely be
>> useful for inspiration.
>
> Wait, what's specifically wrong with it?  This looks like a great
> tool that is already built and has happy users.  Why not try using
> it?  It looks like a good cross langauge linter based on a quick
> read over its docs.
>

> On 2015-12-02 1:49 PM, Gregory Szorc wrote:
>> On Tue, Dec 1, 2015 at 5:28 PM, BYK <mad...@gmail.com> wrote:
>>
>> We already have the logic in moz.build files for dealing with
>> policy inheritance, extracting the metadata directly from
>> hg.mozilla.org,
> etc. I'd
>> rather build on top of that.
>
> I think the suggestion here is only to use the `arc lint` command,
> not the rest of arc.  The .arclint format is very simple,
> maintaining the same information is similar to .hgrc/.gitconfig in
> terms of difficulty. I don't think having the same information in a
> different format (moz.build for example) is that much better or
> worse. .arclint is more readable thought since it captures all
> linting information in the same place.
>
>> It's not like we'll be able to reuse much code for Phabricator
>> anyway, since I'm assuming it's all in PHP.
>
> I find the argument against PHP very weak since we don't need to
> modify the tool, and it seems like an eslint linter is being
> contributed: <https://github.com/phacility/arcanist/pull/200>.
>
> I can help where PHP knowledge is needed (for example, for
> upstreaming an eslint backend.)  :-)
>
> Cheers, Ehsan

You're right, I was dismissing it too quickly. It's certainly worth some
extra thought.

But after giving it some extra thought, I'm still not convinced. While
PHP may be a weak argument, there is still a cost to it. There's an
integration cost (into an almost entirely python based ecosystem) and a
maintenance cost (Engineering Productivity, who I assume will be
maintaining/improving this, has little to no PHP experience). I also
disagree that we won't need to modify the tool, sooner or later our
needs will diverge from phabricator's needs.

I'd be more inclined to use it if it were solving a complex problem, but
it's not. The complexity is already tucked away within all the
individual linters. Fwiw, I already have a working eslint backend in my
prototype (not ready for use, but the basics are there).

At the end of the day, for me personally integrating arclint seems like
at least as big of a task as writing mozlint. The difference being the
latter is something exciting enough for me to volunteer my time on,
while the former isn't. Sorry, I know this sounds a lot like "not
invented here" syndrome, and that's probably true to some degree.

Thanks for calling this out though. I guess if there is overwhelming
consensus that using arclint is better, and someone other than me was
willing to do the work, I could drop mozlint.

-Andrew

On Wed, Dec 2, 2015 at 7:04 PM, Lawrence Mandel <lma...@mozilla.com> wrote:

> On Wed, Dec 2, 2015 at 8:32 PM, Ehsan Akhgari <ehsan....@gmail.com>

> wrote:
>
>> On 2015-12-02 9:59 AM, Andrew Halberstadt wrote:
>>
>>> On 01/12/15 08:28 PM, BYK wrote:
>>>
>>>> Hey there,
>>>>
>>>> Kind of late to the game but may I suggest adopting and already
>>>> established format:
>>>> https://github.com/epriestley/arclint-examples/blob/master/.arclint
>>>>
>>>> `.arclint` file is used by Arcanist, the command line tool for
>>>> Phabricator. It has been working for us quite well. We may choose to
>>>> extend it too.
>>>>
>>>
>>> Thanks for the pointer! I was looking for something like this to use as
>>> a guideline. Here's the source:
>>> https://github.com/phacility/arcanist/tree/master/src/lint
>>>
>>> And here's some documentation:
>>> https://secure.phabricator.com/book/phabricator/article/arcanist_lint/
>>>
>>> While I don't know if I want to copy that exactly, it will definitely be
>>> useful for inspiration.
>>>
>>
>> Wait, what's specifically wrong with it?  This looks like a great tool
>> that is already built and has happy users.  Why not try using it?  It looks
>> like a good cross langauge linter based on a quick read over its docs.
>>
>

> This is a good point. The goal is to enable linting not to write a new
> linting framework. I don't know the details of this specific tool but if
> there is a tool that is close to what we need and gets us to where we need
> to go that is something we should pursue.
>
> Lawrence
>
>
>>

>> On 2015-12-02 1:49 PM, Gregory Szorc wrote:
>> > On Tue, Dec 1, 2015 at 5:28 PM, BYK <mad...@gmail.com> wrote:
>> >
>> > We already have the logic in moz.build files for dealing with policy
>> > inheritance, extracting the metadata directly from hg.mozilla.org,
>> etc. I'd
>> > rather build on top of that.
>>
>> I think the suggestion here is only to use the `arc lint` command, not
>> the rest of arc.  The .arclint format is very simple, maintaining the same
>> information is similar to .hgrc/.gitconfig in terms of difficulty.  I don't
>> think having the same information in a different format (moz.build for
>> example) is that much better or worse.  .arclint is more readable thought
>> since it captures all linting information in the same place.
>>
>> > It's not like we'll be able to reuse much code
>> > for Phabricator anyway, since I'm assuming it's all in PHP.
>>
>> I find the argument against PHP very weak since we don't need to modify
>> the tool, and it seems like an eslint linter is being contributed: <
>> https://github.com/phacility/arcanist/pull/200>.
>>
>> I can help where PHP knowledge is needed (for example, for upstreaming an
>> eslint backend.)  :-)
>>
>

I don't like reinventing wheels.

Linting frameworks are essentially:

1) An interface that knows how to run a linter on a file (or section of
file) and turn it into data structures of results
2) A mechanism for invoking linter implementations
3) The linters themselves

Since most linters are standalone tools, invoking them is typically rather
boilerplate code of "run process, parse output." That can be implemented in
any language rather easily. So, designing a framework that launches a bunch
of linters and collects output really isn't that big of a deal.

If we used Arcanist as the main linting tool, we'd be introducing a new PHP
dependency into our tools and all that entails (there's a lot of hidden
work and complications - see all the pain that integrating Node.js has
been). For integration with mach, MozReview, etc, we'd need to write a
Python interface to the linter framework. By the time you've done all this,
you were probably better off just implementing the basic linting framework
in Python from the beginning. If it turns out Arcanist has some PHP based
linters that are really awesome, we can have our home-grown linter
framework call out into those, just like they launch Node and other random
executables as part of running other linters.

The concern about reinventing wheels is valid. But this is a small wheel
and not worth worrying over.

On 30/11/15 04:06 PM, Gregory Szorc wrote:
> moz.build files should provide a sufficient platform for declaring linters.
> The Files() context manager (
> https://gecko.readthedocs.org/en/latest/build/buildsystem/files-metadata.html)
> can be used. It was always my intent to somehow declare linters and their
> policies from Files(). We may, however, want a universal file (like
> build/templates.mozbuild) that declares a global set of linter policies so
> they can be universally updated (hello universal code style) and easily
> referenced by name. A benefit of Files() is that we can read the data
> directly from hg.mozilla.org so random automation can examine a changeset
> without having to check it out. See
> https://hg.mozilla.org/mozilla-central/json-mozbuildinfo/a18630f9ab42.
> Another benefit of moz.build is it is Python and you can do some crazy
> operator overload foo to get the rules DSL you proposed.

I'm a bit confused by what you're proposing. Are you proposing to use
Files() to store the actual definition/metadata of a linter? If so,
what's the benefit of Files over keeping the metadata inside the linter
itself?

I was thinking of using moz.build simply for gathering a list of linters
from the tree. A linter equivalent to TEST_HARNESS_FILES. From reading
the Files docs, I think this is a separate issue from the one you
describe above, and Files won't help me. Is that correct?

For anyone interested in following along, I filed:
https://bugzilla.mozilla.org/show_bug.cgi?id=1230962

Please keep in mind the patch there is a *very early* prototype.

On 27/11/15 07:35 PM, Andrew Halberstadt wrote:
> With the discussion on firefox-dev about enabling linting tools in ci,
> and the work the mozreview team has been doing to enable linting bots,
> I've been doing some thinking about how we can turn linting into a
> self-serve mechanism.
>
> I want to build a system that holds a registry of linters, and knows how
> and when to apply them to the proper files in the tree.
>
> First, I'd like to state some goals:
>
> 1. It should be dead simple to add a new linter
> 2. New linters should not require any coding to implement
> 3. Linters should be as cheap as possible to run
> 4. It should be possible to add 3rd party linters
> 5. There should be fine grained control over when/where a linter runs
> 6. It should be easy to audit/disable linters
> 7. The process to run linters is the same, whether it be in ci, in
> mozreview or on a developer's machine
>
> Eventually, I envision tens or even hundreds of linters, scattered
> throughout the tree. Some running globally, some running only against
> certain file types, and some running only in certain subdirectories.
> There will be a mach command that can run all the files touched by a
> given changeset through all the appropriate linters. Some linters will
> also be enabled by default in ci, or on mozreview.
>
>
> This implementation is largely inspired by mach. There would be 3 parts:
>
> 1. A mozlint core (equivalent to mach core)
> 2. A lint registry (equivalent to mach_bootstrap.py)
> 3. The linters themselves (equivalent to mach_commands.py)
>
>
> Linters
> -------
>
> It is a non-goal to have complicated and expensive linters that
> do many different things at once. For this reason, a linter would be
> nothing more than a well defined data structure and file type (e.g a
> yaml file with a .lint extension).
>
> Each linter would have some basic metadata (i.e name, description), a
> type, a payload and a set of rules. Here are some examples:
>
> * StringLinter - payload is a string, fails if the string exists
> * RegexLinter - payload is a regex, fails if the regex matches
> * ExternalLinter - payload is a command, fails if the command returns
> non-zero
>
> The "external" type would allow arbitrary complexity (e.g hooking in 3rd
> party tools), but make it enough of an inconvenience that it would only
> be used if absolutely necessary. The goal is to keep linters as small
> and fast as possible
>
> Finally each linter would have a set of rules. Rules are things like
> "ext:js,jsm", "path:browser/*", etc. Rules can be combined with logical
> operators (OR, AND, etc). They govern when a linter gets run against a
> particular file. Both rules and types are well defined.
>
>
> Registry
> --------
>
> The registry is how the core finds all the various linters in the tree.
> It doesn't need to be complex, possibly just a path per line in a file.
> Though other options include storing it in moz.build, a special config
> file or a manifestparser manifest. The main takeaway here is that
> multiple registries can exist. E.g, there can be one for mozreview, one
> for ci and a third for the mach command. There could even be user
> created registries for people who just want to enforce a rule in their
> own code without pushing it onto everyone else.
>
>
> Core
> ----
>
> The mozlint core will define the allowed types and allowed rules. It
> will fail to parse any linters that do not adhere to the definition.
> Each linter type will be implemented here, and passed the payload and
> the list of files as arguments. Not much else to say here, other than
> it'll need to run the linters in parallel to make them as efficient as
> possible.
>
> The final missing pieces are the consumers of mozlint. The consumers are
> what will determine which files need to get linted. E.g, the build
> system may spit out a list of all .cpp files, or mercurial might spit
> out a list of all files touched by a certain changeset, or the list of
> files might be manually typed out on the commandline via a mach command.
> Mozlint will not concern itself about how this list gets generated.
>
> And that's about it. I'm going to start tinkering around with this over
> the weekend and see where it goes. I'm sure there are a lot of things
> that I'm not thinking about. All feedback, both positive and negative,
> is much appreciated!
>
> Cheers,
> Andrew