Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: don't lint files that have not changed #16581

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 29, 2017
@joyeecheung
Copy link
Member Author

Makefile Outdated
./*.md doc src lib benchmark tools/doc/ tools/icu/
LINT_MD_TARGETS = src lib benchmark tools/doc tools/icu
LINT_MD_FILES := $(shell find $(LINT_MD_TARGETS) -type f \
-not -path '*node_modules*' -name '*.md') $(shell ls ./*.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Makefile Outdated
lint-cpp:
lint-cpp: tools/.cpplintstamp

tools/.cpplintstamp: $(LINT_CPP_FILES)
@echo "Running C++ linter..."
@$(PYTHON) tools/cpplint.py $(LINT_CPP_FILES)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

joyeecheung commented Nov 6, 2017

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
Copy link
Member

LGTM

@joyeecheung
Copy link
Member Author

Landed in eebcb48, thanks!

@joyeecheung joyeecheung closed this Nov 8, 2017
joyeecheung added a commit that referenced this pull request Nov 8, 2017
PR-URL: #16581
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@ljharb
Copy link
Member

ljharb commented Nov 8, 2017

@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
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Nov 8, 2017

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

@ljharb
Copy link
Member

ljharb commented Nov 8, 2017

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
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 8, 2017

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

evanlucas pushed a commit that referenced this pull request Nov 13, 2017
PR-URL: #16581
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@MylesBorins
Copy link
Contributor

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.

MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
PR-URL: #16581
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@joyeecheung joyeecheung self-assigned this Nov 17, 2017
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants