Open Bug 1067071 Opened 10 years ago Updated 2 years ago

support opt-in linting of source files

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(Not tracked)

People

(Reporter: heycam, Unassigned)

Details

I'd like to have the build system allow selected source files to be linted.  To start, I'm interested in enforcing #includes and forward class declarations at the top of .h and .cpp files to be sorted.

I've been able to add support for a new variable in moz.build files and pass that information through to the backend.mk files that get generated.  So I can write say:

  FILES_TO_LINT += [
      "SomeFile.h",
      "SomeFile.cpp",
  ]

which then becomes:

  LINTED_FILES := SomeFile.h SomeFile.cpp

in backend.mk.

From there though I'm not sure what's the best point to run the linting command.  If I do it during the export tier, then it's going to end up running the linter regardless of whether the file changed.  If I do it as part of the compilation rule in rules.mk (the $(CPPOBJS) rule) then this works well for the .cpp files but there's no obvious place to run it for the .h files.  Ideally I would only run the linter on a .h file that has changed since last time, and I don't want it to run for each .cpp file that depends on the .h file.

(Unified compilation makes it a bit tricky too, although I see in rules.mk that it extracts the component .cpp files from a Unified_cpp_blah.cpp file, so that'd probably work for me too.)

Any suggestions?
I'm not sure a separate variable is going to be nice here. How many FILES_TO_LINT do you expect there would be?
I want a mechanism where we can start off with no files linted, and gradually add more and more files as they pass the linter.  So in the end in a given directory, all .h and .cpp files would be listed in FILES_TO_LINT.  Happy to entertain other ways of specifying which files to include/exclude.
FWIW, config/check_spidermonkey_style.py does #include order checking within js/src/. It's run as part of js/src/Makefile.in's |check-style| target, which is run by the |check| target.
I think linting should be a boolean flag on SOURCES or UNIFIED_SOURCES, much like no_pgo and flags are today.

I'd *really* like to see linting enabled by default (someday). It isn't difficult to mass rewrite moz.build files since Python has source code transformation tools (like 2to3). So starting with a whitelist shouldn't be too big of a deal.
We're now getting various parts of the project picking up eslint. Hello is now using it, so are Devtools; Android also wants to pick it up (bug 1170804).

Can we start to get some of this prioritized to some extent?

At the moment, we're using .eslintrc/.eslintignore files - because they're what it can pick up easily, and also provides good integration with plugins for editors so that devs can have it run in real time on their files as they edit.
Linting info is something we can add to the "with Files()" sub-context in moz.build files, which was introduced after this bug was filed.

The hard part of this feature is figuring out how to integrating the linting info. There was some work in bug 939350, but it hasn't landed yet. We've also been wanting to integrating auto linting into MozReview.

First thing's first: figure out a way to go from state of files in version control to linter invocations and the spice can flow. I would prefer that be keyed of moz.build files and the Files sub-context because we're building tools to make querying that data easy. See bug 1139218.
Product: Core → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.