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

doc: add "building node with ninja" guide by Fishrock123 · Pull Request #4767 · nodejs/node · GitHub

/ node Public

doc: add "building node with ninja" guide - #4767

Merged
Fishrock123 merged 1 commit into
nodejs:masterfrom
Fishrock123:ninja-build-guide
Feb 23, 2016
Merged

doc: add "building node with ninja" guide#4767
Fishrock123 merged 1 commit into
nodejs:masterfrom
Fishrock123:ninja-build-guide

Conversation

Copy link
Copy Markdown
Contributor

People always ask me about how to do this, and there's no good info about it really anywhere.

Moving from nodejs/docs#38

Fishrock123 added the doc Issues and PRs related to the documentations. label Jan 19, 2016

Copy link
Copy Markdown
Contributor Author

cc @nodejs/documentation

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

Copy link
Copy Markdown
Contributor

Thanks for PR'ing this in! The Docs WG is going to discuss next steps on adding guides in tomorrows meeting at 10AM PST; we'll take action on the PR shortly after that.

Comment thread doc/guides/building-node-with-ninja.md Outdated

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

Throw in a newline after this so markdown knows it's a list. Same on line 11.

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

Hmm, my preview works fine, maybe it's just github. Oh well.

bengl commented Jan 20, 2016

Copy link
Copy Markdown
Member

Please word-wrap at 80 characters, as per https://github.com/nodejs/docs/blob/master/STYLE-GUIDE.md

Copy link
Copy Markdown
Contributor Author

@bengl 80 characters does not work for lists. The list will force a newline. :/

Copy link
Copy Markdown
Contributor

@Fishrock123 I don't think that's so — for example:

  • Hello there.
    I am wrapped.
  • I am wrapped too
    But it's okay.

Source:

* Hello there.
I am wrapped.
* I am wrapped too
But it's okay.

Edit, if that's too ugly:

  • Hello there.
    I am wrapped.
  • I am wrapped too
    But it's okay.
* Hello there.
  I am wrapped.
* I am wrapped too
  But it's okay.

Will work too!

Copy link
Copy Markdown
Contributor Author

@chrisdickinson I don't think this is necessarily consistent between parsers/renderers then:

(Atom's markdown preview)

Copy link
Copy Markdown
Contributor Author

@bengl I consolidated the instructions into one list. :)

Copy link
Copy Markdown
Member

@Fishrock123 you mention a few times how much faster it is -- out of curiosity, could you quantify it?

Copy link
Copy Markdown
Contributor Author

@jbergstroem you mean /usr/bin/time it or what it seems to be?

I'd say it's at least 40% faster

Copy link
Copy Markdown
Member

@Fishrock123 so ninja vs make -j${cores}? Didn't think it was that much faster. Wow.

Copy link
Copy Markdown
Contributor Author

@jbergstroem well, that's how much faster it feels so maybe it isn't idk.

Copy link
Copy Markdown
Member

Just tested it locally:

  • time spent: ninja was 10% faster
  • cpu time: 4% less spent

Copy link
Copy Markdown
Contributor Author

ping @bengl & @chrisdickinson

bengl commented Jan 30, 2016

Copy link
Copy Markdown
Member

ok LGTM 👍

Copy link
Copy Markdown
Contributor Author

@bengl Updated a bit (I wasn't comfortable with some statements), PTAL.

@chrisdickinson I still haven't shortened the lines... do we need to take it back to the docs WG? My editor renderer does not render split lines very nicely.

Copy link
Copy Markdown
Contributor Author

I'm going to land this tomorrow (Tuesday) unless there is additional review. We can fix any problems with it after I guess.

bengl commented Feb 22, 2016

Copy link
Copy Markdown
Member

Whoops, missed this. Yep, LGTM.

Copy link
Copy Markdown
Contributor Author

@chrisdickinson do doc wg members officially have sign-off rights for doc/? I forget..

Qard commented Feb 22, 2016

Copy link
Copy Markdown
Member

LGTM too. 👍

PR-URL: nodejs#4767
Refs: nodejs/docs#38
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Fishrock123 merged commit 65c0feb into nodejs:master Feb 23, 2016
rvagg pushed a commit that referenced this pull request Feb 27, 2016
PR-URL: #4767
Refs: nodejs/docs#38
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
PR-URL: #4767
Refs: nodejs/docs#38
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
MylesBorins pushed a commit that referenced this pull request Mar 10, 2016
PR-URL: #4767
Refs: nodejs/docs#38
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
PR-URL: #4767
Refs: nodejs/docs#38
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
PR-URL: #4767
Refs: nodejs/docs#38
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
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
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants


Back | FazBrowse Home | New Git URL