win: allow skipping the supported platform check #33176

joaocgreis · 2020-04-30T20:38:39Z

This is an alternative to #33108, which allows skipping the supported platform check by setting an environment variable. This works when Node is executed as a subprocess, such as when running npm/node-gyp, unless the variable is explicitly filtered out.

The MSI is still blocked on Windows 7, the exe or packaged version should be used to run on that system.

I don't think we should land this on LTS branches right away, but no strong opinion. LTS versions might run without issues until their EOL, so I'm inclined to not land this there for now.

cc @nodejs/platform-windows

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-04-30T20:40:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31121/

joaocgreis · 2020-04-30T20:40:49Z

Test build: https://nodejs.org/download/test/v15.0.0-test20200430ccd18343cb/

jasnell · 2020-04-30T21:08:06Z

I'm not a fan of doing this via environment variable given that it would likely impact more than just a single process. I'd much rather the command line argument approach.

joaocgreis · 2020-05-01T02:35:37Z

@jasnell how would the command line argument be passed to child processes and Node started from scripts? Or do you think users should find workarounds in these cases?

jasnell · 2020-05-01T12:49:37Z

@joaocgreis ... I won't block this because the cases you mention are definitely difficult, but I'd certainly prefer users find workarounds.

cjihrig · 2020-05-01T13:05:06Z

how would the command line argument be passed to child processes and Node started from scripts?

The de facto answer to that lately seems to be to use NODE_OPTIONS - everything is added as a CLI flag, but can be set via environment variable.

xCykrix · 2020-05-06T03:08:05Z

I'll post this in both places so it is seen by everyone, but if this is what ends up being favored I will gladly let this to be accepted over the resolution I have regarding just putting warnings in place. Both solve the issue at hand, but given the nature of this; it needs to be met under an agreement or a vote in the end because it may set future policies for deprecation with Windows. The command-line flag alone is not really viable due to the fact that child processes will not execute correctly, so either a mutable warning or environment variables are about the only options I can see resolving this with minimal breakage of features.

mhdawson · 2020-05-06T20:29:45Z

The other thought I had is that for a given environment/machine you either want Node.js to run on windows 7 or not. @jasnell I understand the general concern but in this case what is the down side for it applying to multiple processes?

tjrgg · 2020-05-17T20:17:46Z

Now that the TSC has expressed that they'd be happy with this PR (nodejs/TSC#866), any chance this could be merged soon?

doc/api/cli.md

src/node_main.cc

Fixes: nodejs#33034

joaocgreis · 2020-05-21T03:25:17Z

@jasnell thanks, updated.

nodejs-github-bot · 2020-05-21T14:15:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31463/

mhdawson

LGTM

sam-github · 2020-05-21T21:16:46Z

These were tagged TSC agenda, but it looks like consensus was found, and there isn't anything to discuss?

doc/api/cli.md

xCykrix · 2020-05-22T05:09:44Z

+1 from me; I went ahead and closed the issue and pull request I created as this will resolve them both, just to prevent any confusion. 🙂

For Reference: #33034 #33108

gireeshpunathil · 2020-05-22T05:17:34Z

src/node_main.cc

