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

doc: edit BUILDING.md by Trott · Pull Request #23435 · nodejs/node · GitHub

/ node Public

doc: edit BUILDING.md - #23435

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:building-revisions
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

Trott commented Oct 12, 2018

Copy link
Copy Markdown
Member

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 commented Oct 12, 2018

Copy link
Copy Markdown
Collaborator

@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 added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Oct 12, 2018
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 approved these changes Oct 12, 2018
View reviewed changes
Comment thread 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.

addaleax Oct 12, 2018

Copy link
Copy Markdown
Member

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’d just skip the “such as stdio redirection” bit entirely unless we can actually explain how to redirect stdio.

refack Oct 13, 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

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

addaleax Oct 13, 2018

Copy link
Copy Markdown
Member

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

@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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 12, 2018
refack suggested changes Oct 13, 2018
View reviewed changes
Comment thread BUILDING.md Outdated

refack Oct 13, 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

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.

refack Oct 13, 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

See original discussion in #17008

Trott Oct 14, 2018

Copy link
Copy Markdown
Member 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

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

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

Trott commented Oct 14, 2018

Copy link
Copy Markdown
Member Author

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

Trott commented Oct 14, 2018

Copy link
Copy Markdown
Member Author

Landed in eeb2c8f

Trott closed this Oct 14, 2018
Trott added a commit to Trott/io.js that referenced this pull request Oct 14, 2018
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
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
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
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 mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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
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 mentioned this pull request Nov 29, 2018
Merged
Trott deleted the building-revisions branch January 13, 2022 22:50
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

addaleax addaleax approved these changes
jasnell jasnell approved these changes
richardlau richardlau approved these changes
+6 more reviewers
refack refack approved these changes
thefourtheye thefourtheye approved these changes
targos targos approved these changes
sagirk sagirk approved these changes
vsemozhetbyt vsemozhetbyt approved these changes
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 outstanding review comments, 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.

Back | FazBrowse Home | New Git URL