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

[v8.x backport] prepare for markdown linting for prohibited strings #17765

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
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ us a report nonetheless.

- [#14519](https://github.com/nodejs/node/issues/14519): _Internal domain
function can be used to cause segfaults_. Causing program termination using
either the public Javascript APIs or the private bindings layer APIs requires
the ability to execute arbitrary Javascript code, which is already the highest
either the public JavaScript APIs or the private bindings layer APIs requires
the ability to execute arbitrary JavaScript code, which is already the highest
level of privilege possible.

- [#12141](https://github.com/nodejs/node/pull/12141): _buffer: zero fill
Expand Down
3 changes: 3 additions & 0 deletions doc/STYLE_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@
* Function returns should use the following format:
* <code>* Returns: {type|type2} Optional description.</code>
* E.g. <code>* Returns: {AsyncHook} A reference to `asyncHook`.</code>
* Use official styling for capitalization in products and projects.
* OK: JavaScript, Google's V8
* NOT OK: Javascript, Google's v8

[Em dashes]: https://en.wikipedia.org/wiki/Dash#Em_dash
[Javascript type]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Grammar_and_types#Data_structures_and_types
Expand Down
1 change: 1 addition & 0 deletions doc/api/all.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<!--lint disable prohibited-strings-->
@include documentation
@include synopsis
@include assert
Expand Down
4 changes: 2 additions & 2 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ typedef void (*napi_async_complete_callback)(napi_env env,
```

## Error Handling
N-API uses both return values and Javascript exceptions for error handling.
N-API uses both return values and JavaScript exceptions for error handling.
The following sections explain the approach for each case.

### Return values
Expand Down Expand Up @@ -2863,7 +2863,7 @@ Returns `napi_ok` if the API succeeded.

This API allows an add-on author to create a function object in native code.
This is the primary mechanism to allow calling *into* the add-on's native code
*from* Javascript.
*from* JavaScript.

*Note*: The newly created function is not automatically visible from
script after this call. Instead, a property must be explicitly set on any
Expand Down
6 changes: 3 additions & 3 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -1404,9 +1404,9 @@ tarball.
* `lts` {string} a string label identifying the [LTS][] label for this release.
This property only exists for LTS releases and is `undefined` for all other
release types, including _Current_ releases. Currently the valid values are:
- `'Argon'` for the v4.x LTS line beginning with v4.2.0.
- `'Boron'` for the v6.x LTS line beginning with v6.9.0.
- `'Carbon'` for the v8.x LTS line beginning with v8.9.1.
- `'Argon'` for the 4.x LTS line beginning with 4.2.0.
- `'Boron'` for the 6.x LTS line beginning with 6.9.0.
- `'Carbon'` for the 8.x LTS line beginning with 8.9.1.

For example:

Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_ARCHIVE.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js ChangeLog Archive

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>v0.11</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_IOJS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# io.js ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>v3</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V010.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 0.10 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th colspan="2">Stable</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V012.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 0.12 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>Stable</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V4.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 4 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>LTS 'Argon'</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V5.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 5 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>Stable</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V6.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 6 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>LTS 'Boron'</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V7.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 7 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th title="Previously called 'Stable'">Current</th>
Expand Down
2 changes: 2 additions & 0 deletions doc/changelogs/CHANGELOG_V8.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Node.js 8 ChangeLog

<!--lint disable prohibited-strings-->

<table>
<tr>
<th>LTS 'Carbon'</th>
Expand Down
33 changes: 18 additions & 15 deletions doc/guides/maintaining-V8.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,15 @@ 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:
* Open a cherry-pick PR on nodejs/node targeting the appropriate *vY.x-staging* branch (e.g. *v6.x-staging* to fix an issue in V8-5.1).
* 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.
* 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.
* 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.
* Run the Node.js [V8-CI](https://ci.nodejs.org/job/node-test-commit-v8-linux/) in addition to the [Node.js CI](https://ci.nodejs.org/job/node-test-pull-request/).

An example for workflow how to cherry-pick consider the following bug:
https://crbug.com/v8/5199. From the bug we can see that it was merged by V8 into
5.2 and 5.3, and not into V8 5.1 (since it was already abandoned). Since Node.js
`v6.x` uses V8 5.1, the fix needed to cherry-picked. To cherry-pick, here's an
example workflow:
An example for workflow how to cherry-pick consider the bug
[RegExp show inconsistent result with other browsers](https://crbug.com/v8/5199).
From the bug we can see that it was merged by V8 into 5.2 and 5.3, and not into
V8 5.1 (since it was already abandoned). Since Node.js `v6.x` uses V8 5.1, the
fix needed to be cherry-picked. To cherry-pick, here's an example workflow:

* Download and apply the commit linked-to in the issue (in this case a51f429). `curl -L https://github.com/v8/v8/commit/a51f429.patch | git am -3 --directory=deps/v8`. If the branches have diverged significantly, this may not apply cleanly. It may help to try to cherry-pick the merge to the oldest branch that was done upstream in V8. In this example, this would be the patch from the merge to 5.2. The hope is that this would be closer to the V8 5.1, and has a better chance of applying cleanly. If you're stuck, feel free to ping @ofrobots for help.
* Modify the commit message to match the format we use for V8 backports and replace yourself as the author. `git commit --amend --reset-author`. You may want to add extra description if necessary to indicate the impact of the fix on Node.js. In this case the original issue was descriptive enough. Example:
Expand Down Expand Up @@ -287,7 +287,7 @@ To audit for floating patches:
git log --oneline deps/v8
```

To replace the copy of V8 in Node.js, use the '[update-v8](https://gist.github.com/targos/8da405e96e98fdff01a395bed365b816)' script<sup>2</sup>. For example, if you want to replace the copy of V8 in Node.js with the branch-head for V8 5.1 branch:
To replace the copy of V8 in Node.js, use the `[update-v8](https://gist.github.com/targos/8da405e96e98fdff01a395bed365b816)` script<sup>2</sup>. For example, if you want to replace the copy of V8 in Node.js with the branch-head for V8 5.1 branch:

```shell
cd $NODE_DIR
Expand All @@ -304,20 +304,23 @@ This should be followed up with manual refloating of all relevant patches.

The fact that Node.js keeps a vendored, potentially edited copy of V8 in deps/
makes the above processes a bit complicated. An alternative proposal would be to
create a fork of V8 at nodejs/v8 that would be used to maintain the V8 branches.
This has several benefits:
create a fork of V8 at `nodejs/v8` that would be used to maintain the V8
branches. This has several benefits:

* The process to update the version of V8 in Node.js could be automated to track the tips of various V8 branches in nodejs/v8.
* It would simplify cherry-picking and porting of fixes between branches as the version bumps in v8-version.h would happen as part of this update instead of on every change.
* The process to update the version of V8 in Node.js could be automated to track
the tips of various V8 branches in `nodejs/v8`.
* It would simplify cherry-picking and porting of fixes between branches as the version bumps in `v8-version.h` would happen as part of this update instead of on every change.
* It would simplify the V8-CI and make it more automatable.
* The history of the V8 branch in nodejs/v8 becomes purer and it would make it easier to pull in the V8 team for help with reviewing.
* The history of the V8 branch in `nodejs/v8` becomes purer and it would make it
easier to pull in the V8 team for help with reviewing.
* It would make it simpler to setup an automated build that tracks Node.js master + V8 lkgr integration build.

This would require some tooling to:

* A script that would update the V8 in a specific Node.js branch with V8 from upstream (dependent on branch abandoned vs. active).
* We need a script to bump V8 version numbers when a new version of V8 is promoted from nodejs/v8 to nodejs/node.
* Enabled the V8-CI build in Jenkins to build from the nodejs/v8 fork.
* We need a script to bump V8 version numbers when a new version of V8 is
promoted from `nodejs/v8` to `nodejs/node`.
* Enabled the V8-CI build in Jenkins to build from the `nodejs/v8` fork.

## Proposal: Dealing with the need to float patches to a stable/beta

Expand All @@ -342,4 +345,4 @@ up working, we will investigate making this change upstream.

<sup>1</sup>Node.js 0.12 and older are intentionally omitted from this document as their support is ending soon.

<sup>2</sup>It seems that @targos is working on port of this script here https://github.com/targos/update-v8.
<sup>2</sup>@targos is working on [a port of this script](https://github.com/targos/update-v8).