Node Interactive Code & Learn issues on Windows #16278

digitalinfinity · 2017-10-18T02:58:45Z

Version: Node 9
Platform: Windows
Subsystem: build

During Code & Learn at Node Interactive North America, @mike-kaufman, @rachelnicole and others observed several issues by folks building and testing with Node Core on Windows. I wanted to open this issue to capture them and also maybe use this as a discussion point for other improvements.

vcbuild improvements

Lots of folks ran into issues that could have probably been improved by doing a small amount of validation and messaging in vcbuild. Common issues were that folks did not have the right version of python, msys2 or other dependencies in the path. Additionally, a couple folks ran into really slow build times because Windows Defender was turned on and did not have the right exclusions set.
Recommendation: Introduce an opinionated developer shell for Node on Windows that validates system configuration and dependency installations and configures the paths correctly

VS installation

Lots of folks thought they had Visual C++ installed but didn't since the VS Setup is confusing. While that feedback will be communicated back to the VS team, we could probably do better by making it easier to get the build tools.
Recommendation: Update the Windows build guide to point to https://www.npmjs.com/package/windows-build-tools/ as a mechanism for setting up the build. We can certainly point folks at Code & Learn to following those steps.

Other issues

A couple folks had build failures because zconf.h got renamed- git checkout -- fixed the issue but I don't know what the repro steps were that got them to that point. Calling it out here in case it rings a bell for anyone.
Someone ran into test flakiness in test\addons-napi\test_promise. This might have been fixed by Fix race condition bug in test, and show assertion failure values #16037

The text was updated successfully, but these errors were encountered:

digitalinfinity · 2017-10-18T02:59:43Z

cc @nodejs/platform-windows @nodejs/node-chakracore @mhdawson @yodurr

Trott · 2017-10-18T05:09:18Z

Here's another issue that came up twice, but only on Windows. Perhaps not related to build but certainly build-adjacent. Might be a bug in ESLint. Here it is:

After running the tests, lint failed because ESLint seems to (at least on some versions of Windows) mess up matching of paths in .eslintignore. This happens after running test/parallel/test-require-unicode.js. One of the tmp directories in test will contain a subdirectory called 中文目录. If the linter is then run, it will report three errors with a file in that directory even though .eslintignore instructs the linter to ignore the tmp directories. The problem is certainly with the unicode characters in the directory name, as changing the directory name to an ASCII name makes the problem go away.

refack · 2017-10-21T20:26:52Z

I can't recommend windows-build-tools. We could however formulate our own install script.

mike-kaufman · 2017-10-21T23:22:33Z

I can't recommend windows-build-tools.

Can you provide some details on why? At a cursory glance, it seems to do a lot of what's needed to build node core on windows.

refack · 2017-10-22T00:00:07Z

@mike-kaufman as @felixrieseberg defined it - it's a package that should not exist 😄

It does not behave like any other npm package, in that it installs several products to the user's computer, hence changes the global state, and is uninstallable.
It still installs "VS2015 Build Tools for C++" (when VS2017) and which is just barely supported by MS
It installs python in a non canonical location - good for uninstalling - bad for reuse
It installs multiple copies of the Windows SDK - one one is needed

I do acknowledge that it works for most users, but it was geared towards those who don't care about the compiler but just want node-gyp to work. That's not exactly the typical node code dev, and IMHO though we could create an automation script for the audiance, I'm not comfortable recommending WBT for that purpose.

refack · 2017-10-22T00:01:02Z

Also WBT does not install "Git for Windows" which is needed for running core's test suite.

refack · 2017-10-22T00:05:38Z

Recommendation: Introduce an opinionated developer shell for Node on Windows that validates system configuration and dependency installations and configures the paths correctly

That is something I like much more. it could:

Validate requirments
Suggest or install missing
Setup enviroment

mike-kaufman · 2017-10-22T16:34:39Z

