Skip to content
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

win,msi: install tools for native modules #22645

Closed

Conversation

joaocgreis
Copy link
Member

Add a dialog during installation with information about native modules that can optionally run a Boxstarter script at the end of the installation. This script can also be run from Start menu.

Adding semver-minor since this is adding a new feature. Not semver-major since this should not break anything (I tested upgrading).

Fixes: #22311

cc @nodejs/platform-windows

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

Add a dialog during installation with information about native
modules that can optionally run a Boxstarter script at the end of the
installation. This script can also be run from Start menu.

Fixes: nodejs#22311
@joaocgreis joaocgreis added windows Issues and PRs related to the Windows platform. install Issues and PRs related to the installers. semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v6.x labels Sep 1, 2018
@nodejs-github-bot nodejs-github-bot added install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Sep 1, 2018
@joaocgreis
Copy link
Member Author

@joaocgreis
Copy link
Member Author

@jdalton updated for https. This follows the instructions at https://boxstarter.org/Learn/WebLauncher. The script comes from https://github.com/chocolatey/boxstarter/blob/master/BuildScripts/bootstrapper.ps1, but since this is an external tool, shouldn't we be using their published and stable version?

@jdalton
Copy link
Member

jdalton commented Sep 2, 2018

@joaocgreis

Ah nice! They have tagged stable versions too. Are those tags exposed via the web address?

@joaocgreis
Copy link
Member Author

@jdalton not that I know of... There is https://github.com/chocolatey/boxstarter/releases/latest, but I don't know of an easy way to access the files from there.

I'm not sure I understand your issue with using the Boxstarter site directly. We're trusting an external tool, Boxstarter (and Chocolatey), and it'll run a lot more scripts when installing the tools. It's a well established tool, so I believe this makes sense. If the issue is about their website getting hacked, isn't that also a concern with GitHub or the Chocolatey packages themselves?

@jdalton
Copy link
Member

jdalton commented Sep 2, 2018

@joaocgreis

You're right that it's going to be pulling in plenty of other dependencies. I was just curious mostly. The switch to https is win though, so thanks!

\cc @ferventcoder as the creator/lead of Chocolatey and the publisher of Node releases on Chocolatey. This PR is introducing the use of chocolatey/boxstarter so also pinging @mwrock for visibility.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

<String Id="NativeToolsDlgTitle">{\WixUI_Font_Title}Tools for Native Modules</String>
<String Id="NativeToolsDlgDescription">Optionally install the tools necessary to compile native modules.</String>
<String Id="NativeToolsDlgBannerBitmap">WixUI_Bmp_Banner</String>
<String Id="NativeToolsDlgIntro">Some npm modules need to be compiled from C/C++ when installing. If you want to be able to install such modules, some tools (Python 2 and VS Build Tools) need to be installed.</String>
Copy link
Member

Choose a reason for hiding this comment

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

nit: VS Build Tools -> Visual Studio Build Tools

@richardlau
Copy link
Member

