build: add conflict marker check on release#7625
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdexforce-pushed the build-conflict-marker-check branch from 0f4008d to 259d54cCompare
mscdex commented Jul 8, 2016
/cc @nodejs/release |
evanlucas commented Jul 8, 2016
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. |
Makefile Outdated
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.
Fishrock123 commented Jul 9, 2016
Should this be run as a git pre-commit check? |
Fishrock123 commented Jul 9, 2016
(Or on the regular CI?) |
mscdex commented Jul 9, 2016
@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 |
cjihrig commented Jul 9, 2016
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? |
mscdexforce-pushed the build-conflict-marker-check branch from 259d54c to 3d681ddComparemscdex commented Jul 9, 2016
@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 |
mscdexforce-pushed the build-conflict-marker-check branch from 3d681dd to bc3708dComparemscdex commented Jul 12, 2016
@bnoordhuis Added a check for the beginning marker |
mscdexforce-pushed the build-conflict-marker-check branch from bc3708d to 8655b94Compare
rvagg commented Jul 12, 2016
lgtm pending https://ci.nodejs.org/job/node-test-commit/4072/ |
mscdexforce-pushed the build-conflict-marker-check branch from 8655b94 to 52e5594Comparemscdex commented Jul 12, 2016
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/ |
Makefile Outdated
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.
Fishrock123 commented Jul 12, 2016
lgtm as a first step |
mscdexforce-pushed the build-conflict-marker-check branch from 52e5594 to be0062fComparemscdex commented Jul 12, 2016
CI again after changes to conditional: https://ci.nodejs.org/job/node-test-pull-request/3268/ |
bnoordhuis commented Jul 12, 2016
LGTM |
PR-URL: nodejs#7625 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
mscdexforce-pushed the build-conflict-marker-check branch from be0062f to 68b966bComparemscdex commented Jul 20, 2016
One last CI before merging: https://ci.nodejs.org/job/node-test-pull-request/3357/ |
PR-URL: #7625 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins commented Sep 30, 2016
@mscdex should we backport to v4.x? |
PR-URL: #7625 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #7625 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #7625 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
7 participants
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.