Skip to content

doc: edit BUILDING.md#23435

Closed
Trott wants to merge 1 commit intonodejs:masterfrom
Trott:building-revisions
Closed

doc: edit BUILDING.md#23435
Trott wants to merge 1 commit intonodejs:masterfrom
Trott:building-revisions

Conversation

@Trott
Copy link
Member

@Trott Trott commented Oct 12, 2018

I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 12, 2018

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Oct 12, 2018
@Trott
doc: edit BUILDING.md
88874e3 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.
addaleax
addaleax approved these changes Oct 12, 2018
View reviewed changes
BUILDING.md
Windows binary (`node.exe`) in WSL is not recommended, and will not work
without adjustment (such as stdio redirection).
Windows binary (`node.exe`) in WSL is not recommended. It will not work
without workarounds such as stdio redirection.
Copy link
Member

@addaleax addaleax Oct 12, 2018

Choose a reason for hiding this comment

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

I’d just skip the “such as stdio redirection” bit entirely unless we can actually explain how to redirect stdio.

Copy link
Contributor

@refack refack Oct 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We do, sort of, in https://github.com/nodejs/node/blob/f1fed7cb9c770e7841a3914da098e047665b2d53/BUILDING.md#fn3. Please restore.

Copy link
Member

@addaleax addaleax Oct 13, 2018

Choose a reason for hiding this comment

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

@refack It’s absolutely not obvious how WSL and running in terminal emulators are related. When adding back this piece, please suggest wording that actually explains this or links back to the footnote you linked here.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2018
refack
refack suggested changes Oct 13, 2018
View reviewed changes
BUILDING.md Outdated
Copy link
Contributor

@refack refack Oct 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The (such as stdio redirection) bit is the most significant workaround, as in if it's done the Windows Binary works 99% of the time. IMHO it should stay.

Copy link
Contributor

@refack refack Oct 13, 2018

Choose a reason for hiding this comment

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

See original discussion in #17008

Copy link
Member Author

@Trott Trott Oct 14, 2018

Choose a reason for hiding this comment

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

Since removing it was an optional nit in the first place, I'll restore it.

@Trott Trott force-pushed the building-revisions branch from f1fed7c to 88874e3 Compare October 14, 2018 01:40
@Trott
Copy link
Member Author

Trott commented Oct 14, 2018

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

@Trott
Copy link
Member Author

Trott commented Oct 14, 2018

Landed in eeb2c8f

@Trott Trott closed this Oct 14, 2018
Trott added a commit to Trott/io.js that referenced this pull request Oct 14, 2018
@Trott
doc: edit BUILDING.md
eeb2c8f 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

PR-URL: nodejs#23435
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
addaleax pushed a commit that referenced this pull request Oct 14, 2018
@Trott @addaleax
doc: edit BUILDING.md
cf42a2d 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

PR-URL: #23435
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@Trott @jasnell
doc: edit BUILDING.md
d911bab 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

PR-URL: #23435
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
@Trott @MylesBorins
doc: edit BUILDING.md
45fbcbb 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

PR-URL: #23435
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@Trott @rvagg
doc: edit BUILDING.md
07c4552 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

PR-URL: #23435
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@Trott @MylesBorins
doc: edit BUILDING.md
011116c 
I should have done this a few days ago so it could land in time for Code
& Learn, but oh well. Here are some revisions to BUILDING.md to try to
make it easier to read.

PR-URL: #23435
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
@Trott Trott deleted the building-revisions branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@richardlau richardlau richardlau approved these changes

+5 more reviewers

@refack refack refack approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@sagirk sagirk sagirk approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@gdams gdams gdams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.