build: call setlocal in vcbuild.bat

[danbev]

Currently the variables set in vcbuild.bat are mostly global and
escape/leak out into the calling process. For example, running
vcbuild.bat test and then echoing the config variable gives:

vcbuild.bat test...echo %config%Release

After this change the same command give:

vcbuild.bat test...echo %config%%config%

Checklist

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

Affected core subsystem(s)

build

[danbev]

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

[danbev]

Landed in b9a55a9

[MylesBorins]

this landed cleanly on v6.x. LMK if it should be backed out

[refack]

@MylesBorins back it out. This is semver-major

[refack]

This change is semver-major.

[refack]

Revert PR: #16270

[refack]

Post-mortem: what happened to the "who to CC" procedure? https://github.com/nodejs/node/blob/master/doc/onboarding-extras.md
AFAIK it was created so that things will not slip unreviewed.

[danbev]

Apologies for this, I really thought this was a very minor change 😞 Sorry about causing extra work.

Post-mortem: what happened to the "who to CC" procedure?

Even though I'm aware of that section in the docs I've not been using it at all. I'll make sure to use it in future PRs.

[lance]

@refack I think there should be more to the post-mortem than this. There is no specific group or person identified for build in https://github.com/nodejs/node/blob/master/doc/onboarding-extras.md#who-to-cc-in-issues, only platform - e.g. @nodejs/platform-windows. It might be good to update the "who to CC" section with a bit about build: PRs, since it's not immediately obvious that the platform team should have been notified.

To be honest, I'm pretty sure I would have done the same thing here. It's a one line change, reviewed by a TSC member, landing after 48 hours have elapsed. Other than those 48 hours having occurred primarily over a weekend, it seems pretty much by-the-book.

In #16270 you mention that this behavior was discussed several times in the past, but where would one look to know about these discussions? If we can improve the documentation to prevent an error like this in the future, it would be great. Sometimes you just don't know what you don't know.

/cc @jasnell@MylesBorins@addaleax@danbev

[refack]

First of all I'm replying here since it's interesting for me to try and learn. not piling on @danbev whom I respect and IMHO did nothing wrong.

@lance I tend to agree with your conclusions.

1. I would probably have done the same, I don't think the process was broken.
2. I don't CC enough, both in PRs I open, and PRs I review.
3. Written knowledge is highly fragmented around the GitHub org, and while code changes can be git blamed, thing that were decided not to do, have no trace except in GitHub comments.

That's why I only touched on the "who to CC" issue, since at the moment more eyeballs is the best heuristic I'm aware of to defragment the "institutional knowledge"

[MylesBorins]

I've backed this out of v6.x-staging... we should land the revert commit on 8.x in the next release

[refack]

we should land the revert commit on 8.x in the next release

I've noted that down.

[lance]

@refack in this case, is it worth a code comment in vcbuild.bat? I only suggest this because apparently the issue has been discussed and rejected before. I don't know enough about building on Windows to know if it's likely to come up again or not.

I also tend to think of /cc'ing as nagging a bit. That's a misperception on my part for sure. But of course, I cc'd the world in my comment above. :)