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.

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

doc: add note for build path #14354

Closed
wants to merge 3 commits into from
Closed

Conversation

kylelamse
Copy link

@kylelamse kylelamse commented Jul 18, 2017

Fixes: #14337

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

doc

@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 Jul 18, 2017
BUILDING.md Outdated
@@ -174,6 +174,7 @@ Prerequisites:
* Basic Unix tools required for some tests,
[Git for Windows](http://git-scm.com/download/win) includes Git Bash
and tools which can be included in the global `PATH`.
* NOTE: If the path to your home directory contains a space, the build will likely fail.
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 replace home with build directory, I don’t think the home directory should really have much of an influence.

Also, this is not specific to Windows. ;)

Copy link
Author

Choose a reason for hiding this comment

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

Good to know. Where in the doc should it go?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 18, 2017

Overal LGTM, but @addaleax's comment re note expanding should be considered.

@kylelamse
Copy link
Author

kylelamse commented Jul 18, 2017

@addaleax the reason why I put it in the Windows-specific section is because in #14337 the vcbuild.bat script tried to reference my home path here:

'C:\Users\Kyle\icupkg' is not recognized as an internal or external command,
  operable program or batch file.

I dropped the repo in my root directory, ran the .bat file, and it still attempted to execute icupkg in the same path. My home path was C:\Users\Kyle Lamse\ so it looks like it chopped off my last name.

@mscdex mscdex added the windows Issues and PRs related to the Windows platform. label Jul 18, 2017
@gibfahn
Copy link
Member

gibfahn commented Jul 18, 2017

Why not put it under ## Building Node.js on supported platforms, we want it to be visible for anyone going into that section.

While you're at it, could you wrap to 80 chars?

@vsemozhetbyt vsemozhetbyt changed the title doc: add note for Windows build path doc: add note for build path Jul 19, 2017
BUILDING.md Outdated
@@ -174,6 +174,7 @@ Prerequisites:
* Basic Unix tools required for some tests,
[Git for Windows](http://git-scm.com/download/win) includes Git Bash
and tools which can be included in the global `PATH`.
* NOTE: If the path to your build directory contains a space, the build will likely fail.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Get rid of NOTE: . Just say it: If the path to your build directory contains a space, the build will likely fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just make sure it doesn't get sucked into the GfW bullet point. (maybe add a blank line)

BUILDING.md Outdated
@@ -174,6 +174,7 @@ Prerequisites:
* Basic Unix tools required for some tests,
[Git for Windows](http://git-scm.com/download/win) includes Git Bash
and tools which can be included in the global `PATH`.
* NOTE: If the path to your build directory contains a space, the build will likely fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make sure it doesn't get sucked into the GfW bullet point. (maybe add a blank line)

@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Ping @refack @addaleax and @vsemozhetbyt . please take another look, I added a commit the addressed the standing feedback

jasnell pushed a commit that referenced this pull request Aug 24, 2017
PR-URL: #14354
Fixes: #14337
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 24, 2017

Landed in 43029da

@jasnell jasnell closed this Aug 24, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
PR-URL: nodejs/node#14354
Fixes: nodejs/node#14337
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
PR-URL: nodejs/node#14354
Fixes: nodejs/node#14337
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
PR-URL: #14354
Fixes: #14337
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
PR-URL: #14354
Fixes: #14337
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
PR-URL: #14354
Fixes: #14337
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot build on windows