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

Revert "win,msi: install tools for native modules" #24344

Merged
merged 1 commit into from Nov 18, 2018

Conversation

9 participants
@refack
Member

refack commented Nov 13, 2018

This reverts #22645 and subsequent patches for the v10.x branch.

Revision: 257a5e9
win: add prompt to tools installation script

Revision: e9a2915
win: clarify Boxstarter behavior on install tools

Revision: 3b895d1
win,msi: display license notes before installing tools

Revision: cf284c8
win,msi: install Boxstarter from elevated shell

Revision: 2b7e18d
win,msi: highlight installation of 3rd-party tools

Revision: ebf36cd
win,msi: install tools for native modules

Refs: #22645
Refs: #23987
Refs: nodejs/Release#369
Refs: #23838
Refs: nodejs/security-wg#439

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
@refack

This comment has been minimized.

Member

refack commented Nov 13, 2018

@joaocgreis

Note the last two commits never made it to a release, we can't really judge their impact. I'm OK with doing this only in v11 for now.

@cfanoulis

LGTM

I feel bad for saying this, but imo they weren't needed in the first place

@refack

This comment has been minimized.

Member

refack commented Nov 13, 2018

I'm OK with doing this only in v11 for now.

@joaocgreis I targeted this for v10. Is that Ok?

@refack

This comment has been minimized.

Member

refack commented Nov 13, 2018

I feel bad for saying this, but imo they weren't needed in the first place

@cfanoulis for the record, making it easier for Windows users to install native-addons has been a long time goal. With this PR I'm not advocating removing the new script, it just needs more work, and it probably just not ready for our LTS release line.

@cfanoulis

This comment has been minimized.

Contributor

cfanoulis commented Nov 13, 2018

I have been discussing this with other developers over other media upon the releasing of v10.12.0 , and everyone was questioning themselves: but why not just npm i -g windows-build-tools.

You may say, but they didn't select an NPM installation. Wouldn't hurt (always in my belief) to briefly install npm, get the build tools, then uninstall npm

@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Nov 13, 2018

@refack This is OK, I'm sorry I was not clear.

By "I'm OK with doing this only in v11 for now" I meant judging the impact of the last 2 commits, that should be included in the next v11 release.

@refack

This comment has been minimized.

Member

refack commented Nov 13, 2018

but why not just `npm i -g windows-build-tools`.

That's a fair question. See #22311 (comment) for some discussion. Short answer is we are looking for a formula were we control its lifecycle, and that will get Microsoft's blessing.

We are going to continue improving install_tools until we optimize it.

@targos targos referenced this pull request Nov 14, 2018

Closed

win: add prompt to tools installation script #23987

2 of 2 tasks complete

@joaocgreis joaocgreis referenced this pull request Nov 14, 2018

Closed

win: add customization warning to tools script #24348

2 of 2 tasks complete
@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Nov 14, 2018

@cfanoulis I also talked about windows-build-tools in #22645 (comment)

@refack

This comment has been minimized.

Member

refack commented Nov 15, 2018

@Trott

This comment has been minimized.

Member

Trott commented Nov 16, 2018

Since this is on an LTS release line, I guess someone from @nodejs/releasers has to land it. FWIW, IMO it would be nice if this landed sooner rather than later.

@targos

This comment has been minimized.

Member

targos commented Nov 17, 2018

@richardlau

This comment has been minimized.

Member

richardlau commented Nov 18, 2018

Since this is on an LTS release line, I guess someone from @nodejs/releasers has to land it.

Not true, https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#how-are-lts-branches-managed

Any Collaborator may land commits into a staging branch, but only the release team should land commits into the LTS branch while preparing a new LTS release.

So it's fine for any collaborator to land in v10.x-staging. If noone else does beforehand, I'll land this on Monday.

FWIW, IMO it would be nice if this landed sooner rather than later.

I'm not sure when we're planning the next 10.x release, @nodejs/lts?

Revert "win,msi: install tools for native modules"
This reverts:
	Revision: 257a5e9
	win: add prompt to tools installation script

	Revision: e9a2915
	win: clarify Boxstarter behavior on install tools

	Revision: 3b895d1
	win,msi: display license notes before installing tools

	Revision: cf284c8
	win,msi: install Boxstarter from elevated shell

	Revision: 2b7e18d
	win,msi: highlight installation of 3rd-party tools

	Revision: ebf36cd
	win,msi: install tools for native modules

PR-URL: #24344
Refs: #22645
Refs: #23987
Refs: nodejs/Release#369
Refs: #23838
Refs: nodejs/security-wg#439
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

@refack refack force-pushed the refack:v10-revert-install-native-tools branch from 5e835a7 to 469473d Nov 18, 2018

@refack refack removed the author ready label Nov 18, 2018

@refack refack removed the land-on-v10.x label Nov 18, 2018

@refack refack merged commit 469473d into nodejs:v10.x-staging Nov 18, 2018

@refack refack deleted the refack:v10-revert-install-native-tools branch Nov 18, 2018

@refack refack removed the request for review from tniessen Nov 18, 2018

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 18, 2018

@refack you are not a member of @nodejs/backporters and as such should not have landed this on a staging branch. If you are not seeing things move fast enough please feel free to ping someone directly.

@refack

This comment has been minimized.

Member

refack commented Nov 18, 2018

@MylesBorins Please read comment above by @richardlau

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 18, 2018

@refack that is a misreading imho... we have never allowed for non LTS members to backport to the staging branch

@targos

This comment has been minimized.

Member

targos commented Nov 18, 2018

Then the documentation should be fixed. We cannot assume that every collaborator knows that we do not effectively follow the rule as it is written.

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 18, 2018

@richardlau

This comment has been minimized.

Member