+      !(IsWindowsServer() && IsWindows8OrGreater()) &&
+      (GetEnvironmentVariableA(SKIP_CHECK_VAR, buf, sizeof(buf)) !=
+       SKIP_CHECK_SIZE ||
+       strncmp(buf, SKIP_CHECK_VALUE, SKIP_CHECK_SIZE + 1) != 0)) {
    fprintf(stderr, "This application is only supported on Windows 8.1, "


Suggested change

fprintf(stderr, "This application is only supported on Windows 8.1, "

fprintf(stderr, "Node.js is only supported on Windows 8.1, "

Otherwise it brings a confusion to the user: which application? the one that runs on Node.js, Is it coming from a third party module, etc.

(I know this word not touched by this PR, but given that we are changing the behavior here, it makes sense to reduce confusion)

BridgeAR

LGTM % comments

joaocgreis · 2020-05-26T15:40:49Z

Updated.

@jasnell can you take a look at node.1?

nodejs-github-bot · 2020-05-26T15:41:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31566/

joaocgreis · 2020-05-28T14:42:33Z

The last changes here have not been reviewed, it would be good to have a review before landing (or confirmation from one of the reviewers above). Thanks!

BridgeAR

Still LGTM

BridgeAR · 2020-05-30T01:54:47Z

Landed in 2c0a4fa

Fixes: nodejs#33034 PR-URL: nodejs#33176 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

TheJaredWilcurt · 2020-06-18T12:52:31Z

This is unfortunate. Windows 7 is still used by 20% of computers. This choice will disproportionately impact lower income and those in developing countries, and may cause major security problems for those that cannot update dependencies due to them requiring a version of Node that won't run on that their machine. Based on usage statistics 7 is in decline (33% down to 20% in the last 12 months), but the lower its numbers get, the slower the decline will become. Windows XP usage is about equivalent with ChromeOS usage even today (though likely only from business devices not used by individuals but by groups, like ATMs, kiosks, medical equipment, etc.). But 7 is still in use by actual people. Hopefully the reduced effort of supporting it means greater advancement in Node to offset these negative effects.

Fixes: #33034 PR-URL: #33176 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #33376 cli: * (SEMVER-MINOR) add alias for report-directory to make it consistent (AshCripps) #33587 crypto: * (SEMVER-MINOR) allow KeyObjects in postMessage (Tobias Nießen) #33360 deps: * (SEMVER-MINOR) V8: cherry-pick 0d6debcc5f08 (Michaël Zasso) #33376 * (SEMVER-MINOR) update V8 to 8.3.110.9 (Michaël Zasso) #33376 dgram: * (SEMVER-MINOR) allow typed arrays in .send() (Sarat Addepalli) #22413 events: * (SEMVER-MINOR) initial implementation of experimental EventTarget (James M Snell) #33556 fs: * (SEMVER-MINOR) implement lutimes (Maël Nison) #33399 http: * (SEMVER-MINOR) expose host and protocol on ClientRequest (wenningplus) #33803 * (SEMVER-MINOR) add maxTotalSockets to agent class (rickyes) #33617 * (SEMVER-MINOR) return this from OutgoingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from ClientRequest#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from IncomingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) added scheduling option to http agent (delvedor) #33278 http2: * (SEMVER-MINOR) return this for Http2ServerRequest#setTimeout (Pranshu Srivastava) #33994 * (SEMVER-MINOR) do not modify explicity set date headers (Pranshu Srivastava) #33160 process: * (SEMVER-MINOR) add unhandled-rejection throw and warn-with-error-code (Dan Fabulich) #33475 src: * (SEMVER-MINOR) store key data in separate class (Tobias Nießen) #33360 * (SEMVER-MINOR) add NativeKeyObject base class (Tobias Nießen) #33360 * (SEMVER-MINOR) rename internal key handles to KeyObjectHandle (Tobias Nießen) #33360 * (SEMVER-MINOR) add equality operators for BaseObjectPtr (Anna Henningsen) #33772 * (SEMVER-MINOR) introduce BaseObject base FunctionTemplate (Anna Henningsen) #33772 * (SEMVER-MINOR) add public APIs to manage v8::TracingController (Anna Henningsen) #33850 win: * (SEMVER-MINOR) allow skipping the supported platform check (João Reis) #33176 worker: * (SEMVER-MINOR) add public method for marking objects as untransferable (Anna Henningsen) #33979 * (SEMVER-MINOR) emit `'messagerror'` events for failed deserialization (Anna Henningsen) #33772 * (SEMVER-MINOR) allow passing JS wrapper objects via postMessage (Anna Henningsen) #33772 * (SEMVER-MINOR) allow transferring/cloning generic BaseObjects (Anna Henningsen) #33772 worker,fs: * (SEMVER-MINOR) make FileHandle transferable (Anna Henningsen) #33772 zlib: * (SEMVER-MINOR) add `maxOutputLength` option (unknown) #33516 PR-URL: #34093

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #33376 cli: * (SEMVER-MINOR) add alias for report-directory to make it consistent (AshCripps) #33587 crypto: * (SEMVER-MINOR) allow KeyObjects in postMessage (Tobias Nießen) #33360 deps: * (SEMVER-MINOR) V8: cherry-pick 0d6debcc5f08 (Michaël Zasso) #33376 * (SEMVER-MINOR) update V8 to 8.3.110.9 (Michaël Zasso) #33376 dgram: * (SEMVER-MINOR) allow typed arrays in .send() (Sarat Addepalli) #22413 events: * (SEMVER-MINOR) initial implementation of experimental EventTarget (James M Snell) #33556 fs: * (SEMVER-MINOR) implement lutimes (Maël Nison) #33399 http: * (SEMVER-MINOR) expose host and protocol on ClientRequest (wenningplus) #33803 * (SEMVER-MINOR) add maxTotalSockets to agent class (rickyes) #33617 * (SEMVER-MINOR) return this from OutgoingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from ClientRequest#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from IncomingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) added scheduling option to http agent (delvedor) #33278 http2: * (SEMVER-MINOR) return this for Http2ServerRequest#setTimeout (Pranshu Srivastava) #33994 * (SEMVER-MINOR) do not modify explicity set date headers (Pranshu Srivastava) #33160 process: * (SEMVER-MINOR) add unhandled-rejection throw and warn-with-error-code (Dan Fabulich) #33475 src: * (SEMVER-MINOR) store key data in separate class (Tobias Nießen) #33360 * (SEMVER-MINOR) add NativeKeyObject base class (Tobias Nießen) #33360 * (SEMVER-MINOR) rename internal key handles to KeyObjectHandle (Tobias Nießen) #33360 * (SEMVER-MINOR) add equality operators for BaseObjectPtr (Anna Henningsen) #33772 * (SEMVER-MINOR) introduce BaseObject base FunctionTemplate (Anna Henningsen) #33772 * (SEMVER-MINOR) add public APIs to manage v8::TracingController (Anna Henningsen) #33850 stream*: * runtime deprecate Transform._transformState (Robert Nagy) #32763 win: * (SEMVER-MINOR) allow skipping the supported platform check (João Reis) #33176 worker: * (SEMVER-MINOR) add public method for marking objects as untransferable (Anna Henningsen) #33979 * (SEMVER-MINOR) emit `'messagerror'` events for failed deserialization (Anna Henningsen) #33772 * (SEMVER-MINOR) allow passing JS wrapper objects via postMessage (Anna Henningsen) #33772 * (SEMVER-MINOR) allow transferring/cloning generic BaseObjects (Anna Henningsen) #33772 worker,fs: * (SEMVER-MINOR) make FileHandle transferable (Anna Henningsen) #33772 zlib: * (SEMVER-MINOR) add `maxOutputLength` option (unknown) #33516 PR-URL: #34093

Fixes: #33034 PR-URL: #33176 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #33376 cli: * (SEMVER-MINOR) add alias for report-directory to make it consistent (AshCripps) #33587 crypto: * (SEMVER-MINOR) allow KeyObjects in postMessage (Tobias Nießen) #33360 deps: * (SEMVER-MINOR) V8: cherry-pick 0d6debcc5f08 (Michaël Zasso) #33376 * (SEMVER-MINOR) update V8 to 8.3.110.9 (Michaël Zasso) #33376 dgram: * (SEMVER-MINOR) allow typed arrays in .send() (Sarat Addepalli) #22413 events: * (SEMVER-MINOR) initial implementation of experimental EventTarget (James M Snell) #33556 fs: * (SEMVER-MINOR) implement lutimes (Maël Nison) #33399 http: * (SEMVER-MINOR) expose host and protocol on ClientRequest (wenningplus) #33803 * (SEMVER-MINOR) add maxTotalSockets to agent class (rickyes) #33617 * (SEMVER-MINOR) return this from OutgoingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from ClientRequest#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) return this from IncomingMessage#destroy() (Colin Ihrig) #32789 * (SEMVER-MINOR) added scheduling option to http agent (delvedor) #33278 http2: * (SEMVER-MINOR) return this for Http2ServerRequest#setTimeout (Pranshu Srivastava) #33994 * (SEMVER-MINOR) do not modify explicity set date headers (Pranshu Srivastava) #33160 process: * (SEMVER-MINOR) add unhandled-rejection throw and warn-with-error-code (Dan Fabulich) #33475 src: * (SEMVER-MINOR) store key data in separate class (Tobias Nießen) #33360 * (SEMVER-MINOR) add NativeKeyObject base class (Tobias Nießen) #33360 * (SEMVER-MINOR) rename internal key handles to KeyObjectHandle (Tobias Nießen) #33360 * (SEMVER-MINOR) add equality operators for BaseObjectPtr (Anna Henningsen) #33772 * (SEMVER-MINOR) introduce BaseObject base FunctionTemplate (Anna Henningsen) #33772 * (SEMVER-MINOR) add public APIs to manage v8::TracingController (Anna Henningsen) #33850 stream*: * runtime deprecate Transform._transformState (Robert Nagy) #32763 win: * (SEMVER-MINOR) allow skipping the supported platform check (João Reis) #33176 worker: * (SEMVER-MINOR) add public method for marking objects as untransferable (Anna Henningsen) #33979 * (SEMVER-MINOR) emit `'messagerror'` events for failed deserialization (Anna Henningsen) #33772 * (SEMVER-MINOR) allow passing JS wrapper objects via postMessage (Anna Henningsen) #33772 * (SEMVER-MINOR) allow transferring/cloning generic BaseObjects (Anna Henningsen) #33772 worker,fs: * (SEMVER-MINOR) make FileHandle transferable (Anna Henningsen) #33772 zlib: * (SEMVER-MINOR) add `maxOutputLength` option (unknown) #33516 PR-URL: #34093

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Apr 30, 2020

joaocgreis mentioned this pull request Apr 30, 2020

src,win: Replacement of unsupported versions of Windows runtime exit. #33108

Closed

2 tasks

xCykrix mentioned this pull request May 6, 2020

Dispute of "block running on EOL Windows versions #31954" - Alternative Suggestion #33034

Closed

bzoz approved these changes May 18, 2020

View reviewed changes

jasnell reviewed May 18, 2020

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

jasnell reviewed May 18, 2020

View reviewed changes

src/node_main.cc Outdated Show resolved Hide resolved

joaocgreis added 2 commits May 21, 2020 01:49

win: allow skipping the supported platform check

4ebe1fc

Fixes: nodejs#33034

fixup! win: allow skipping the supported platform check

9e2c90f

joaocgreis force-pushed the joaocgreis-K4U-skip-plat-check branch from ccd1834 to 9e2c90f Compare May 21, 2020 03:22

jasnell approved these changes May 21, 2020

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label May 21, 2020

mhdawson approved these changes May 21, 2020

View reviewed changes

apapirovski approved these changes May 21, 2020

View reviewed changes

jasnell reviewed May 22, 2020

View reviewed changes

doc/api/cli.md Show resolved Hide resolved

gireeshpunathil approved these changes May 22, 2020

View reviewed changes

richardlau mentioned this pull request May 22, 2020

tools: fix master branch build for Windows on Arm #33511

Closed

1 task

BridgeAR approved these changes May 23, 2020

View reviewed changes

richardlau mentioned this pull request May 26, 2020

win: block running on EOL Windows versions #31954

Closed

3 tasks

fixup! win: allow skipping the supported platform check

77eb635

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2020

BridgeAR approved these changes May 29, 2020

View reviewed changes

jasnell approved these changes May 29, 2020

View reviewed changes

BridgeAR closed this May 30, 2020

xCykrix mentioned this pull request Jun 16, 2020

Node.js 14.x supported on windows 8.1 and above? #33000

Closed

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

addaleax added the dont-land-on-v12.x label Sep 22, 2020

richardlau mentioned this pull request Sep 30, 2020

Node v14.13.0-x64 does not install on Win7 x64 #35430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win: allow skipping the supported platform check #33176

win: allow skipping the supported platform check #33176

joaocgreis commented Apr 30, 2020

nodejs-github-bot commented Apr 30, 2020

joaocgreis commented Apr 30, 2020

jasnell commented Apr 30, 2020

joaocgreis commented May 1, 2020

jasnell commented May 1, 2020

cjihrig commented May 1, 2020

xCykrix commented May 6, 2020

mhdawson commented May 6, 2020 •

edited

Loading

tjrgg commented May 17, 2020

joaocgreis commented May 21, 2020

nodejs-github-bot commented May 21, 2020

mhdawson left a comment

sam-github commented May 21, 2020

xCykrix commented May 22, 2020

gireeshpunathil May 22, 2020

BridgeAR left a comment

joaocgreis commented May 26, 2020

nodejs-github-bot commented May 26, 2020

joaocgreis commented May 28, 2020

BridgeAR left a comment

BridgeAR commented May 30, 2020

TheJaredWilcurt commented Jun 18, 2020

	fprintf(stderr, "This application is only supported on Windows 8.1, "
	fprintf(stderr, "Node.js is only supported on Windows 8.1, "

win: allow skipping the supported platform check #33176

win: allow skipping the supported platform check #33176

Conversation

joaocgreis commented Apr 30, 2020

Checklist

nodejs-github-bot commented Apr 30, 2020

joaocgreis commented Apr 30, 2020

jasnell commented Apr 30, 2020

joaocgreis commented May 1, 2020

jasnell commented May 1, 2020

cjihrig commented May 1, 2020

xCykrix commented May 6, 2020

mhdawson commented May 6, 2020 • edited Loading

tjrgg commented May 17, 2020

joaocgreis commented May 21, 2020

nodejs-github-bot commented May 21, 2020

mhdawson left a comment

Choose a reason for hiding this comment

sam-github commented May 21, 2020

xCykrix commented May 22, 2020

gireeshpunathil May 22, 2020

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

joaocgreis commented May 26, 2020

nodejs-github-bot commented May 26, 2020

joaocgreis commented May 28, 2020

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented May 30, 2020

TheJaredWilcurt commented Jun 18, 2020

mhdawson commented May 6, 2020 •

edited

Loading