search

felixrieseberg / windows-build-tools

:package: Install C++ Build Tools for Windows using npm
MIT License
3.4k stars 236 forks source link

More MS collaboration #137

Open jdalton opened 6 years ago

jdalton commented 6 years ago

Hi @felixrieseberg!

Microsoft's Developer Ecosystem and Platform team is working to improve Node on Windows. I'm wondering what you're thoughts are on having more Microsoft collaboration in windows-build-tools and maybe even eventually moving windows-build-tools to the Microsoft org? Are there issues or hack-arounds Microsoft can help smooth over?

felixrieseberg commented 6 years ago

I'm a big fan of this – as windows-build-tools is used by more and more organizations, it could really use some stabilization. Seeing more help from Microsoft (and even interest to eventually move it) warms my heart.

The area that's most troubling to me right now is an accurate detection of a successful installation, including those cases where installation wasn't necessary. There are two problems here:

1) We're detecting success by comparing the installer's log files against a list of known strings. That's obviously terrible, especially since I don't know the full list of possible log entries. 2) We received tons of reports that the Visual Studio installer wouldn't quit, leading windows-build-tools to hang. We're now calling process.exit() once we think that installation succeeded, but man, that feels so bad.

joaocgreis commented 6 years ago

@felixrieseberg you might not have seen my comment at https://github.com/felixrieseberg/windows-build-tools/pull/84#issuecomment-418952650, I did not receive a notification so GitHub might not have triggered one at all. I'd like to know what issues you found with VS2017. It should be fully supported, so if some modules are having trouble I'd like to investigate. Thanks!

felixrieseberg commented 6 years ago

@joaocgreis I frankly forgot which modules exactly failed to build and wish I had written down what exactly motivated me to go back to 2015. If you already tried the popular modules and they build fine now, I'd be more than happy to move to 2017 as a default.

Do you want to make a PR? We could flip things, choose 2017 as a default, and offer a --vs2015 parameter.

joaocgreis commented 6 years ago

@felixrieseberg I'm working on the installation process issues you described above, I'll include moving to VS2017 as well.

joaocgreis commented 6 years ago

Some notes about the issues mentioned above:

1. Reading the log files to determine success: The VS2017 installer creates a new process group to run the actual installation and the executable we call exits right away. So, it's not possible to wait for the child process to exit and get the return code. Given this, reading the log file is not such a bad thing, but it's very tricky to get right.

Detecting the exit code is possible and is the way that Chocolatey uses, but it's also quite complex: https://github.com/jberezanski/ChocolateyPackages/blob/929af36e2fb349ac1f9f8477c096ad2a6f5d0764/chocolatey-visualstudio.extension/extensions/Wait-VSInstallerProcesses.ps1 . Duplicating this here might lead to more workarounds in the future, so using Chocolatey directly to install VS (or everything) might be a better approach. The downside is that Chocolatey has to be installed, and we might have to work around issues in Chocolatey instead - though this sounds better to me.

2. The installer not quitting: This seems to be related to the outdated completion string. There might be other causes, so the best way forward would be to solve the point above.

Martin-Pitt commented 6 years ago

I'm not sure if it's big deal or not, but it's nice when things aren't sketchy or when there are any legal gray areas. But just in case: https://github.com/Microsoft/nodejs-guidelines/issues/72

If you want it to be an officially supported path then please do so in writing.