Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: edit BUILDING.md #23435

Closed
wants to merge 1 commit into from
Closed

doc: edit BUILDING.md #23435

wants to merge 1 commit into from

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

@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

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

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

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

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
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: George Adams <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: George Adams <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: George Adams <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: George Adams <[email protected]>
@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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: George Adams <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: George Adams <[email protected]>
@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

@gdams gdams gdams approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@richardlau richardlau richardlau approved these changes

@sagirk sagirk sagirk approved these changes

@targos targos targos approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

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.