doc: update BUILDING.md #8704

rainabba · 2016-09-21T22:37:37Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

Added note about vcbuild being included as batch to clarify that it's not needed from msbuild tools or visual studio.

jasnell

LGTM

Trott · 2016-09-22T05:38:03Z

BUILDING.md

@@ -103,6 +103,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`.
+* vcbuild is provided by this repo as a batch file so if using powershell, use local syntax `./vcbuild nosign`


I'd prefer this:

Please note that vcbuild is a batch file provided with the Node.js source code. If using PowerShell, invoke with a leading ./. For example: ./vcbuild nosign

Note that it should be a standalone sentence below the bulleted list of prerequisites, not an item on the list (as it is not a prerequisite).

@Trott

Change per request of @Trott

rainabba

Changes per request from @Trott

rainabba · 2016-09-22T16:29:43Z

@Trott I've not used the code-review system on Github before so I hope I executed that change correctly. If not, I'd appreciate guidance and please know it wasn't for lack of trying.

richardlau · 2016-09-22T18:09:19Z

BUILDING.md

@@ -103,7 +103,8 @@ 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`.
-* vcbuild is provided by this repo as a batch file so if using powershell, use local syntax `./vcbuild nosign`
+
+Please note that vcbuild is a batch file provided with the Node.js source code. If using PowerShell, use local syntax. For example: ./vcbuild nosign


Since this is Windows, wouldn't .\ be more correct (and work in cmd.exe and PowerShell)? Is there any reason not to prefix all the examples .\ (e.g. .\vcbuild) to be consistent with ./configure/./node examples in the Unix / OS X section?

That sounds like an even better solution if that will work on all common Windows environments.

Good point!

fhinkel · 2016-09-22T18:11:24Z

BUILDING.md

