Skip to content

Revert "build: call setlocal in vcbuild.bat" - #16270

Closed
refack wants to merge 0 commit into
nodejs:masterfrom
refack:revert-b9a55a9
Closed

Revert "build: call setlocal in vcbuild.bat"#16270
refack wants to merge 0 commit into
nodejs:masterfrom
refack:revert-b9a55a9

Conversation

@refack

@refackrefack commented Oct 17, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

This reverts commit b9a55a9.

The above commit is semver-major and was landed too quickly.
vcbuild.bat exports multiple environment variables, which is essential for continued build steps. Disabling this setup is semver-major as it will break embedders workflows, and breaks developers tools.
Besides the semverity this behaviour was discussed several times in the past, and it was decided that it should stay as is.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

build,windows

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Oct 17, 2017
@refackrefack mentioned this pull request Oct 17, 2017
Closed
2 tasks
@addaleax

addaleax commented Oct 17, 2017

Copy link
Copy Markdown
Member

The commit message and PR description should ideally provide a reason for the revert

@refack

refack commented Oct 17, 2017

Copy link
Copy Markdown
ContributorAuthor

Updated OP

jasnell
jasnell approved these changes Oct 18, 2017
View reviewed changes

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, this definitely did not get enough review.

danbev
danbev approved these changes Oct 18, 2017
View reviewed changes

@danbevdanbev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

addaleax
addaleax approved these changes Oct 18, 2017
View reviewed changes

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, this does make sense now :)

@jasnell

jasnell commented Oct 18, 2017

Copy link
Copy Markdown
Member

@danbev ... no worries at all!

MylesBorins
MylesBorins approved these changes Oct 18, 2017
View reviewed changes

@MylesBorinsMylesBorins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Shouldn't have to wait 48 hours

@refack

refack commented Oct 18, 2017

Copy link
Copy Markdown
ContributorAuthor

LGTM

Shouldn't have to wait 48 hours

No real rush IMHO. Now we have a PR it's traceable/searchable, and it can be a sort of Canary, to see if anyone else cries out.

refack added a commit that referenced this pull request Oct 19, 2017
@refack
build: revert "call setlocal in vcbuild.bat"
c3ae57f
This reverts commit b9a55a9.
PR-URL: #16270
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@refackrefack closed this Oct 19, 2017
@refack
refack deleted the revert-b9a55a9 branch October 19, 2017 22:37
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@refack@MylesBorins
build: revert "call setlocal in vcbuild.bat"
6ce04d9
This reverts commit b9a55a9.
PR-URL: #16270
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@refack@addaleax
build: revert "call setlocal in vcbuild.bat"
bdfa031
This reverts commit b9a55a93c91fb7fd7ac81e182f843f28014179ca.
PR-URL: nodejs/node#16270
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins

MylesBorins commented Nov 16, 2017

Copy link
Copy Markdown
Contributor

Setting dont land for v6.x as this has to do with a semver major change

I'm not sure if this should have landed on 8.x

/cc @refack@gibfahn

@refack

refack commented Nov 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Yes, don't land, since the original PR did land.

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@refack@addaleax
build: revert "call setlocal in vcbuild.bat"
d3c40bf
This reverts commit b9a55a93c91fb7fd7ac81e182f843f28014179ca.
PR-URL: nodejs/node#16270
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@richardlaurichardlau mentioned this pull request Jan 9, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
+2 more reviewers
@danbevdanbevdanbev approved these changes
@MylesBorinsMylesBorinsMylesBorins approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buildIssues and PRs related to build files or the CI.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@refack@addaleax@jasnell@MylesBorins@danbev@cjihrig@nodejs-github-bot