tools: don't lint files that have not changed

[joyeecheung]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

After #16284 landed we now run linters before running test. This PR prevents make from linting files that have not changed, so e.g. if someone only touches lib they don't have to wait for cpp linter or the markdown linter to finish.

JS linter does not need this because eslint has cache already.

The time for running make lint the second time before this patch:

make lint  37.86s user 2.08s system 105% cpu 37.893 total

After:

make lint  2.57s user 0.56s system 103% cpu 3.032 total

[joyeecheung]

CI: https://ci.nodejs.org/job/node-test-pull-request/11065/

[bnoordhuis]

Is there a reason to shell out to find and ls instead of using $(wildcard ...)?

[joyeecheung]

I have tried to use $(wildcard ..), but will need to filter out files inside node_modules and loop over the LINT_MD_TARGETS...not sure if there is a better way to write this?

[bnoordhuis]

It's not super duper pretty but you can filter the results of $(wildcard ...) like this:

LINT_MD_FILES := \
    $(foreach x,$(wildcard ...), \
        $(if $(findstring node_modules,$(x)),,$(x)))

[bnoordhuis]

Replace $(LINT_CPP_FILES) with $? and it will run the linter only for files that changed since last time.

It would be nice to do the same thing with tools/check-imports.py but that script will need a tweak to accept a list of file names.

[joyeecheung]

Updated to use $? in the cpp lint target. Can't do the same for the lint-md targets because remark somehow prints the files that has been changed if we pass the arguments like that..

I have tried the wildcard approach but find it a tad too complicated if we need to search the directories recursively (including both **/*.md and *.md), so I've decided to keep it simple with shell find, but shell ls can be replaced.

@bnoordhuis @gireeshpunathil @apapirovski PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/11217/

[gireeshpunathil]

LGTM

[joyeecheung]

Landed in eebcb48, thanks!

[ljharb]

@joyeecheung fwiw this isn't actually a good idea to do; using eslint-plugin-import, for example, means that changing file A might cause a linting error in file B even if file B doesn't change; the same is true of any change to eslint, any eslint config file, or any eslint config/plugin used.

The only safe thing is to always lint every file on every CI run.

[gireeshpunathil]

@ljharb - can you please show an example of such side-effect please?

[Trott]

@ljharb I don't think this change affects ESLint. It is still being invoked on every file every time.

[ljharb]

Ah, i can't speak for a non-JS linter :-)

@gireeshpunathil require('./file') in foo.js, and then deleting file.js would cause a linter error in foo.js without that file having changed, if you were using import/no-unresolved, for example. The only time a linter is safe to run on "changed files only" is if you're never looking across files for effects; however, in a world with a module system, a linter that doesn't look across files is missing out on a lot of opportunity to ensure correctness.

Separately, you could add an .eslintrc file (or change any linting tool's config) without touching the file, and the altered config could cause new linter errors in untouched files. This could happen in any language, with any linting tool, that had configuration that lived outside the code file itself.

[joyeecheung]

@ljharb This PR is not related to JS linting, it only changes the linting of C++ and markdown files.

[joyeecheung]

@ljharb Looking at it again I think it affects this PR as well, in that we have not put cpplint.py and remark as dependencies(we should, since the idea is if something is used in a make rule then it should probably be added to the dependencies). I'll open another PR fixing those.

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.