@@ -104,6 +104,8 @@ Prerequisites:
  [Git for Windows](http://git-scm.com/download/win) includes Git Bash
  and tools which can be included in the global `PATH`.

+Please note that vcbuild is a batch file provided with the Node.js source code. If using PowerShell, use local syntax. For example: ./vcbuild nosign


I wouldn't know what local syntax means in that context. I'd prefer use a leading .\.

Could you enclose vcbuild and ./vcbuild nosign in backticks to format them as code?
Also, this probably misses a trailing dot.

ChALkeR

See the in-line comment, plus this needs an actual commit message formatted according to the documentation.

ChALkeR · 2016-09-22T19:00:49Z

BUILDING.md

@@ -104,6 +104,8 @@ Prerequisites:
  [Git for Windows](http://git-scm.com/download/win) includes Git Bash
  and tools which can be included in the global `PATH`.

+Please note that vcbuild is a batch file provided with the Node.js source code. If using PowerShell, use local syntax. For example: ./vcbuild nosign


Could you enclose vcbuild and ./vcbuild nosign in backticks to format them as code?
Also, this probably misses a trailing dot.

Trott · 2016-09-22T22:22:09Z

My little text-edit to help others has become a project in itself :/

IMO, people underestimate the difficulty of writing clear documentation. But the value is immense, so thanks for doing this, and I encourage you to not give up quite yet!

Trott · 2016-09-22T22:28:10Z

@rainabba I opened a PR against your branch that I think is what is being sought here. Can you take a look? If it looks good to you, you can merge it and it will be part of this PR.

If that works for you, then whoever lands (if it's not me) this should just squash my commit into yours.

squash: nits

rainabba · 2016-09-22T22:42:28Z

@Trott The challenge here for me wasn't in the documentation (though your PR I just approved was definitely a cleaner approach than mine) so much as the PR process here that I'm still getting the hang of. Now per the request from @ChALkeR , I need to clean up my commit message, but I'm not sure the best way to make that happen and get that change through to here so it's clean without changing that one commit, then force pushing. I'd think that would be okay if nobody else had touched it, but I fear the moment I made the PR that would become a bad idea and I'm not sure how else to accomplish without a new PR. Of course, this assumes that the change requested by @ChALkeR is a show-stopper.

Trott · 2016-09-22T22:49:31Z

@rainabba: I would say:

You don't have to worry about fixing the commit message. Whoever lands this can modify the commit message to be the correct format.
Do look at the doc that @ChALkeR linked and run git log to look at existing commit messages so you get a feel for that.

If, after doing the second item above, you really want to fix the commit message, go for it. You can either edit your first commit to have the correctly-formatted message and force push, or you can squash everything into one commit and force-push.

But if all you do is just be familiar with it for next time, that's great. It doesn't need to be fixed by you this time, especially if you're uncertain of how it will affect GitHub's treatment of your branch in the pull request. (I'm pretty sure it will be fine no matter what you do, for what it's worth.)

imyller

LGTM

imyller · 2016-09-22T23:11:36Z

@Trott I can help this to a smooth landing after 48 hour window for PR is over.

@rainabba As said, don't worry. If required, we can handle the details like as commit message formatting and squashing commits when landing.

rainabba · 2016-09-22T23:15:22Z

@Trott @imyller Thanks guys. This has been a good learning experience for github reviews and contributing to nodejs as well as teaching me how to build/test.

fhinkel · 2016-09-23T07:45:15Z

Side note: It's no problem for GitHub if you force push onto a branch that's used in a PR :)

imyller · 2016-09-23T09:27:45Z

@fhinkel I regularly do just that to update PR content. Only things force push can potentially conflict with is the ongoing CI run and CI->GitHub status updates; and starting new CI fixes that.

lpinca

LGTM

imyller · 2016-09-23T23:06:34Z

I'll start landing this:

Five LGTMs
No objections
Requested changes have been made
CI not required

Added note about vcbuild being included as batch to clarify that it's not needed from msbuild tools or visual studio. PR-URL: #8704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

imyller · 2016-09-23T23:14:19Z

landed in 92c1d96

Thank you for effort and contribution, @rainabba 👍

Added note about vcbuild being included as batch to clarify that it's not needed from msbuild tools or visual studio. PR-URL: #8704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

joaocgreis · 2017-02-08T18:11:34Z

@MylesBorins about the dont-land-on-v4.x label here, is it just because it doesn't land cleanly? I cherry-picked and solved conflicts locally, I believe this is good to have, do you mind if I push?

MylesBorins · 2017-02-08T21:54:38Z

@joaocgreis that was basically it, most doc changes that don't land cleanly have simply been dont-land.

Added note about vcbuild being included as batch to clarify that it's not needed from msbuild tools or visual studio. PR-URL: #8704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

joaocgreis · 2017-02-09T16:42:52Z

Thanks @MylesBorins

Landed in v4.x-staging in 5b98fd0

Added note about vcbuild being included as batch to clarify that it's not needed from msbuild tools or visual studio. PR-URL: #8704 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Update BUILDING.md

7fab1fc

Added note about vcbuild being included as batch to clarify that it's not needed from msbuild tools or visual studio.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 21, 2016

jasnell approved these changes Sep 21, 2016

View reviewed changes

mscdex added the build Issues and PRs related to build files or the CI. label Sep 22, 2016

Trott requested changes Sep 22, 2016

View reviewed changes

Update BUILDING.md

df27e59

Change per request of @Trott

rainabba commented Sep 22, 2016

View reviewed changes

richardlau reviewed Sep 22, 2016

View reviewed changes

fhinkel approved these changes Sep 22, 2016

View reviewed changes

ChALkeR suggested changes Sep 22, 2016

View reviewed changes

squash: nits

3686894

Merge pull request #1 from Trott/patch-1

1bc3fcb

squash: nits

Trott approved these changes Sep 22, 2016

View reviewed changes

imyller approved these changes Sep 22, 2016

View reviewed changes

imyller self-assigned this Sep 22, 2016

rainabba mentioned this pull request Sep 22, 2016

assertion when attaching the Chrome dev tools #8155

Closed

lpinca approved these changes Sep 23, 2016

View reviewed changes

imyller changed the title ~~Update BUILDING.md~~ doc: update BUILDING.md Sep 23, 2016

imyller closed this Sep 23, 2016

imyller removed their assignment Sep 23, 2016

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

richardlau mentioned this pull request Dec 7, 2016

build: change nosign flag to sign and flip the logic #10156

Closed

2 tasks

richardlau mentioned this pull request Jan 3, 2017

doc: corrected vcbuild parameters for testing on windows #10112

Closed

3 tasks

joaocgreis mentioned this pull request Feb 8, 2017

v7.x backport - doc: correct vcbuild options for windows testing #10686

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: update BUILDING.md #8704

doc: update BUILDING.md #8704

rainabba commented Sep 21, 2016 •

edited by imyller

Loading

jasnell left a comment

Trott Sep 22, 2016 •

edited

Loading

rainabba left a comment

rainabba commented Sep 22, 2016

richardlau Sep 22, 2016

Trott Sep 22, 2016

rainabba Sep 22, 2016

fhinkel Sep 22, 2016

ChALkeR Sep 22, 2016

ChALkeR left a comment

ChALkeR Sep 22, 2016

Trott commented Sep 22, 2016

Trott commented Sep 22, 2016

rainabba commented Sep 22, 2016 •

edited

Loading

Trott commented Sep 22, 2016

imyller left a comment

imyller commented Sep 22, 2016

rainabba commented Sep 22, 2016

fhinkel commented Sep 23, 2016

imyller commented Sep 23, 2016

lpinca left a comment

imyller commented Sep 23, 2016 •

edited

Loading

imyller commented Sep 23, 2016

joaocgreis commented Feb 8, 2017

MylesBorins commented Feb 8, 2017

joaocgreis commented Feb 9, 2017

doc: update BUILDING.md #8704

doc: update BUILDING.md #8704

Conversation

rainabba commented Sep 21, 2016 • edited by imyller Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell left a comment

Choose a reason for hiding this comment

Trott Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

rainabba left a comment

Choose a reason for hiding this comment

rainabba commented Sep 22, 2016

richardlau Sep 22, 2016

Choose a reason for hiding this comment

Trott Sep 22, 2016

Choose a reason for hiding this comment

rainabba Sep 22, 2016

Choose a reason for hiding this comment

fhinkel Sep 22, 2016

Choose a reason for hiding this comment

ChALkeR Sep 22, 2016

Choose a reason for hiding this comment

ChALkeR left a comment

Choose a reason for hiding this comment

ChALkeR Sep 22, 2016

Choose a reason for hiding this comment

Trott commented Sep 22, 2016

Trott commented Sep 22, 2016

rainabba commented Sep 22, 2016 • edited Loading

Trott commented Sep 22, 2016

imyller left a comment

Choose a reason for hiding this comment

imyller commented Sep 22, 2016

rainabba commented Sep 22, 2016

fhinkel commented Sep 23, 2016

imyller commented Sep 23, 2016

lpinca left a comment

Choose a reason for hiding this comment

imyller commented Sep 23, 2016 • edited Loading

imyller commented Sep 23, 2016

joaocgreis commented Feb 8, 2017

MylesBorins commented Feb 8, 2017

joaocgreis commented Feb 9, 2017

rainabba commented Sep 21, 2016 •

edited by imyller

Loading

Trott Sep 22, 2016 •

edited

Loading

rainabba commented Sep 22, 2016 •

edited

Loading

imyller commented Sep 23, 2016 •

edited

Loading