For reference, this is what the additional dialog looks like (with the installer from the test build link in #22645 (comment)):

image

@targos
Copy link
Member

targos commented Sep 5, 2018

What happens if Visual Studio is already installed and one checks the box?

@joaocgreis
Copy link
Member Author

@richardlau updated, thanks!

@targos VS2017 supports installations of different VS editions side by side. So this should just install the VS Build Tools or update with the required components if already installed.

@joaocgreis
Copy link
Member Author

Updated screenshots:

2018-09-05 17_40_09-node js setup

2018-09-05 17_46_38-c__windows_system32_cmd exe

@richardlau
Copy link
Member

FWIW the installer isn't built by the regular PR CI's but João did a test build in #22645 (comment) and I have separately built the installer locally with the current changes for this PR with Visual Studio 2017 and WiX Toolset 3.11.1.

Tested the dialog pops up as expected, that the link to https://github.com/nodejs/node-gyp#on-windows works and the batch file starts at the end of the installation (if the checkbox is ticked) and from the Start menu link (both times I closed the Window to cancel as I didn't want to make changes to my development machine).

joaocgreis added a commit that referenced this pull request Sep 12, 2018
Add a dialog during installation with information about native
modules that can optionally run a Boxstarter script at the end of the
installation. This script can also be run from Start menu.

Fixes: #22311
PR-URL: #22645
Reviewed-By: Richard Lau <[email protected]>
@joaocgreis
Copy link
Member Author

Landed in 3993793

Thanks @richardlau!

@oliversalzburg
Copy link
Contributor

Why not npm install windows-build-tools? Seems like that would be a much more obvious choice than the choco approach taken here.

@joaocgreis
Copy link
Member Author

@oliversalzburg essentially for reliability. It would be great if installing things would be just start the installer and wait for it to complete, but there are many details that can make it fail. Boxstarter and Chocolatey are solutions dedicated to make automatic installations work well, and handle many points for us (installing Windows upgrades, reboots, and so on). windows-build-tools is a nice tool for anyone familiar with npm, but not as reliable as Boxstarter and Chocolatey. In fact, I believe that windows-build-tools would be better if it used something like the script here in the background. I'm not claiming the solution here will work perfectly in every situation, but it already works quite well and there's a community invested in improving it.

There are also other points to consider, that we could probably work around but don't have to with this. When new versions of Python or VS come out the current script should be able to upgrade without issues, while windows-build-tools would have to be updated and tested by us. Also, Node currently does not come with any module installed. We would have to consider where to install and how well it would work for other users in the machine.

So, the current solution should be as close as possible to "one click and it works". For anyone wanting more control, this includes links to instructions that should be quite simple to follow and one of the options there is windows-build-tools.

@gep13
Copy link

gep13 commented Oct 12, 2018

I wanted to make sure that @yodurr, @crutkas and @bitcrazed had seen this as well. The usage of BoxStarter and Chocolatey here is similar to what is being created here: https://github.com/Microsoft/windows-dev-box-setup-scripts for different developer setups.

@crutkas
Copy link

crutkas commented Oct 12, 2018

would love to better understand the exact needs and what can be done. @yodurr without a doubt would too.

jasnell pushed a commit that referenced this pull request Oct 17, 2018
Notable changes:

* assert
  * The diff output is now a tiny bit improved by sorting object
    properties when inspecting the values that are compared with each
    other. #22788
* cli
  * The options parser now normalizes `_` to `-` in all multi-word
    command-line flags, e.g. `--no_warnings` has the same effect as
    `--no-warnings`. #23020
  * Added bash completion for the `node` binary. To generate a bash
    completion script, run `node --completion-bash`. The output can be
    saved to a file which can be sourced to enable completion.
    #20713
* crypto
  * Added support for PEM-level encryption.
    #23151
  * Added an API asymmetric key pair generation. The new methods
    `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be
    used to generate public and private key pairs. The API supports
    RSA, DSA and EC and a variety of key encodings (both PEM and DER).
    #22660
* fs
  * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If
    this option is set to true, non-existing parent folders will be
    automatically created. #21875
* http2
  * Added a `'ping'` event to `Http2Session` that is emitted whenever a
    non-ack `PING` is received.
    #23009
  * Added support for the `ORIGIN` frame.
    #22956
  * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect
    protocol support to allow use of WebSockets over HTTP/2.
    #23284
* module
  * Added `module.createRequireFromPath(filename)`. This new method can
    be used to create a custom require function that will resolve
    modules relative to the filename path.
    #19360
* process
  * Added a `'multipleResolves'` process event that is emitted whenever
    a `Promise` is attempted to be resolved multiple times, e.g. if the
    `resolve` and `reject` functions are both called in a `Promise`
    executor. #22218
* url
  * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These
    methods can be used to correctly convert between file: URLs and
    absolute paths. #22506
* util
  * Added the `sorted` option to `util.inspect()`. If set to `true`,
    all properties of an object and Set and Map entries will be sorted
    in the returned string. If set to a function, it is used as a
    compare function. #22788
  * The `util.instpect.custom` symbol is now defined in the global
    symbol registry as `Symbol.for('nodejs.util.inspect.custom')`.
    #20857
  * Added support for `BigInt` numbers in `util.format()`.
    #22097
* V8 API
  * A number of V8 C++ APIs have been marked as deprecated since they
    have been removed in the upstream repository. Replacement APIs
    are added where necessary. #23159
* Windows
  * The Windows msi installer now provides an option to automatically
    install the tools required to build native modules.
    #22645
* Workers
  * Debugging support for Workers using the DevTools protocol has been
    implemented. #21364
  * The public `inspector` module is now enabled in Workers.
    #22769
* Added new collaborators:
  * digitalinfinity - Hitesh Kanwathirtha

PR-URL: #23313
@XieJiSS
Copy link

XieJiSS commented Oct 20, 2018

nodejs/Release#369

Could it be possible to change the prompt (of the Boxstarter Script) into a Yes/No question? Some users may proceed without reading the text before.

i.e. Have you saved all your files and closed all open programs? (y/N)

@jasnell
Copy link
Member

jasnell commented Oct 30, 2018

@joaocgreis, et al, this appears to be causing some issues for some users... #23838

@tniessen
Copy link
Member

tniessen commented Nov 5, 2018

This also appears to cause nodejs/security-wg#439.

@tdaniely

This comment has been minimized.

@MPagel
Copy link

MPagel commented Nov 13, 2018

What happens if Visual Studio is already installed and one checks the box?

At least for python, it installs a new copy, ignoring the prior presence of it on the system. It also alters the path such that the new Py path is before or instead of the old install, requiring either manual adjustments to path or re-install of pip libraries needed for non-node.js python scripts.

refack added a commit to refack/node that referenced this pull request Nov 18, 2018
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: nodejs#24344
Refs: nodejs#22645
Refs: nodejs#23987
Refs: nodejs/Release#369
Refs: nodejs#23838
Refs: nodejs/security-wg#439
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@runewake2
Copy link

Can this be rolled back? It caused my PC to be added to a group policy, Windows Updates to be disabled, my PC to be hard restarted multiple times and littered my PC with a number of other permanent changes.

rvagg pushed a commit that referenced this pull request Nov 28, 2018
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install Issues and PRs related to the installers. semver-minor PRs that contain new features and should be released in the next minor version. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: make node on Windows install, or prompt users to get, windows-build-tools