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

Node 11 won't run on Windows Nano Server #25843

Closed
bterlson opened this issue Jan 31, 2019 · 8 comments · Fixed by #25900
Closed

Node 11 won't run on Windows Nano Server #25843

bterlson opened this issue Jan 31, 2019 · 8 comments · Fixed by #25900
Labels
report Issues and PRs related to process.report.

Comments

@bterlson
Copy link

  • Version: Node 11
  • Platform: Windows Nano Server

#22712 introduced a dependency on NETAPI32.dll’s NetApiBufferFree and NetServerGetInfo which are not present on Windows Nano Server, and as a result, Node crashes on launch with STATUS_DLL_NOT_FOUND.

Looking at the code in #22712, I can suggest alternative dependencies (admittedly based on limited information/understanding): use GetVersionEx to get OS information, and use GetComputerNameEx to get the machine name.

Both GetVersionEx and GetComputerNameEx are guaranteed to be available on all versions of Windows and so should be preferred over other alternatives (including GetComputerName, which seems to be used in the fallback codepath in #22712).

Please let me know if I can help further with getting this issue resolved!

@bterlson bterlson changed the title Node 11 won't run on Windows Node 11 won't run on Windows Nano Server Jan 31, 2019
@gireeshpunathil
Copy link
Member

@bterlson - thanks for reporting this. Don't know our support statement around Nano server. In addition, either the CI or I have system to test this . Do you want to go for a PR?

@gireeshpunathil gireeshpunathil added the report Issues and PRs related to process.report. label Jan 31, 2019
@bterlson
Copy link
Author

I would love to PR, but it would take me a lot of time that I don't have right now 😂 If necessary I can poke around here and see if I can find someone to take on this work?

I think the support statement for Windows Nano Server is somewhat irrelevant since we can fix this in a way that "just works" across all Windows versions. So I think you don't really even need CI beyond your normal Windows CI - if you replace the Net* calls with GetVersionEx and GetComputerNameEx and things pass in normal Windows CI you can have pretty high confidence this will work on every Windows.

That said, I am happy to help testing bits on nano server to validate this issue is fixed, and longer term I can help get nano server under CI because, long term, I'd like to see that happen 😄

/cc @bmeck who I was discussing this with earlier

@addaleax
Copy link
Member

Fwiw, I’ve mentioned this somewhere, but I’d love it if we could replace the platform-specific code with uv_os_uname as much as possible?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 31, 2019

I'm planning to work on that this weekend. There is a change I need to make in libuv first though.

@jstarks
Copy link

jstarks commented Jan 31, 2019

It looks like uv_os_uname doesn't include the hostname, while the current code does query it. Is there a libuv function that includes this?

Otherwise the current implementation of uv_os_uname would certainly work fine on Nano Server containers.

@addaleax
Copy link
Member

@jstarks I think uv_os_gethostname might do the trick :)

@bterlson
Copy link
Author

@cjihrig I'll be around this weekend if you need help! I'll get a nano server VM up and running in anticipation.

cjihrig added a commit to cjihrig/libuv that referenced this issue Feb 2, 2019
Currently, on Windows the uv_utsname_t's version field can
be an empty string if no service packs are installed. This isn't
very helpful, and a lot more information is available in the
Windows registry. This commit prepends the full product name
to the existing service pack information.

Refs: nodejs/node#25843
Refs: libuv#2128 (comment)
@cjihrig
Copy link
Contributor

cjihrig commented Feb 2, 2019

@bterlson see libuv/libuv#2170. If we take that approach, we should be able to remove the problematic code mentioned in the OP.

cjihrig added a commit to cjihrig/node that referenced this issue Feb 5, 2019
PR-URL: nodejs#25900
Fixes: nodejs#25843
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
cjihrig added a commit to cjihrig/libuv that referenced this issue Feb 5, 2019
Currently, on Windows the uv_utsname_t's version field can
be an empty string if no service packs are installed. This isn't
very helpful, and a lot more information is available in the
Windows registry. This commit prepends the full product name
to the existing service pack information.

Refs: nodejs/node#25843
Refs: libuv#2128 (comment)
addaleax pushed a commit that referenced this issue Feb 6, 2019
PR-URL: #25900
Fixes: #25843
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
cjihrig added a commit to cjihrig/libuv that referenced this issue Feb 7, 2019
Currently, on Windows the uv_utsname_t's version field can
be an empty string if no service packs are installed. This isn't
very helpful, and a lot more information is available in the
Windows registry. This commit prepends the full product name
to the existing service pack information.

Refs: nodejs/node#25843
Refs: libuv#2128 (comment)
PR-URL: libuv#2170
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
njlr pushed a commit to buckaroo-pm/libuv that referenced this issue Apr 5, 2019
Currently, on Windows the uv_utsname_t's version field can
be an empty string if no service packs are installed. This isn't
very helpful, and a lot more information is available in the
Windows registry. This commit prepends the full product name
to the existing service pack information.

Refs: nodejs/node#25843
Refs: libuv#2128 (comment)
PR-URL: libuv#2170
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants