doc: Windows building officially supported only on x64 hosts by refack · Pull Request #21443 · nodejs/node

refack · 2018-06-21T15:00:20Z

Since we are CI testing on x64 hosts, we can only commit to x64 hosts.
We could add x86_x86 and x86_x64 as Experimental, to indicate willingness to accept community PRs.

Ref: #21402
Ref: #21437

Checklist

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

nodejs-github-bot · 2018-06-21T15:00:22Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/111/pipeline

refack · 2018-06-21T15:00:51Z

/CC @nodejs/platform-windows @nodejs/build

seishun · 2018-06-21T15:06:37Z

What is x86_x86?

seishun · 2018-06-21T15:07:00Z

Why add quotes?

Unless this is supposed to imply that Visual Studio 2017 is not required on a non-x64 host, "on an x64 host" doesn't belong here.

It's a proper product name.

Visual Studio 2017 is a product name, why isn't it in quotes?

seishun · 2018-06-21T15:08:57Z

What is "x64_x86"?

refack · 2018-06-21T15:09:03Z

It's a proper product name.

refack · 2018-06-21T15:11:18Z

x86_x86 was a typo
x64_x86 is Microsoft designation for cross compiling x86 (32bit) target, on an x64(64bit) host.
calling: "C:\Program Files\VSCommunity2017\VC\\Auxiliary\Build\vcvarsall.bat" x86_x86 [ERROR:vcvarsall.bat] Invalid argument found : x86_x86 [ERROR:vcvarsall.bat] Error in script usage. The correct usage is: Syntax: vcvarsall.bat [arch] [platform_type] [winsdk_version] [-vcvars_ver=vc_version] where : [arch]: x86 | amd64 | x86_amd64 | x86_arm | x86_arm64 | amd64_x86 | amd64_arm | amd64_arm64

"x64_x86" is a Microsoft-specific thing, it will confuse people. They expect the Architectures column to list the architecture on which Node runs, not how it's built.

I also think this is confusing. IMHO we should keep this as it is, and add the note about x64 host.

bzoz

I'm -1 on x64_x86. IMHO we should put note about only support compilation on 64bit machines in the "Notes" column

refack · 2018-07-19T20:33:32Z

@seishun @bzoz @mmarchini @jasnell PTAL.
I Moved the change to a note. Since there were already a few heterogeneous notes, I made them uniform, but I'd be happy to hear comments about the style.

seishun · 2018-07-19T20:35:34Z

I should have been more explicit: please don't put product names in quotes.

We actually don't test with the Build Tools package anymore. And we only use Windows SDK 10.

vsemozhetbyt · 2018-07-19T20:32:51Z

Teir -> Tier

vsemozhetbyt · 2018-07-19T20:35:03Z

windows -> Windows

vsemozhetbyt · 2018-07-19T20:39:00Z

Node's -> Node.js's? (A bit awkward, but it seems we try to be consistent here.)

How about Node's -> the

Seems good.

seishun · 2018-07-19T20:55:13Z

This is under "Supported Toolchains". There is no need to mention Windows 10 SDK or bitness here. The specific requirements are listed in a separate section. Why change this line at all?

IMHO SDK and host bitness are parts of the toolchain 🤷‍♂️

I'm on vacation, so I can't review this. I'm dismissing my -1 to not block this

gibfahn · 2018-07-22T23:50:11Z

I Moved the change to a note. Since there were already a few heterogeneous notes, I made them uniform, but I'd be happy to hear comments about the style.

Notes look really good FWIW.

mmarchini

Still LGTM. Nice job improving the notes!

refack · 2018-08-16T22:16:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/577/
(+ https://ci.nodejs.org/job/node-test-commit-linux-containered/6343/)

Since we are CI testing on x64 hosts, we can only commit to x64 hosts. PR-URL: nodejs#21443 Refs: nodejs#21402 Refs: nodejs#21437 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: João Reis <reis@janeasystems.com>

Since we are CI testing on x64 hosts, we can only commit to x64 hosts. PR-URL: #21443 Refs: #21402 Refs: #21437 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: João Reis <reis@janeasystems.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Jun 21, 2018

refack added windows Issues and PRs related to the Windows platform. and removed build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Jun 21, 2018

seishun reviewed Jun 21, 2018
View reviewed changes

Comment threadBUILDING.md Outdated
seishunJun 21, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is x86_x86?

seishun reviewed Jun 21, 2018
View reviewed changes

Comment threadBUILDING.md Outdated
seishunJun 21, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is "x64_x86"?

refack commented Jun 21, 2018
View reviewed changes

jasnell approved these changes Jun 21, 2018
View reviewed changes

mmarchini approved these changes Jun 21, 2018
View reviewed changes

bzoz previously requested changes Jun 25, 2018
View reviewed changes

refack added doc Issues and PRs related to the documentations. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Jul 19, 2018

seishun reviewed Jul 19, 2018
View reviewed changes

refack mentioned this pull request Jul 19, 2018
doc: Windows and Visual Studio supported versions nodejs/build#1407
Closed

vsemozhetbyt reviewed Jul 19, 2018
View reviewed changes

seishun reviewed Jul 19, 2018
View reviewed changes

joaocgreis approved these changes Jul 23, 2018
View reviewed changes

mmarchini approved these changes Jul 23, 2018
View reviewed changes

refack force-pushed the refack-patch-3 branch 2 times, most recently from 2bc2c58 to 796d510Compare August 16, 2018 22:00

refack force-pushed the refack-patch-3 branch from 796d510 to 0eb4a4eCompare August 16, 2018 23:07

refack force-pushed the refack-patch-3 branch from 0eb4a4e to 73b8ddeCompare August 16, 2018 23:08

refack merged commit 73b8dde into nodejs:masterAug 16, 2018

refack deleted the refack-patch-3 branch August 16, 2018 23:09

targos mentioned this pull request Sep 5, 2018
Release proposal: v10.10.0 #22716
Merged

This was referenced Sep 6, 2018
2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236
Closed
2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221
Closed

Uh oh!

Uh oh!

Conversation

refack commented Jun 21, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 21, 2018

Uh oh!

refack commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 19, 2018

Uh oh!

seishunJul 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jul 22, 2018

Uh oh!

mmarchini left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

seishunJul 19, 2018•
edited
Loading