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_main: use ERROR_EXE_MACHINE_TYPE_MISMATCH instead 1 as exit code #8204

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@yorkie
Member

yorkie commented Aug 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

windows

Description of change

If IsWindows7OrGreater() returns false, the Node.js program should
exit with a more specific code ERROR_EXE_MACHINE_TYPE_MISMATCH instead
the code 0x1(ERROR_INVALID_FUNCTION).

@yorkie yorkie added the windows label Aug 21, 2016

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 21, 2016

System Error Codes in Windows is here.

@addaleax

View changes

src/node_main.cc Outdated
@@ -7,7 +7,7 @@ int wmain(int argc, wchar_t *wargv[]) {
if (!IsWindows7OrGreater()) {
fprintf(stderr, "This application is only supported on Windows 7, "
"Windows Server 2008 R2, or higher.");
exit(1);
exit(0xd8); // code: ERROR_EXE_MACHINE_TYPE_MISMATCH

This comment has been minimized.

@addaleax

addaleax Aug 21, 2016

Member

I think you can also #include <windows.h> above and then use the macro directly, if you prefer.

This comment has been minimized.

@yorkie

yorkie Aug 21, 2016

Member

@addaleax thank for your suggestion, windows.h has not been used yet in other sources, such that including windows.h here would increase the building time because:

It defines a very large number of Windows specific functions that can be used in C.

I'm not sure if we should include that file for a simple macro?

This comment has been minimized.

@addaleax

addaleax Aug 21, 2016

Member

@yorkie ¯_(ツ)_/¯ … I doubt that it would make a huge difference.

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 21, 2016

Two nits, the commit message should probably use src: as the subsystem, and its subject line shortened to no more than 50 characters. LGTM with that (with or without the header).

CI: https://ci.nodejs.org/job/node-test-commit/4697/

@yorkie yorkie force-pushed the yorkie:windows/improve-exit-code branch 2 times, most recently Aug 21, 2016

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 21, 2016

@addaleax Fix nits, BTW the CI seems failed :(

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 22, 2016

Well, new CI run then just to be sure: https://ci.nodejs.org/job/node-test-commit/4709/

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 24, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 24, 2016

@nodejs/platform-windows ... I'm not too familiar with how these codes are typically used on Windows. Is this a fairly innocuous change or is there the possibility of breaking things? (that is, is this a semver-patch or semver-major?)

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 24, 2016

I think this should be marked as semver-major, because it actually changes the interface for user-land even though we have not documented yet.

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 24, 2016

By the way, the CI failed on arm-fanned with https://ci.nodejs.org/job/node-test-binary-arm/3476/RUN_SUBSET=1,label=pi1-raspbian-wheezy/console, I'm not sure if this is related with this patch, cc @nodejs/build :(

@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Aug 25, 2016

@yorkie Can you include just WinError.h and use the macro?

The commit message is still too long. What about something like src,win: use correct exit code in old versions ?

I confirm the CI failure is unrelated.

@yorkie yorkie force-pushed the yorkie:windows/improve-exit-code branch 2 times, most recently Aug 25, 2016

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 25, 2016

@joaocgreis Done, thanks for your tip and confirmation :)

@joaocgreis

This comment has been minimized.

Member

joaocgreis commented Aug 25, 2016

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 28, 2016

Ping @nodejs/ctc because this has been marked as major :)

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 28, 2016

Still LGTM :)

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 28, 2016

Could someone please confirm if the CI#3829 is green or that not related with this change?

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 29, 2016

That CI was green with only known flaky failures on AIX.

src,win: use correct exit code in old versions
If `IsWindows7OrGreater()` returns `false`, the Node.js program should
exit with a more specific code ERROR_EXE_MACHINE_TYPE_MISMATCH instead
the code 0x1(ERROR_INVALID_FUNCTION)

PR-URL: #8204
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@yorkie yorkie force-pushed the yorkie:windows/improve-exit-code branch to a080f1a Aug 29, 2016

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 29, 2016

Ok @addaleax, could we land this now as a080f1a?

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 29, 2016

@yorkie I see no reason not to :)

(If anything I said came over as meaning that this would be blocked by me, that was definitely not my intent 😄)

yorkie added a commit that referenced this pull request Aug 30, 2016

src,win: use correct exit code in old versions
If `IsWindows7OrGreater()` returns `false`, the Node.js program should
exit with a more specific code ERROR_EXE_MACHINE_TYPE_MISMATCH instead
the code 0x1(ERROR_INVALID_FUNCTION)

PR-URL: #8204
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@yorkie

This comment has been minimized.

Member

yorkie commented Aug 30, 2016

Landed in a3c5567 and never mind, just confirmation if this commit is completely right :)

@yorkie yorkie closed this Aug 30, 2016

@yorkie yorkie deleted the yorkie:windows/improve-exit-code branch Aug 30, 2016

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment