build: add conflict marker check on release #7625
Conversation
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
0f4008d
to
259d54c
Compare
|
/cc @nodejs/release |
|
should this also include an update to |
|
@evanlucas Not that I'm aware of? I mean, it wouldn't hurt, but this automatic check should work too. |
| @@ -463,6 +463,12 @@ release-only: | |||
| echo 'Please update Added: tags in the documentation first.' ; \ | |||
| exit 1 ; \ | |||
| fi | |||
| @if $(grep -IPqrs '^>>>>>>> [0-9A-Fa-f]+' benchmark deps doc lib src test tools) || \ | |||
There was a problem hiding this comment.
I'd also check for ^<<<<<<< (but not ^======= because that's going to match a gazillion section markers in documentation.)
There was a problem hiding this comment.
Why both? Wouldn't the current regexp match all instances anyway?
There was a problem hiding this comment.
Would it? I'm thinking of situations where someone removes some of the conflict markers after manual resolution but not everything.
There was a problem hiding this comment.
If that's the case, it's possible they could leave the ======= behind too and we wouldn't have an easy way of checking for that.
There was a problem hiding this comment.
Right. We'd need three checks if we wanted to be really thorough but the third one isn't practical. It's not perfect but two checks is still better than one.
|
Should this be run as a git pre-commit check? |
|
(Or on the regular CI?) |
|
@Fishrock123 I dunno, I just decided to put it on pre-release since it may take some time to scan all the files and someone leaving conflict markers does not happen often. shrug |
|
I think making this part of the CI would be good. I don't think it should hold up a release, or be on the release manager to resolve while trying to get a release out the door. If it might take a while to execute, would it be possible to include the check in the CI lint job, and not make it run on local machines? |
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
259d54c
to
3d681dd
Compare
|
@cjihrig You could probably say the same thing about holding up a release because "REPLACEME" was found in API docs (which is currently the case). I don't really have a strong opinion about when it happens, so long as we can ensure it gets caught early enough without noticeably impacting normal operations. I've moved it to the |
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
3d681dd
to
bc3708d
Compare
|
@bnoordhuis Added a check for the beginning marker |
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
bc3708d
to
8655b94
Compare
|
lgtm pending https://ci.nodejs.org/job/node-test-commit/4072/ |
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
8655b94
to
52e5594
Compare
|
I tweaked the grep args, apparently (Free)BSD does not support the Perl regexps option, but the Extended regexps option works just as well. CI again: https://ci.nodejs.org/job/node-test-pull-request/3262/ |
| lint-ci: jslint-ci cpplint | ||
| @if [ "$(shell grep -IEqrs "$(CONFLICT_RE)" benchmark deps doc lib src test tools > /dev/null 2>&1; echo $$?)" != "0" \ |
There was a problem hiding this comment.
Can't this be simplified to if ! ( grep -EIqrs ... ) && ! ( find . -maxdepth 1 ... ); then? You could swap the clauses, replace && with || and drop the negation to shorten it even more.
You probably don't need to redirect stdio either if you are suppressing errors with -s.
|
lgtm as a first step |
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
52e5594
to
be0062f
Compare
|
CI again after changes to conditional: https://ci.nodejs.org/job/node-test-pull-request/3268/ |
|
LGTM |
PR-URL: nodejs#7625 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
mscdex
force-pushed
the
build-conflict-marker-check
branch
from
be0062f
to
68b966b
Compare
|
One last CI before merging: https://ci.nodejs.org/job/node-test-pull-request/3357/ |
PR-URL: #7625 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
@mscdex should we backport to v4.x? |
PR-URL: #7625 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
just finished backporting... the job found some markers that shouldn't have been there and has already saved v4.x from some pain <3 @mscdex |
PR-URL: #7625 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #7625 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #7625 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
Description of change
This change to the release process helps find conflict markers in files so that they don't make it into a release.