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 Boxstarter from elevated shell #22988

Closed

Conversation

joaocgreis
Copy link
Member

This fixes the issue described in #22645 (comment) (cc @targos).

Boxstarter asks for elevation to install packages, but not to install Boxstarter itself. If Boxstarter is already installed, the command succeeds with or without elevation. This PR changes the batch file to run all the commands from an elevated PowerShell.

LTS and semver: This is a bug fix for #22645, which is currently only in master. That PR is semver-minor without a strong case for backporting, so this one should not be backported as well even if this is semver-patch.

cc @nodejs/platform-windows @richardlau

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

Boxstarter asks for elevation to install packages, but not to install
Boxstarter itself. Thus, run all the commands from an elevated
PowerShell.

Refs: nodejs#22645
@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 Sep 21, 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 21, 2018
@joaocgreis
Copy link
Member Author

@targos
Copy link
Member

targos commented Sep 21, 2018

@targos
Copy link
Member

targos commented Sep 21, 2018

I tested it and it works, nice!
Rubberstamp LGTM on the actual change.

@tniessen
Copy link
Member

This does not permamently install Boxstart itself, just the required packages, right?

@targos
Copy link
Member

targos commented Sep 21, 2018

It permanently installs the Boxstarter shell

@tniessen
Copy link
Member

Is that visible to the user prior to the installation? The MSI dialog window does not seem to say so.

@joaocgreis
Copy link
Member Author

@tniessen there's a mention of Boxstarter in the shell text before installation. This installs Chocolatey and Boxstarter as system tools (which are a good thing to have I believe). The most visible part is the Boxstarter Shell that appears in the desktop. Do you think that's an issue? I could add a hackish workaround for that, but I'd rather not unless it proves to be a problem. There are links for manual steps for people that want more control over the installation.

tniessen added a commit to tniessen/node that referenced this pull request Sep 21, 2018
Currently, the installation wizard more or less silently installs
third-party software (Boxstarter + Chocolatey). This adds some text
to the MSI installation dialog and to the Boxstarter installation
script.

Refs: nodejs#22645
Refs: nodejs#22988
@tniessen
Copy link
Member

This installs Chocolatey and Boxstarter as system tools (which are a good thing to have I believe)

I agree, but it should be obvious to the user that that is going to happen, and I don't think it currently is. I opened #23003 to deal with that.

This discussion is unrelated to this PR though, the change itself LGTM.

@addaleax
Copy link
Member

@tniessen @joaocgreis Is this ready for landing? It seems that way but I’m super unfamiliar with the subject matter so I don’t feel comfortable pushing the button just yet. :)

@tniessen
Copy link
Member

I'd think so, but I cannot actually test the installer right now, would be great if someone else from @nodejs/platform-windows could confirm.

tniessen added a commit that referenced this pull request Sep 24, 2018
Currently, the installation wizard more or less silently installs
third-party software (Boxstarter + Chocolatey). This adds some text
to the MSI installation dialog and to the Boxstarter installation
script.

PR-URL: #23003
Refs: #22645
Refs: #22988
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: João Reis <[email protected]>
@joaocgreis
Copy link
Member Author

This is ready to land. Both @targos and I tested this, I tested on Windows 7, 2012R2 and 10. It is possible (or even likely) that some program out there or some configuration will make the installation fail, so more testing would always be welcome. (I believe that using Chocolatey/Boxstarter is both the simplest way and the one that will cause less issues, but installing these tools is hard.)

I will land this tomorrow if no one gets to it first or finds any issue.

joaocgreis added a commit that referenced this pull request Sep 26, 2018
Boxstarter asks for elevation to install packages, but not to install
Boxstarter itself. Thus, run all the commands from an elevated
PowerShell.

Refs: #22645
PR-URL: #22988
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joaocgreis
Copy link
Member Author

Landed in 0461dd9

Thanks for your reviews @targos @tniessen @jasnell !

@joaocgreis joaocgreis closed this Sep 26, 2018
targos pushed a commit that referenced this pull request Sep 27, 2018
Currently, the installation wizard more or less silently installs
third-party software (Boxstarter + Chocolatey). This adds some text
to the MSI installation dialog and to the Boxstarter installation
script.

PR-URL: #23003
Refs: #22645
Refs: #22988
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: João Reis <[email protected]>
targos pushed a commit that referenced this pull request Sep 27, 2018
Boxstarter asks for elevation to install packages, but not to install
Boxstarter itself. Thus, run all the commands from an elevated
PowerShell.

Refs: #22645
PR-URL: #22988
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[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. 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.

6 participants