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

build: only lint markdown files that have changed (POSIX-only) #31923

Merged
merged 1 commit into from
Mar 1, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 23, 2020

Update Makefile so that only markdown files that have changed will be
linted. Currently, if one file in doc/api has changed, all files in
doc/api are linted.

On Windows, the lint-md task currently lints all files regardless of
whether any files has changed, and that behavior is unchanged here.

A further improvement is that when tools/lint-md.js is rebuilt, the
timestamp file is removed so that all files are linted again. This is
because rebuilding lint-md.js can introduce new rules or modify existing
rules, so re-linting everything helps make sure that accidental breakage
doesn't slip by unnoticed.

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

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Feb 23, 2020
@Trott
Copy link
Member Author

Trott commented Feb 23, 2020

@nodejs/linting @nodejs/build-files @DerekNonGeneric

@Trott
Copy link
Member Author

Trott commented Feb 23, 2020

On my laptop, this change saves 17 seconds on the markdown lint run when a single .md file in doc/api is edited.

$ touch doc/api/assert.md && time make lint-md
Running Markdown linter on docs...

real	0m17.836s
user	0m20.322s
sys	0m1.128s
$ git stash pop && !!
git stash pop && touch doc/api/assert.md && time make lint-md
On branch master
Your branch is up-to-date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   Makefile

no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (b0765bc82f1d2948f958ebd630363f8dc67d2271)
Running Markdown linter on docs...

real	0m0.812s
user	0m0.601s
sys	0m0.218s
$

@nodejs-github-bot
Copy link
Collaborator

@DerekNonGeneric
Copy link
Contributor

I agree that, due to the potential for a rebuilt linter to have different verification criteria, all verifiable files would need to be re-checked. However, I don't think it's necessary to have a timestamp file at all.

Rather than placing a timestamp (.mdlintstamp) file in the tools directory each time a full verification is completed, I think it would be more intelligent to run touch ./tools/lint-md.js, which would update the timestamp of the actual linter doing the verification. Likewise, a successfully rebuilt linter would automatically have its timestamp updated.

That way, it would be known that any files newer than the timestamp of this linter file would need to be re-verified. What do you think?

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Feb 23, 2020

I should add that each time the linter is re-built, all verifiable files would then need to be touched, which would cause their timestamps to be newer than the linter file (Markdown newer than linter == unverified). This would result in them needing to be re-verified. Here's how it would work in practice.

Building the linter (run make lint-md-rollup)

  1. Build Markdown linter [linter's timestamp is now current]
  2. Touch all Markdown targets [linter is now older than Markdown targets]

Linting the docs (run make lint-md)

  1. Lint all Markdown targets newer than tools/lint-md.js (all of them)
  2. On success, touch tools/lint-md.js [linter is now newer than Markdown targets]

It's just an idea if interested in doing away with the stamp file. If not, LGTM.

@Trott
Copy link
Member Author

Trott commented Feb 24, 2020

I think having a separate timestamp file like we do now is preferable. If someone is having weird issues with their files not being linted (been there!) and they just want to hit a reset button and have all files re-linted (been there too!), it's easy with a timestamp file: Just remove it.

In the scenario you describe, we'd either have to "touch" all lintable files (yikes), artificially set the timestamp on lint-md.js to something in the (probably distant) past, or introduce an otherwise-unnecessary option like a -f flag or something that will then likely mean another Makefile task.

@Trott
Copy link
Member Author

Trott commented Feb 24, 2020

Also, in the current setting, the Makefile itself handles most of the timestamp magic. If we move it to be the timestamp of the lint-md.js file, we lose most or all of that.

@DerekNonGeneric
Copy link
Contributor

Very good points. Perhaps having an independent stamp file would be preferable.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 24, 2020
@nschonni
Copy link
Member

Might end up having a high overhead, but did you consider creating an MD5 hash of the files instead of the timestamp? That would be cross platform compatible AFAIK

@Trott
Copy link
Member Author

Trott commented Feb 25, 2020

Might end up having a high overhead, but did you consider creating an MD5 hash of the files instead of the timestamp? That would be cross platform compatible AFAIK

Timestamp is already implemented. I modified the existing implementation.I did not consider replacing it with a completely new implementation.

If someone wants to experiment with other implementations, that's fine, of course. I don't plan on doing that here.

@Trott
Copy link
Member Author

Trott commented Feb 27, 2020

Another benefit to this change is that the doc files are linted even if there are failures in the "misc" files. If you look at the current lint results for #31976, you'll see that the errors in the misc files show up but then the linter bails out and it doesn't lint the doc file changes which also have lint issues. I'd prefer it always report all the problems, which will be the case with this change.

Update Makefile so that only markdown files that have changed will be
linted. Currently, if one file in doc/api has changed, all files in
doc/api are linted.

On Windows, the lint-md task currently lints all files regardless of
whether any files has changed, and that behavior is unchanged here.

A further improvement is that when tools/lint-md.js is rebuilt, the
timestamp file is removed so that all files are linted again. This is
because rebuilding lint-md.js can introduce new rules or modify existing
rules, so re-linting everything helps make sure that accidental breakage
doesn't slip by unnoticed.

PR-URL: nodejs#31923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member Author

Trott commented Mar 1, 2020

Landed in 751c240

@Trott Trott merged commit 751c240 into nodejs:master Mar 1, 2020
@Trott Trott deleted the just-the-changes branch March 1, 2020 18:41
@codebytere codebytere mentioned this pull request Mar 3, 2020
codebytere pushed a commit that referenced this pull request Mar 3, 2020
Update Makefile so that only markdown files that have changed will be
linted. Currently, if one file in doc/api has changed, all files in
doc/api are linted.

On Windows, the lint-md task currently lints all files regardless of
whether any files has changed, and that behavior is unchanged here.

A further improvement is that when tools/lint-md.js is rebuilt, the
timestamp file is removed so that all files are linted again. This is
because rebuilding lint-md.js can introduce new rules or modify existing
rules, so re-linting everything helps make sure that accidental breakage
doesn't slip by unnoticed.

PR-URL: #31923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Update Makefile so that only markdown files that have changed will be
linted. Currently, if one file in doc/api has changed, all files in
doc/api are linted.

On Windows, the lint-md task currently lints all files regardless of
whether any files has changed, and that behavior is unchanged here.

A further improvement is that when tools/lint-md.js is rebuilt, the
timestamp file is removed so that all files are linted again. This is
because rebuilding lint-md.js can introduce new rules or modify existing
rules, so re-linting everything helps make sure that accidental breakage
doesn't slip by unnoticed.

PR-URL: #31923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Update Makefile so that only markdown files that have changed will be
linted. Currently, if one file in doc/api has changed, all files in
doc/api are linted.

On Windows, the lint-md task currently lints all files regardless of
whether any files has changed, and that behavior is unchanged here.

A further improvement is that when tools/lint-md.js is rebuilt, the
timestamp file is removed so that all files are linted again. This is
because rebuilding lint-md.js can introduce new rules or modify existing
rules, so re-linting everything helps make sure that accidental breakage
doesn't slip by unnoticed.

PR-URL: #31923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Update Makefile so that only markdown files that have changed will be
linted. Currently, if one file in doc/api has changed, all files in
doc/api are linted.

On Windows, the lint-md task currently lints all files regardless of
whether any files has changed, and that behavior is unchanged here.

A further improvement is that when tools/lint-md.js is rebuilt, the
timestamp file is removed so that all files are linted again. This is
because rebuilding lint-md.js can introduce new rules or modify existing
rules, so re-linting everything helps make sure that accidental breakage
doesn't slip by unnoticed.

PR-URL: #31923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants