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 managing multiple builds section to benchmarking guide #16142

Conversation

hekike
Copy link
Contributor

@hekike hekike commented Oct 11, 2017

Checklist
Affected core subsystem(s)
  • doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 11, 2017
@@ -163,6 +163,20 @@ First build two versions of Node.js, one from the master branch (here called
`./node-master`) and another with the pull request applied (here called
`./node-pr-5134`).

*To run multiple compiled versions paralell you need to copy the output of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling: parallel, also maybe put an in before parallel.

@hekike hekike force-pushed the docs/guide-writing-and-running-benchmarks-multiple-builds branch from 90e5d17 to cb9fe06 Compare October 19, 2017 18:12
@hekike
Copy link
Contributor Author

hekike commented Oct 19, 2017

@Fishrock123 thanks, fixed!

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM — just some nits

@@ -163,6 +163,20 @@ First build two versions of Node.js, one from the master branch (here called
`./node-master`) and another with the pull request applied (here called
`./node-pr-5134`).

*To run multiple compiled versions in parallel you need to copy the output of the
Copy link
Member

Choose a reason for hiding this comment

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

Could this not be italics? I think it flows well with the rest of the copy here so it doesn't need to stand out.

@@ -163,6 +163,20 @@ First build two versions of Node.js, one from the master branch (here called
`./node-master`) and another with the pull request applied (here called
`./node-pr-5134`).

*To run multiple compiled versions in parallel you need to copy the output of the
build: `cp ./out/Release/node ./node-master`.
Check out the following example:*
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the linebreak before this.

@apapirovski
Copy link
Member

@Fishrock123 Would you mind reviewing this as it appears your feedback has been addressed?

@hekike hekike force-pushed the docs/guide-writing-and-running-benchmarks-multiple-builds branch from cb9fe06 to 776ac9c Compare October 26, 2017 12:14
@hekike
Copy link
Contributor Author

hekike commented Oct 26, 2017

@apapirovski thanks, fixed!

@nodejs nodejs deleted a comment from nitalynn1 Oct 26, 2017
@apapirovski
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/11007/ (just for all the linting & markdown checking, will stop after that job)

apapirovski pushed a commit that referenced this pull request Oct 27, 2017
PR-URL: #16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@apapirovski
Copy link
Member

Landed in 2438877

Thanks @hekike! Congrats on becoming a Contributor!!

gibfahn pushed a commit that referenced this pull request Oct 30, 2017
PR-URL: #16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
PR-URL: #16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
PR-URL: #16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#16142
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants