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: add prompt to tools installation script #23987

Closed

Conversation

joaocgreis
Copy link
Member

Since the script to install tools for native modules (#22645) was included in releases, we've got several comments from users surprised by reboots.

This adds a more visible warning and a prompt to lessen the probability of users skipping ahead without reading.

Fixes: nodejs/Release#369

cc @nodejs/platform-windows

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

@joaocgreis joaocgreis added windows Issues and PRs related to the Windows platform. install Issues and PRs related to the installers. tools Issues and PRs related to the tools directory. dont-land-on-v6.x labels Oct 31, 2018
@joaocgreis
Copy link
Member Author

joaocgreis commented Oct 31, 2018

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/18252/ (EDIT: Yellow, one flaky test failed.)

echo Please type YES followed by enter to confirm that you have saved all your
set /p "ACCEPT_PROMPT=work and closed all open programs: "
if /i not "%ACCEPT_PROMPT%"=="yes" (
echo Please type YES or close the window to exit.

This comment was marked as outdated.

@joaocgreis
Copy link
Member Author

@jdalton thanks! Updated.

@ferventcoder
Copy link
Contributor

I like the type yes to confirm, but this does not mention the UAC bit - I would like to see the wording from #24000 with this yes prompt

@ferventcoder
Copy link
Contributor

Also I missed this was here on initial search and filed my own PR - apologies on that. We would like the wording and notes from the other PR brought into here to address that UAC will be disabled and make the user aware of what they will need to do should they stop the process instead of letting it finish

ferventcoder and others added 3 commits October 31, 2018 21:31
Clarify the behavior of what Boxstarter may do when it runs on a box
to install all the necessary tools so that there are no surprises to
the end user when the script is run.

Currently there is no interface that warns the user that Boxstarter
will reboot the machine possibly multiple times depending on how many
dependencies need to be installed and doesn't mention a need to disable
UAC. For folks who see what may look like a reboot loop, we feel it is
necessary to make them aware that UAC will be disabled and they will
need to take action to re-enable UAC manually if they interfere/stop
the script from finishing.
@joaocgreis joaocgreis force-pushed the joaocgreis-IAV-tools-prompt branch from 300f228 to 23c02c0 Compare October 31, 2018 21:54
@joaocgreis
Copy link
Member Author

@ferventcoder your PR adds a lot of good info, thanks for it! I included it here, let me know if this was not what you had in mind. I took the opportunity to change the commit message (subsystem and 72-col lines) and added a fixup addressing feedback from @Trott. I also moved my prompt to the new screen you created. Let me know if there's any change you'd like to make.

@refack
Copy link
Contributor

refack commented Oct 31, 2018

@ferventcoder
Copy link
Contributor

@joaocgreis looks great, thank you!

echo in fact it is just a normal Windows Updates reboot cycle.
echo.
echo If this is not what you would like to occur, you can close this window
echo to stop now.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can pull these two lines out since you reference this below.

echo Your computer may REBOOT SEVERAL TIMES WITHOUT FURTHER WARNING.
echo Please type YES followed by enter to confirm that you have saved all your
set /p "ACCEPT_PROMPT=work and closed all open programs: "
if /i not "%ACCEPT_PROMPT%"=="yes" (
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this handle case sensitivity? You are asking for "YES", and this accepts "yes"

Copy link
Member Author

Choose a reason for hiding this comment

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

The /i switch is for ignoring case. I used uppercase above to make it obvious that only yes needs to be typed, but the user can type in any case.

@joaocgreis
Copy link
Member Author

@ferventcoder thanks, updated!

@jdalton this changed a lot since you reviewed. I'll assume your approval still holds, please let me know if not.

@joaocgreis
Copy link
Member Author

If there are no objections, I will land this tomorrow.

Full CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/18291/

@MylesBorins
Copy link
Contributor

@refack can you open a revert against v10.x-staging

@targos
Copy link
Member

targos commented Nov 14, 2018

@MylesBorins: #24344

targos pushed a commit that referenced this pull request Nov 15, 2018
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136
targos pushed a commit that referenced this pull request Nov 15, 2018
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

PR-URL: #24350
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 <reis@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Clarify the behavior of what Boxstarter may do when it runs on a box
to install all the necessary tools so that there are no surprises to
the end user when the script is run.

Currently there is no interface that warns the user that Boxstarter
will reboot the machine possibly multiple times depending on how many
dependencies need to be installed and doesn't mention a need to disable
UAC. For folks who see what may look like a reboot loop, we feel it is
necessary to make them aware that UAC will be disabled and they will
need to take action to re-enable UAC manually if they interfere/stop
the script from finishing.

PR-URL: #23987
Fixes: nodejs/Release#369
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Fixes: nodejs/Release#369

PR-URL: #23987
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <reis@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Clarify the behavior of what Boxstarter may do when it runs on a box
to install all the necessary tools so that there are no surprises to
the end user when the script is run.

Currently there is no interface that warns the user that Boxstarter
will reboot the machine possibly multiple times depending on how many
dependencies need to be installed and doesn't mention a need to disable
UAC. For folks who see what may look like a reboot loop, we feel it is
necessary to make them aware that UAC will be disabled and they will
need to take action to re-enable UAC manually if they interfere/stop
the script from finishing.

PR-URL: #23987
Fixes: nodejs/Release#369
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Fixes: nodejs/Release#369

PR-URL: #23987
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
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 <reis@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Nov 29, 2018
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. 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.

None yet