richardlau commented Nov 18, 2018

@refack that is a misreading imho... we have never allowed for non LTS members to backport to the staging branch

So I'll take responsibility here as per my comment above. IMO it's not a misreading -- the documented process is plainly wrong if we're restricting landing commits on staging branches. (#24465 will address this for the collaborator guide).

Neither the collaborator guide nor the release README (https://github.com/nodejs/Release/blob/20ff702661d11d5ceeeb495764e575ba13e5fc17/README.md#lts-staging-branches) forbid non-LTS members from landing in the LTS staging branches. The language in both makes distinctions between the release branches (e.g. v10.x) and staging branches (e.g. v10.x-staging) and then singles out the release branch as being restricted to members of the release team. The collaborator guide as currently written allows "any collaborator" to land commits in the staging branch (having previously defined the staging branch earlier in the same section).

danbev added a commit that referenced this pull request Nov 22, 2018

doc: clarify who may land on an LTS staging branch
Current language is a bit confusing

PR-URL: #24465
Refs: #24344 (comment)
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

targos added a commit that referenced this pull request Nov 24, 2018

doc: clarify who may land on an LTS staging branch
Current language is a bit confusing

PR-URL: #24465
Refs: #24344 (comment)
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@runewake2

This comment has been minimized.

runewake2 commented Nov 25, 2018

Hi, can this be reopened? While it removes the damaging scripts, no documentation is added to address reverting PC's that have already been modified by these scripts. A documentation update should be included with this change as a good number of the people I have polled have experienced some form of system corruption (many were not even aware Node.JS was the source until I brought this to their attention!).

This change currently only prevents further damage to users systems. I would like to see it go further to help repair the corrupted systems that now exist. The current best option is reinstalling the OS which is an awful one.

@refack

This comment has been minimized.

Member

refack commented Nov 25, 2018

Hello @runewake2,
Just for simplicity, this PR is a 1:1 reverting of existing commits (this is also limited in scope to node 10).
IMO documentation improvements could come in as a separate PR.

@cfanoulis

This comment has been minimized.

Contributor

cfanoulis commented Nov 25, 2018

Hey @runewake2,

Would you be willing to clarify what you mean by saying system corruption?

a good number of the people I have polled have experienced some form of system corruption

Whilst I was again this change, I tried this installation method on different hardware and/or different OS versions (Windows 7/8.1/10) and I never experienced any errors and/or system corruptions.
Do note, that if you do not wish to continue here (this has started to wander a bit offtopic), I would be much willing to have a talk over Discord or over email (charalampos {at} cfanoulis.me)

~Charalampos

@runewake2

This comment has been minimized.

runewake2 commented Nov 25, 2018

This enrolled my PC in group policy, disabled UAC and disabled automatic updates at a minimum (I haven't fully looked into the extent of the changes, one person mentioned their Windows Store no longer functioning though I don't have that issue). See #24637, happy to discuss further over there. A number of my viewers have reached out to confirm this same policy had been added to their PC's as well after installing Node.JS. The fact that these changes may have occurred is extremely unclear (hidden in a ~9000 line log file under %appdata%/Boxstarter and I suspect very few people check if their PC suddenly has a Group Policy or that their PC's update settings have been modified because they wanted to make a quick React or Electron app. IMO this needs to be made extremely clear, if this is preventing things like Security Updates from being received doubly so.

@refack

This comment has been minimized.

Member

refack commented Nov 25, 2018

system corruption

Not answering for @runewake2, but in my POV, boot loops, or system left with UAC / auto-update disabled, is some measure of corruption.
Even a single unexpected forced reboot (bypassing "save before exit") is an unwanted consequence. But that doesn't have a lingering effect, and doesn't require reverting...

@cfanoulis

This comment has been minimized.

Contributor

cfanoulis commented Nov 25, 2018

Oh, I see. Thanks @runewake2 and @refhack , I see good reasoning now behind this revert

@codebytere codebytere referenced this pull request Nov 27, 2018

Closed

v10.13.1 proposal #24675

rvagg added a commit that referenced this pull request Nov 28, 2018

Revert "win,msi: install tools for native modules"
This reverts:
	Revision: 257a5e9
	win: add prompt to tools installation script

	Revision: e9a2915
	win: clarify Boxstarter behavior on install tools

	Revision: 3b895d1
	win,msi: display license notes before installing tools

	Revision: cf284c8
	win,msi: install Boxstarter from elevated shell

	Revision: 2b7e18d
	win,msi: highlight installation of 3rd-party tools

	Revision: ebf36cd
	win,msi: install tools for native modules

PR-URL: #24344
Refs: #22645
Refs: #23987
Refs: nodejs/Release#369
Refs: #23838
Refs: nodejs/security-wg#439
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

rvagg added a commit that referenced this pull request Nov 28, 2018

doc: clarify who may land on an LTS staging branch
Current language is a bit confusing

PR-URL: #24465
Refs: #24344 (comment)
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 29, 2018

Revert "win,msi: install tools for native modules"
This reverts:
	Revision: 257a5e9
	win: add prompt to tools installation script

	Revision: e9a2915
	win: clarify Boxstarter behavior on install tools

	Revision: 3b895d1
	win,msi: display license notes before installing tools

	Revision: cf284c8
	win,msi: install Boxstarter from elevated shell

	Revision: 2b7e18d
	win,msi: highlight installation of 3rd-party tools

	Revision: ebf36cd
	win,msi: install tools for native modules

PR-URL: #24344
Refs: #22645
Refs: #23987
Refs: nodejs/Release#369
Refs: #23838
Refs: nodejs/security-wg#439
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

@MylesBorins MylesBorins referenced this pull request Nov 29, 2018

Merged

v10.14.1 proposal #24711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment