FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

doc: Windows building officially supported only on x64 hosts - #21443

Merged
refack merged 1 commit into
nodejs:masterfrom
refack:refack-patch-3
Aug 16, 2018
ConversationCommits (1)ChecksFiles changed
Merged

doc: Windows building officially supported only on x64 hosts#21443
refack merged 1 commit into
nodejs:masterfrom
refack:refack-patch-3

Conversation

refack commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

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 commented Jun 21, 2018

Copy link
Copy Markdown
Collaborator

@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

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

refack commented Jun 21, 2018

Copy link
Copy Markdown
Contributor Author

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

seishun reviewed Jun 21, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

seishun Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is x86_x86?

seishun reviewed Jun 21, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

seishun Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why add quotes?

seishun Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

refack Jun 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's a proper product name.

seishun Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

seishun reviewed Jun 21, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

seishun Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is "x64_x86"?

refack commented Jun 21, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

refack Jun 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's a proper product name.

Comment thread BUILDING.md Outdated

refack Jun 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

seishun Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"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.

bzoz Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bzoz previously requested changes Jun 25, 2018
View reviewed changes

bzoz left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack commented Jul 19, 2018

Copy link
Copy Markdown
Contributor Author

@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.

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
Comment thread BUILDING.md Outdated

seishun Jul 19, 2018 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack Jul 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack mentioned this pull request Jul 19, 2018
Closed
vsemozhetbyt reviewed Jul 19, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

vsemozhetbyt Jul 19, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Teir -> Tier

Comment thread BUILDING.md Outdated

vsemozhetbyt Jul 19, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

windows -> Windows

Comment thread BUILDING.md Outdated

vsemozhetbyt Jul 19, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack Jul 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How about Node's -> the

vsemozhetbyt Jul 19, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Seems good.

seishun reviewed Jul 19, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

seishun Jul 19, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

refack Jul 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bzoz dismissed their stale review July 21, 2018 12:59

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

gibfahn commented Jul 22, 2018

Copy link
Copy Markdown
Member

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 approved these changes Jul 23, 2018
View reviewed changes

mmarchini left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Still LGTM. Nice job improving the notes!

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

refack commented Aug 16, 2018

Copy link
Copy Markdown
Contributor Author

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

doc: Windows building supported on x64
73b8dde 
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>
refack merged commit 73b8dde into nodejs:master Aug 16, 2018
refack deleted the refack-patch-3 branch August 16, 2018 23:09
targos pushed a commit that referenced this pull request Aug 19, 2018
doc: Windows building supported on x64
474eda6 
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>
targos pushed a commit that referenced this pull request Sep 3, 2018
doc: Windows building supported on x64
c894145 
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>
targos mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+5 more reviewers
seishun seishun left review comments
vsemozhetbyt vsemozhetbyt left review comments
joaocgreis joaocgreis approved these changes
mmarchini mmarchini approved these changes
bzoz bzoz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL