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: minor doc improvements #17722

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions doc/guides/building-node-with-ninja.md
Original file line number Diff line number Diff line change
@@ -1,28 +1,36 @@
# Building Node with Ninja

The purpose of this guide is to show how to build Node.js using [Ninja][], as doing so can be significantly quicker than using `make`. Please see [Ninja's site][Ninja] for installation instructions (unix only).
The purpose of this guide is to show how to build Node.js using [Ninja][], as
doing so can be significantly quicker than using `make`. Please see
[Ninja's site][Ninja] for installation instructions (unix only).

To build Node with ninja, there are 3 steps that must be taken:

1. Configure the project's OS-based build rules via `./configure --ninja`.
2. Run `ninja -C out/Release` to produce a compiled release binary.
3. Lastly, make symlink to `./node` using `ln -fs out/Release/node node`.

When running `ninja -C out/Release` you will see output similar to the following if the build has succeeded:
When running `ninja -C out/Release` you will see output similar to the following
if the build has succeeded:

```txt
ninja: Entering directory `out/Release`
[4/4] LINK node, POSTBUILDS
```

The bottom line will change while building, showing the progress as `[finished/total]` build steps.
This is useful output that `make` does not produce and is one of the benefits of using Ninja.
Also, Ninja will likely compile much faster than even `make -j4` (or `-j<number of processor threads on your machine>`).
The bottom line will change while building, showing the progress as
`[finished/total]` build steps. This is useful output that `make` does not
produce and is one of the benefits of using Ninja. Also, Ninja will likely
compile much faster than even `make -j4` (or
`-j<number of processor threads on your machine>`).

## Considerations

Ninja builds vary slightly from `make` builds. If you wish to run `make test` after, `make` will likely still need to rebuild some amount of Node.
Ninja builds vary slightly from `make` builds. If you wish to run `make test`
after, `make` will likely still need to rebuild some amount of Node.

As such, if you wish to run the tests, it can be helpful to invoke the test runner directly, like so:
As such, if you wish to run the tests, it can be helpful to invoke the test
runner directly, like so:
`tools/test.py --mode=release message parallel sequential -J`

## Alias
Expand All @@ -31,7 +39,8 @@ As such, if you wish to run the tests, it can be helpful to invoke the test runn

## Producing a debug build

The above alias can be modified slightly to produce a debug build, rather than a release build as shown below:
The above alias can be modified slightly to produce a debug build, rather than a
release build as shown below:
`alias nnodedebug='./configure --ninja && ninja -C out/Debug && ln -fs out/Debug/node node_g'`


Expand Down
57 changes: 29 additions & 28 deletions doc/guides/maintaining-V8.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ process.

* Unfixed bugs. The bug exists in the V8 master branch.
* Fixed, but needs backport. The bug may need porting to one or more branches.
* Backporting to active branches.
* Backporting to abandoned branches.
* Backporting to active branches.
* Backporting to abandoned branches.
* Backports identified by the V8 team. Bugs identified by upstream V8 that we
haven't encountered in Node.js yet.

Expand Down Expand Up @@ -188,14 +188,14 @@ backport the fix:
* Identify which version of V8 the bug was fixed in.
* Identify if any active V8 branches still contain the bug:
* A tracking bug is needed to request a backport.
* If there isn't already a V8 bug tracking the fix, open a new merge request
bug using this [Node.js specific template][V8TemplateMergeRequest].
* If a bug already exists
* Add a reference to the GitHub issue.
* Attach *merge-request-x.x* labels to the bug for any active branches
that still contain the bug. (e.g. merge-request-5.3,
merge-request-5.4)
* Add ofrobots-at-google.com to the cc list.
* If there isn't already a V8 bug tracking the fix, open a new merge request
bug using this [Node.js specific template][V8TemplateMergeRequest].
* If a bug already exists
* Add a reference to the GitHub issue.
* Attach *merge-request-x.x* labels to the bug for any active branches
that still contain the bug. (e.g. merge-request-5.3,
merge-request-5.4)
* Add ofrobots-at-google.com to the cc list.
* Once the merge has been approved, it should be merged using the
[merge script documented in the V8 wiki][V8MergingPatching]. Merging requires
commit access to the V8 repository. If you don't have commit access you can
Expand All @@ -214,24 +214,24 @@ to be cherry-picked in the Node.js repository and V8-CI must test the change.

* For each abandoned V8 branch corresponding to an LTS branch that is affected
by the bug:
* Checkout a branch off the appropriate *vY.x-staging* branch (e.g.
*v6.x-staging* to fix an issue in V8 5.1).
* Cherry-pick the commit(s) from the V8 repository.
* On Node.js < 9.0.0: Increase the patch level version in `v8-version.h`.
This will not cause any problems with versioning because V8 will not
publish other patches for this branch, so Node.js can effectively bump the
patch version.
* On Node.js >= 9.0.0: Increase the `v8_embedder_string` number in
`common.gypi`.
* In some cases the patch may require extra effort to merge in case V8 has
changed substantially. For important issues we may be able to lean on the
V8 team to get help with reimplementing the patch.
* Open a cherry-pick PR on `nodejs/node` targeting the *vY.x-staging* branch
and notify the `@nodejs/v8` team.
* Run the Node.js [V8 CI] in addition to the [Node.js CI].
Note: The CI uses the `test-v8` target in the `Makefile`, which uses
`tools/make-v8.sh` to reconstruct a git tree in the `deps/v8` directory to
run V8 tests.
* Checkout a branch off the appropriate *vY.x-staging* branch (e.g.
*v6.x-staging* to fix an issue in V8 5.1).
* Cherry-pick the commit(s) from the V8 repository.
* On Node.js < 9.0.0: Increase the patch level version in `v8-version.h`.
This will not cause any problems with versioning because V8 will not
publish other patches for this branch, so Node.js can effectively bump the
patch version.
* On Node.js >= 9.0.0: Increase the `v8_embedder_string` number in
`common.gypi`.
* In some cases the patch may require extra effort to merge in case V8 has
changed substantially. For important issues we may be able to lean on the
V8 team to get help with reimplementing the patch.
* Open a cherry-pick PR on `nodejs/node` targeting the *vY.x-staging* branch
and notify the `@nodejs/v8` team.
* Run the Node.js [V8 CI] in addition to the [Node.js CI].
Note: The CI uses the `test-v8` target in the `Makefile`, which uses
`tools/make-v8.sh` to reconstruct a git tree in the `deps/v8` directory to
run V8 tests.

The [`update-v8`] tool can be used to simplify this task. Run
`update-v8 backport --sha=SHA` to cherry-pick a commit.
Expand Down Expand Up @@ -275,6 +275,7 @@ Original commit message:
Refs: https://github.com/v8/v8/commit/a51f429772d1e796744244128c9feeab4c26a854
PR-URL: https://github.com/nodejs/node/pull/7833
```

* Open a PR against the `v6.x-staging` branch in the Node.js repo. Launch the
normal and [V8 CI] using the Node.js CI system. We only needed to backport to
`v6.x` as the other LTS branches weren't affected by this bug.
Expand Down
4 changes: 3 additions & 1 deletion doc/guides/maintaining-npm.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ $ git checkout vX.Y.Z
$ make release
```

Note: please run `npm dist-tag ls npm` and make sure this is the `latest` **dist-tag**. `latest` on git is usually released as `next` when it's time to downstream
Note: please run `npm dist-tag ls npm` and make sure this is the `latest`
**dist-tag**. `latest` on git is usually released as `next` when it's time to
downstream

## Step 3: Remove old npm

Expand Down
2 changes: 2 additions & 0 deletions doc/guides/writing-and-running-benchmarks.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ arrays/zero-int.js n=25 type=Buffer: 90.49906662339653
```

It is possible to execute more groups by adding extra process arguments.

```console
$ node benchmark/run.js arrays buffers
```
Expand Down Expand Up @@ -439,6 +440,7 @@ function main(conf) {
```

Supported options keys are:

* `port` - defaults to `common.PORT`
* `path` - defaults to `/`
* `connections` - number of concurrent connections to use, defaults to 100
Expand Down
15 changes: 10 additions & 5 deletions doc/guides/writing-tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,8 @@ const freelist = require('internal/freelist');

When writing assertions, prefer the strict versions:

* `assert.strictEqual()` over `assert.equal()`
* `assert.deepStrictEqual()` over `assert.deepEqual()`
- `assert.strictEqual()` over `assert.equal()`
- `assert.deepStrictEqual()` over `assert.deepEqual()`

When using `assert.throws()`, if possible, provide the full error message:

Expand All @@ -263,9 +263,9 @@ in each release.

For example:

* `let` and `const` over `var`
* Template literals over string concatenation
* Arrow functions when appropriate
- `let` and `const` over `var`
- Template literals over string concatenation
- Arrow functions when appropriate

## Naming Test Files

Expand Down Expand Up @@ -306,12 +306,14 @@ the upstream project, send another PR here to update Node.js accordingly.
Be sure to update the hash in the URL following `WPT Refs:`.

## C++ Unit test

C++ code can be tested using [Google Test][]. Most features in Node.js can be
tested using the methods described previously in this document. But there are
cases where these might not be enough, for example writing code for Node.js
that will only be called when Node.js is embedded.

### Adding a new test

The unit test should be placed in `test/cctest` and be named with the prefix
`test` followed by the name of unit being tested. For example, the code below
would be placed in `test/cctest/test_env.cc`:
Expand Down Expand Up @@ -345,18 +347,21 @@ static void at_exit_callback(void* arg) {
```

Next add the test to the `sources` in the `cctest` target in node.gyp:

```console
'sources': [
'test/cctest/test_env.cc',
...
],
```

Note that the only sources that should be included in the cctest target are
actual test or helper source files. There might be a need to include specific
object files that are compiled by the `node` target and this can be done by
adding them to the `libraries` section in the cctest target.

The test can be executed by running the `cctest` target:

```console
$ make cctest
```
Expand Down
54 changes: 34 additions & 20 deletions doc/onboarding-extras.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,26 +42,27 @@
| upgrading npm | @fishrock123, @MylesBorins |
| upgrading V8 | @nodejs/v8, @nodejs/post-mortem |

When things need extra attention, are controversial, or `semver-major`: @nodejs/tsc
When things need extra attention, are controversial, or `semver-major`:
@nodejs/tsc

If you cannot find who to cc for a file, `git shortlog -n -s <file>` may help.


## Labels

### By Subsystem

We generally sort issues by a concept of "subsystem" so that we know what part(s) of the codebase it touches.
We generally sort issues by a concept of "subsystem" so that we know what
part(s) of the codebase it touches.

**Subsystems generally are**:

* `lib/*.js`
* `doc`, `build`, `tools`, `test`, `deps`, `lib / src` (special), and there may be others.
* `doc`, `build`, `tools`, `test`, `deps`, `lib / src` (special), and there may
be others.
* `meta` for anything non-code (process) related

There may be more than one subsystem valid for any particular issue / PR.


### General

Please use these when possible / appropriate
Expand All @@ -74,13 +75,19 @@ Please use these when possible / appropriate
--

* `semver-{minor,major}`
* be conservative – that is, if a change has the remote *chance* of breaking something, go for semver-major
* be conservative – that is, if a change has the remote *chance* of breaking
something, go for semver-major
* when adding a semver label, add a comment explaining why you're adding it
* minor vs. patch: roughly: "does it add a new method / does it add a new section to the docs"
* major vs. everything else: run last versions tests against this version, if they pass, **probably** minor or patch
* A breaking change helper ([full source](https://gist.github.com/chrisdickinson/ba532fa0e4e243fb7b44)):
* minor vs. patch: roughly: "does it add a new method / does it add a new
section to the docs"
* major vs. everything else: run last versions tests against this version, if
they pass, **probably** minor or patch
* A breaking change helper
([full source](https://gist.github.com/chrisdickinson/ba532fa0e4e243fb7b44)):
```sh
git checkout $(git show -s --pretty='%T' $(git show-ref -d $(git describe --abbrev=0) | tail -n1 | awk '{print $1}')) -- test; make -j4 test
SHOW=$(git show-ref -d $(git describe --abbrev=0) | tail -n1 | awk '{print $1}')
git checkout $(git show -s --pretty='%T' $SHOW) -- test
make -j4 test
```

### LTS/Version labels
Expand All @@ -92,21 +99,28 @@ We use labels to keep track of which branches a commit should land on:
* Also used when the work of backporting a change outweighs the benefits
* `land-on-v?.x`
* Used by releasers to mark a PR as scheduled for inclusion in an LTS release
* Applied to the original PR for clean cherry-picks, to the backport PR otherwise
* Applied to the original PR for clean cherry-picks, to the backport PR
otherwise
* `backport-requested-v?.x`
* Used to indicate that a PR needs a manual backport to a branch in order to land the changes on that branch
* Typically applied by a releaser when the PR does not apply cleanly or it breaks the tests after applying
* Used to indicate that a PR needs a manual backport to a branch in order to
land the changes on that branch
* Typically applied by a releaser when the PR does not apply cleanly or it
breaks the tests after applying
* Will be replaced by either `dont-land-on-v?.x` or `backported-to-v?.x`
* `backported-to-v?.x`
* Applied to PRs for which a backport PR has been merged
* `lts-watch-v?.x`
* Applied to PRs which the LTS working group should consider including in a LTS release
* Does not indicate that any specific action will be taken, but can be effective as messaging to non-collaborators
* Applied to PRs which the LTS working group should consider including in a
LTS release
* Does not indicate that any specific action will be taken, but can be
effective as messaging to non-collaborators
* `lts-agenda`
* For things that need discussion by the LTS working group
* (for example semver-minor changes that need or should go into an LTS release)
* (for example semver-minor changes that need or should go into an LTS
release)
* `v?.x`
* Automatically applied to changes that do not target `master` but rather the `v?.x-staging` branch
* Automatically applied to changes that do not target `master` but rather the
`v?.x-staging` branch

Once a release line enters maintenance mode, the corresponding labels do not
need to be attached anymore, as only important bugfixes will be included.
Expand All @@ -120,20 +134,20 @@ need to be attached anymore, as only important bugfixes will be included.
* `arm`, `mips`, `s390`, `ppc`
* No x86{_64}, since that is the implied default


## Updating Node.js from Upstream

* `git remote add upstream git://github.com/nodejs/node.git`

to update from nodejs/node:

* `git checkout master`
* `git remote update -p` OR `git fetch --all` (I prefer the former)
* `git merge --ff-only upstream/master` (or `REMOTENAME/BRANCH`)


## best practices

* commit often, out to your github fork (origin), open a PR
* when making PRs make sure to spend time on the description:
* every moment you spend writing a good description quarters the amount of time it takes to understand your code.
* every moment you spend writing a good description quarters the amount of
time it takes to understand your code.
* usually prefer to only squash at the *end* of your work, depends on the change
Loading