GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: remove x86 from os.arch() options by gibfahn · Pull Request #17899 · nodejs/node

Conversation

@gibfahn

@gibfahn gibfahn commented Dec 28, 2017

Copy link
Copy Markdown
Member

I don't think it's possible for process.arch (which comes from
V8's target_arch) to be x86.

Also updates process.arch to have the same information.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to Node.js documentation. label Dec 28, 2017
@YurySolovyov

YurySolovyov commented Dec 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Seems like building it like that is still possible

node/configure

Lines 63 to 64 in 9c00f07

valid_arch = ('arm', 'arm64', 'ia32', 'mips', 'mipsel', 'mips64el', 'ppc',
'ppc64', 'x32','x64', 'x86', 's390', 's390x')

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis approved these changes Dec 28, 2017
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@YurySolovyov It's a synonym for ia32:

node/configure

Lines 862 to 863 in 9c00f07

if target_arch == 'x86':
target_arch = 'ia32'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/process.md Outdated

@bnoordhuis bnoordhuis Dec 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The emphasis seems a bit too much but I see os.md does that too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@apapirovski apapirovski Dec 28, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would lean towards removing the emphasis, it doesn't really add much. I'm sure @Trott would also have an opinion on this... 😉

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott reacted with laugh emoji
All reactions
  • 😄 1 reaction

@gibfahn gibfahn Dec 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

SGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca approved these changes Dec 28, 2017
View reviewed changes
apapirovski
apapirovski approved these changes Dec 28, 2017
View reviewed changes
Comment thread doc/api/process.md Outdated

@apapirovski apapirovski Dec 28, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would lean towards removing the emphasis, it doesn't really add much. I'm sure @Trott would also have an opinion on this... 😉

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott reacted with laugh emoji
All reactions
  • 😄 1 reaction
jasnell
jasnell approved these changes Dec 28, 2017
View reviewed changes
cjihrig
cjihrig approved these changes Dec 28, 2017
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. I'm also in favor of removing the emphasis in the paragraph in process.md.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
TimothyGu
TimothyGu approved these changes Dec 29, 2017
View reviewed changes

@TimothyGu TimothyGu left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What is the difference between ia32 and x32?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bnoordhuis

bnoordhuis commented Dec 29, 2017

Copy link
Copy Markdown
Member

@TimothyGu https://en.wikipedia.org/wiki/X32_ABI

allows programs to take advantage of the benefits of x86-64 instruction set [...] while using 32-bit pointers and thus avoiding the overhead of 64-bit pointers
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
danbev
danbev approved these changes Dec 29, 2017
View reviewed changes
@gibfahn

gibfahn commented Dec 29, 2017

Copy link
Copy Markdown
Member Author

@bnoordhuis so does that mean that an x32 binary wouldn't be able to run on a true Intel 32-bit machine (because it needs the 64-bit instruction set)?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TimothyGu

TimothyGu commented Dec 29, 2017

Copy link
Copy Markdown
Member

I knew something like that existed but forgot the name… oh well, I blame the Windows-esque “x64” terminology.

@gibfahn That’s correct. It provides access to the x86-64 expanded set of registers. I don’t think many people are using it though.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn gibfahn self-assigned this Dec 29, 2017
@gibfahn

gibfahn commented Jan 3, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/15215/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn
doc: remove x86 from os.arch() options
ecf6e79 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: nodejs#17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@gibfahn
gibfahn force-pushed the process-arch branch from a9bea3e to ecf6e79 Compare January 3, 2018 23:32
@gibfahn

gibfahn commented Jan 3, 2018

Copy link
Copy Markdown
Member Author

Landed in ecf6e79
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn
gibfahn merged commit ecf6e79 into nodejs:master Jan 3, 2018
@gibfahn
gibfahn deleted the process-arch branch January 3, 2018 23:34
@gibfahn gibfahn mentioned this pull request Jan 4, 2018
Detect merged/closed PR nodejs/node-core-utils#137
Closed
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
5ced72b 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
4ee6bf7 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
3fd6581 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
v9.4.0 proposal #18069
Merged
@MylesBorins MylesBorins added lts-watch-v6.x labels Jan 24, 2018
@MylesBorins

MylesBorins commented Jan 24, 2018

Copy link
Copy Markdown
Contributor

Should this land in LTS? It lands cleanly on 6.x and 8.x
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Jan 24, 2018

Copy link
Copy Markdown
Member Author

Should this land in LTS? It lands cleanly on 6.x and 8.x

Yes!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
45cb79d 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
e49dd53 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 24, 2018
@MylesBorins MylesBorins mentioned this pull request Feb 7, 2018
v6.13.0 proposal #18342
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
09ab609 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
a3fc7e1 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@gibfahn @MylesBorins
doc: remove x86 from os.arch() options
1b68986 
It is not possible for `process.arch` (which comes from V8's
`target_arch`) to be `x86`.

Also updates `process.arch` to have the same information.

PR-URL: #17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
+3 more reviewers
@apapirovski apapirovski apapirovski approved these changes
@danbev danbev danbev approved these changes
@TimothyGu TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gibfahn gibfahn

Labels

doc Issues and PRs related to Node.js documentation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@gibfahn @YurySolovyov @bnoordhuis @TimothyGu @MylesBorins @apapirovski @danbev @jasnell @lpinca @cjihrig @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.