+1 on an opinionated dev shell & a "download & install tools script". In my experience, the install of tools is easier to get right when it is local for your enlistment (ie, don't worry about sharing of python install). That said, this "local install" may not be possible if the tool installs modify registry & env vars.

gibfahn · 2017-10-22T17:41:33Z

vcbuild improvements

I can't speak for anyone else, but the biggest thing that stops me from trying to improve vcbuild.bat is the way it's written and structured. If we could progress #12310 (looks like reimplementing in Powershell is the most popular solution) then it'd be a lot easier for people to contribute.

digitalinfinity · 2017-10-24T21:49:20Z

@refack yes! that's exactly what I had in mind for an opinionated developer shell. Re using windows-build-tools, you make good points. However, I'm not sure what other alternatives are. I think the key requirement is a friction-free experience to have VS/tools set up exactly how Node-Core needs it. @yodurr had suggested a boxstarter script but I don't have a lot of experience around the pros and cons of that. Would love to hear other suggestions here.

Makes eslint exclude directories without enumerating their content PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

refack · 2017-10-26T22:48:39Z

However, I'm not sure what other alternatives are.

A portable option exists:

With VS2017's new layout, the C++ build tools could be xcopy installed.
The trick is installing Windows SDK, but that could be scripted.
Python and GfW can be xcopy installed.

A second alternative is just to run the 3 installers. In any case I believe checking the requirements are set up and respond with an informative message will cover 90% of cases.

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2017-11-03T23:30:55Z

Should this remain open at this point? I imagine "yes". If so, maybe we can get a checklist of concrete actions to take?

joyeecheung · 2017-11-09T01:36:46Z

@Trott

Here's another issue that came up twice, but only on Windows. Perhaps not related to build but certainly build-adjacent. Might be a bug in ESLint. Here it is:
After running the tests, lint failed because ESLint seems to (at least on some versions of Windows) mess up matching of paths in .eslintignore. This happens after running test/parallel/test-require-unicode.js. One of the tmp directories in test will contain a subdirectory called 中文目录. If the linter is then run, it will report three errors with a file in that directory even though .eslintignore instructs the linter to ignore the tmp directories. The problem is certainly with the unicode characters in the directory name, as changing the directory name to an ASCII name makes the problem go away.

Just bumped into this on Mac, this looks like a eslint bug. I was on v8.x-staging.

digitalinfinity · 2017-11-09T02:09:57Z

I think it still should remain open but I'm not sure about the concrete actions. Some of these are probably just feedback to Microsoft/VS but others are definitely Node-related. Would it fall under the purview of any working group? There probably needs to be more thinking done before producing a concrete list of items (and as an aside, I'm happy to participate in that process or help coordinate if needed)

Trott · 2017-11-14T23:21:19Z

Just bumped into this on Mac, this looks like a eslint bug. I was on v8.x-staging.

@joyeecheung Just a guess, but there's probably an issue with master using a leading . on tmp directories and v8x.-staging probably not. So if you switched from master and got lint errors on .tmp directory files, you probably were linting files that were put there by master. master has a different .eslintignore than v8.x-staging.

joyeecheung · 2017-11-15T02:50:37Z

@Trott Yes I think you're right, thanks!

Trott · 2017-11-15T04:13:45Z

I think it still should remain open but I'm not sure about the concrete actions. Some of these are probably just feedback to Microsoft/VS but others are definitely Node-related. Would it fall under the purview of any working group? There probably needs to be more thinking done before producing a concrete list of items (and as an aside, I'm happy to participate in that process or help coordinate if needed)

@digitalinfinity The appropriate working group (if any) will likely depend on the nature of the concrete actions. Maybe we should do our best to come up with a list of pain points, and then evaluate each one for which categories each one falls into:

concrete actions can be immediately identified
working group it can be referred to
feedback for Microsoft
further discussion/evaluation needed in this issue

digitalinfinity · 2017-11-16T01:52:30Z

Makes sense, thanks @Trott. I'll try and get a list of pain points put together and categorized in the way you suggest. I'll collate it from this thread, but if you have other threads that I should look at, I can do that too.

P.S: Re bootstrapping, @bzoz just opened #17046 which should help simplify the environment setup considerably!

bzoz · 2017-11-16T10:13:01Z

#17015 will help with vcbuild and missing/wrong version of Python

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The build breaks if there's a non-ASCII character on the path to the building directory. Ref: nodejs#16278 Ref: nodejs#14336

The build breaks if there's a non-ASCII character on the path to the building directory. PR-URL: #16735 Refs: #16278 Refs: #14336 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

cicorias · 2018-02-14T00:56:40Z

FYI @felixrieseberg has updated and addressed VS2017 aspects in https://www.npmjs.com/package/windows-build-tools/ . While still doing global things, it does help quite a bit. This isn't the final answer and in general Windows experience starting with the installer in #4603 need some love.

At least a "note" on the existence with a warning instead of full recommendation would help

The build breaks if there's a non-ASCII character on the path to the building directory. PR-URL: #16735 Refs: #16278 Refs: #14336 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

bzoz · 2018-03-21T12:51:34Z

#17015 landed so we now have nicer error reporting. We also have Boxstarer script for easy installation of all needed tools: https://github.com/nodejs/node/pull/17046/files.

Is there anything else to be done here?

BridgeAR · 2018-04-22T17:41:09Z

I am closing this as I guess it is resolved by now. If we run into new problems later on, we can always open it again.

The build breaks if there's a non-ASCII character on the path to the building directory. PR-URL: nodejs#16735 Refs: nodejs#16278 Refs: nodejs#14336 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

joyeecheung added build Issues and PRs related to build files or the CI. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. windows Issues and PRs related to the Windows platform. labels Oct 18, 2017

refack added the meta Issues and PRs related to the general management of the project. label Oct 21, 2017

refack mentioned this issue Oct 21, 2017

Tweak esignore #16372

Merged

2 tasks

Trott mentioned this issue Oct 28, 2017

CommonFixturesFix - Fixed references to common.fixturesDir in test-tl… #16041

Closed

4 tasks

mmarchini mentioned this issue Nov 4, 2017

docs: warn users about non-ASCII paths on build #16735

Closed

2 tasks

bzoz mentioned this issue Nov 16, 2017

tools: add Boxstarter script #17046

Closed

2 tasks

mmarchini pushed a commit to mmarchini/node that referenced this issue Dec 12, 2017

docs: warn users about non-ASCII paths on build

0e90f1a

The build breaks if there's a non-ASCII character on the path to the building directory. Ref: nodejs#16278 Ref: nodejs#14336

BridgeAR closed this as completed Apr 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node Interactive Code & Learn issues on Windows #16278

Node Interactive Code & Learn issues on Windows #16278

digitalinfinity commented Oct 18, 2017

digitalinfinity commented Oct 18, 2017

Trott commented Oct 18, 2017

refack commented Oct 21, 2017

mike-kaufman commented Oct 21, 2017

refack commented Oct 22, 2017

refack commented Oct 22, 2017

refack commented Oct 22, 2017

mike-kaufman commented Oct 22, 2017

gibfahn commented Oct 22, 2017

digitalinfinity commented Oct 24, 2017

refack commented Oct 26, 2017

Trott commented Nov 3, 2017

joyeecheung commented Nov 9, 2017 •

edited

Loading

digitalinfinity commented Nov 9, 2017

Trott commented Nov 14, 2017 •

edited

Loading

joyeecheung commented Nov 15, 2017

Trott commented Nov 15, 2017

digitalinfinity commented Nov 16, 2017

bzoz commented Nov 16, 2017

cicorias commented Feb 14, 2018

bzoz commented Mar 21, 2018

BridgeAR commented Apr 22, 2018

Node Interactive Code & Learn issues on Windows #16278

Node Interactive Code & Learn issues on Windows #16278

Comments

digitalinfinity commented Oct 18, 2017

vcbuild improvements

VS installation

Other issues

digitalinfinity commented Oct 18, 2017

Trott commented Oct 18, 2017

refack commented Oct 21, 2017

mike-kaufman commented Oct 21, 2017

refack commented Oct 22, 2017

refack commented Oct 22, 2017

refack commented Oct 22, 2017

mike-kaufman commented Oct 22, 2017

gibfahn commented Oct 22, 2017

digitalinfinity commented Oct 24, 2017

refack commented Oct 26, 2017

Trott commented Nov 3, 2017

joyeecheung commented Nov 9, 2017 • edited Loading

digitalinfinity commented Nov 9, 2017

Trott commented Nov 14, 2017 • edited Loading

joyeecheung commented Nov 15, 2017

Trott commented Nov 15, 2017

digitalinfinity commented Nov 16, 2017

bzoz commented Nov 16, 2017

cicorias commented Feb 14, 2018

bzoz commented Mar 21, 2018

BridgeAR commented Apr 22, 2018

joyeecheung commented Nov 9, 2017 •

edited

Loading

Trott commented Nov 14, 2017 •

edited

Loading