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: do not use Boxstarter to install tools #24677

Closed
wants to merge 3 commits into
base: master
from

Conversation

@joaocgreis
Copy link
Member

joaocgreis commented Nov 27, 2018

The script to install Python and Visual Studio on Windows uses Boxstarter to reboot the machine whenever a reboot is pending and disable some Windows features that can make the installation fail. This has been causing issues for some users, leaving the machine in an unexpected state when the installation is cancelled.

This PR removes Boxstarter, changing the script to use Chocolatey directly, thus not changing machine configuration. This makes it possible for the installation to fail because of interference from updates or previous installations, but I could not make this happen in my tests with the packages installed here. Hence, I believe this is a good trade-off to fix the issues that have been happening.

Fixes: #23838
Refs: #24637

cc @nodejs/platform-windows @gep13

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
win: do not use Boxstarter to install tools
Use Chocolatey directly in the tools installation script.

Fixes: #23838
@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Nov 27, 2018

@joaocgreis sadly an error occured when I tried to trigger a build :(

@bzoz

bzoz approved these changes Nov 27, 2018

echo.
echo Please close all open programs for the duration of the installation.

This comment has been minimized.

@gep13

gep13 Nov 28, 2018

Is there a reason that this part has been removed? I think this advice is still valid.

This comment has been minimized.

@aduh95

aduh95 Nov 28, 2018

Contributor

Do you really close any program to install a software on Windows? I for one never do, and I haven't seen anyone do that...

This comment has been minimized.

@joaocgreis

joaocgreis Nov 28, 2018

Member

While in most case there are no issues, this is always good advice to make sure there is no interference. I'll add it back.

This comment has been minimized.

@pauby

pauby Dec 4, 2018

@aduh95 One of the issues we had raised several times on the Boxstarter repo is that users do not know that Boxstarter will be rebooting their machines and they lose their work.

echo.
echo Please close all open programs for the duration of the installation.
echo If the installation fails, please ensure Windows is fully updated and reboot
echo your computer before trying again.

This comment has been minimized.

@gep13

gep13 Nov 28, 2018

Can we clarify exactly what this means? Does this mean trigger the entire msi installation again? Can they get back to the prompt to select the Chocolatey installation? Or is this referring to manually triggering this .bat file?

@@ -36,7 +36,6 @@ echo result of a Chocolatey install. This acceptance occurs whether you know the
echo license terms or not. Read and understand the license terms of the packages
echo being installed and their dependencies prior to installation:
echo - https://chocolatey.org/packages/chocolatey
echo - https://chocolatey.org/packages/boxstarter
echo - https://chocolatey.org/packages/python2
echo - https://chocolatey.org/packages/visualstudio2017buildtools

This comment has been minimized.

@gep13

gep13 Nov 28, 2018

Based on the Chocolatey packages being installed, this one is no longer being directly installed, so I am assuming that it is coming in as a dependency. Since you are not listing all the dependent packages, I don't think it is right to explicitly call this one out.

@gep13

This comment has been minimized.

Copy link

gep13 commented Nov 28, 2018

@joaocgreis said...
his makes it possible for the installation to fail because of interference from updates or previous installations, but I could not make this happen in my tests with the packages installed here.

Can you elaborate on the tests that you did here?

Based on the installation test that I did here:

https://www.youtube.com/watch?v=gwFpXXIJTvs&t=117s

At least one reboot was required, which if not done, would likely have failed the subsequent package installation.

@joaocgreis

This comment has been minimized.

Copy link
Member

joaocgreis commented Nov 28, 2018

@gep13 thanks for your review, updated.

Can you elaborate on the tests that you did here?

I tested this on Windows 2008, 2012, 2016 and 10. I tested this on Windows 10 while system updates were installing. The installation always completed successfully.

It is possible that there is some software or some state a machine can be in that will make this fail in a way that Boxstarter would have prevented, that's why I used it before. What I see here is that the installations always succeed, but leave the machine with a reboot pending. Chocolatey already displays a clear warning in this case when it completes.

@joaocgreis

This comment has been minimized.

@gep13

This comment has been minimized.

Copy link

gep13 commented Nov 29, 2018

@joaocgreis said...
I tested this on Windows 2008, 2012, 2016 and 10.

Sounds good!

The potential for problems likely comes machines that don't have the Windows Updates installed. I am going to guess that all the machines that you tested on were reasonably new VM's that had Windows Updates installed recently. As a result, the Windows Updates that are dependent packages didn't actually do anything, as they were not required. This is an area where we might still see some reports, but hopefully fewer.

@gep13

gep13 approved these changes Nov 29, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 1, 2018

One more review on this would be good. @nodejs/platform-windows was already pinged, but I guess since this is pretty text-heavy and the text is, in a sense, documentation, /ping @nodejs/documentation

@joaocgreis

This comment has been minimized.

Copy link
Member

joaocgreis commented Dec 5, 2018

Thanks for the reviews!

Landed in 76afdff.

@joaocgreis joaocgreis closed this Dec 5, 2018

joaocgreis added a commit that referenced this pull request Dec 5, 2018

win: do not use Boxstarter to install tools
Use Chocolatey directly in the tools installation script.

PR-URL: #24677
Fixes: #23838
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

@joaocgreis joaocgreis referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

@Trott Trott added the notable-change label Dec 6, 2018

BridgeAR added a commit that referenced this pull request Dec 6, 2018

win: do not use Boxstarter to install tools
Use Chocolatey directly in the tools installation script.

PR-URL: #24677
Fixes: #23838
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

BridgeAR added a commit that referenced this pull request Dec 6, 2018

2018-12-06, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

win: do not use Boxstarter to install tools
Use Chocolatey directly in the tools installation script.

PR-URL: #24677
Fixes: #23838
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

win: do not use Boxstarter to install tools
Use Chocolatey directly in the tools installation script.

PR-URL: #24677
Fixes: #23838
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

win: do not use Boxstarter to install tools
Use Chocolatey directly in the tools installation script.

PR-URL: nodejs#24677
Fixes: nodejs#23838
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment