Treat V8 platform symbols same as base #49596

sparist · 2023-09-10T20:34:54Z

This exports V8 platform symbols in the Node shared library build.

nodejs-github-bot · 2023-09-10T20:34:58Z

Review requested:

@nodejs/gyp
@nodejs/v8-update

bnoordhuis · 2023-09-11T17:45:42Z

tools/v8_gypfiles/v8.gyp

@@ -1339,10 +1339,7 @@
        '<(V8_ROOT)/src/libplatform/worker-thread.h',
      ],
      'conditions': [
-        ['component=="shared_library"', {
-          'direct_dependent_settings': {
-            'defines': ['USING_V8_PLATFORM_SHARED'],


Dropping USING_V8_PLATFORM_SHARED removes the dllimport for consumers. Can you explain how this helps? At first glance it seems like it'd break the build.

The V8 platform symbols were not being exported with dllexport without this change, so I was unable to link on Windows. After the change, it indeed does not break the build. I recall adapting existing code from other V8 GYP code for this.

Okay, let's see what the CI says.

@bnoordhuis I'm not sure any of the regular CI will be testing the shared libray build. We have https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-as-shared-lib/ for linux but I don't think we have the equivalent for windows.

PR-URL: nodejs#48548 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs#48484 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#48569 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#48593 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982 PR-URL: nodejs#48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

Move the `joinDuplicateHeaders` option to the correct alphabetical order. PR-URL: nodejs#48617 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

`test.todo`, `test.only` and `test.skip` are expected to return the same as `test`. This commit corrects the inconsistent behavior of these shorthands. Fixes: nodejs#48557 PR-URL: nodejs#48555 Reviewed-By: Moshe Atlow <[email protected]>

Co-authored-by: Joyee Cheung <[email protected]> PR-URL: nodejs#48566 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Reorder arguments of internal helper functions such that their order is consistent across X509 property getters. Add ReturnPropertyThroughBIO() and ReturnProperty(). Use these new helpers to deduplicate code across various X509 property getters. PR-URL: nodejs#48563 Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Alba Mendez <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2.3.6 to 2.20.1. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@83f0fe6...f6e388e) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#48627 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

Bumps [ossf/scorecard-action](https://github.com/ossf/scorecard-action) from 2.1.3 to 2.2.0. - [Release notes](https://github.com/ossf/scorecard-action/releases) - [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md) - [Commits](ossf/scorecard-action@80e868c...08b4669) --- updated-dependencies: - dependency-name: ossf/scorecard-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#48628 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.4.0 to 2.4.1. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@128a634...55d479f) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#48626 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

PR-URL: nodejs#48631 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: nodejs#48632 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]>

Bumps [actions/checkout](https://github.com/actions/checkout) from 3.5.2 to 3.5.3. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@8e5e7e5...c85c95e) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#48625 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

PR-URL: nodejs#48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This reverts commit 893c000. Refs: nodejs#48575 (comment) PR-URL: nodejs#48652 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

PR-URL: nodejs#48596 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>

Fixes: nodejs#47321 Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7 PR-URL: nodejs#48573 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#48644 Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Clarify that `transform._transform()` callback second argument is used only if the first argument is `null`, i.e. no error occured processing the chunk. PR-URL: nodejs#48680 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Akhil Marsonya <[email protected]>

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: nodejs#46523 PR-URL: nodejs#48592 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576 PR-URL: nodejs#48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: nodejs#48757 Fixes: nodejs#48603 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#48677 Reviewed-By: Paolo Insogna <[email protected]>

Removes flakiness from the mentioned test due to the x86 memory limit PR-URL: nodejs#48750 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

PR-URL: nodejs#48726 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs#48746 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

* The first argument `node` should be a const pointer. * The second argument `spaces` should not be a signed integer type. * The local variable `child` should be size_t. * The local variable `pair` in the range declaration should be a reference type to avoid copying the object. Refs: nodejs#48677 PR-URL: nodejs#48770 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

PR-URL: nodejs#48751 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#48586 Reviewed-By: Antoine du Hamel <[email protected]>

This is the certdata.txt[0] from NSS 3.93, released on 2023-06-29. This is the version of NSS that shipped in Firefox 116 on 2023-08-01. Certificates added: - Sectigo Public Server Authentication Root E46 - Sectigo Public Server Authentication Root R46 - SSL.com TLS RSA Root CA 2022 - SSL.com TLS ECC Root CA 2022 - Atos TrustedRoot Root CA ECC TLS 2021 - Atos TrustedRoot Root CA RSA TLS 2021 Certificates removed: - Hongkong Post Root CA 1 - E-Tugra Certification Authority - E-Tugra Global Root CA RSA v3 - E-Tugra Global Root CA ECC v3 [0] https://hg.mozilla.org/projects/nss/raw-file/NSS_3_93_RTM/lib/ckfw/builtins/certdata.txt PR-URL: nodejs#49341 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

mhdawson · 2023-09-12T20:54:38Z

Run of sharedlib ci job on linux for this PR - https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-as-shared-lib/1972/

Export V8 platform symbols in the Node shared library build, in the same was as with V8 base, to allow linking on Windows. PR-URL: nodejs#49596

Export V8 platform symbols in the Node shared library build, in the same way as with V8 base, to allow linking on Windows. PR-URL: nodejs#49596

targos · 2023-09-13T06:26:52Z

Does the problem only affect v18.x?

Export V8 platform symbols in the Node shared library build, in the same way as with V8 base, to allow linking on Windows. PR-URL: nodejs#49596

sparist · 2023-09-13T15:33:47Z

The problem presumably affects all versions, but I'm stuck on v18.x due to #49522

targos · 2023-09-15T13:47:35Z

Then please open the PR against the main branch. It will then be released and backported as any other fix.

lux01 · 2023-09-16T07:36:16Z

I ran a DLL build of v20.6.1 both with and without these changes and can confirm that the following additional symbols are exported by libnode.dll after this change

diff.txt

mhdawson · 2023-09-16T18:00:44Z

@lux01 thanks for comfirming

Export V8 platform symbols in the Node shared library build, in the same way as with V8 base, to allow linking on Windows. PR-URL: nodejs#49596

sparist · 2023-09-20T17:38:19Z

This pull request is replaced by #49740, which is based on main.

Export V8 platform symbols in the Node shared library build, in the same way as with V8 base, to allow linking on Windows. PR-URL: nodejs#49596

nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Sep 10, 2023

bnoordhuis reviewed Sep 11, 2023

View reviewed changes

atlowChemi and others added 24 commits September 12, 2023 10:59

http: server add async dispose

fcd0424

PR-URL: nodejs#48548 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

http2: server add asyncDispose

7a00bda

PR-URL: nodejs#48548 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

test: add missing assertions to test-runner-cli

dbca197

PR-URL: nodejs#48593 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

doc: fix options order

61231c2

Move the `joinDuplicateHeaders` option to the correct alphabetical order. PR-URL: nodejs#48617 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

test_runner: fixed test shorthands return type

3fc8004

`test.todo`, `test.only` and `test.skip` are expected to return the same as `test`. This commit corrects the inconsistent behavior of these shorthands. Fixes: nodejs#48557 PR-URL: nodejs#48555 Reviewed-By: Moshe Atlow <[email protected]>

tools: update lint-md-dependencies to [email protected]

609804d

PR-URL: nodejs#48631 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Rich Trott <[email protected]>

tools: update eslint to 8.44.0

c77d9c8

PR-URL: nodejs#48632 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]>

child_process: support Symbol.dispose

535de5a

PR-URL: nodejs#48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Revert "test: remove test-crypto-keygen flaky designation"

0b62218

This reverts commit 893c000. Refs: nodejs#48575 (comment) PR-URL: nodejs#48652 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

events: allow safely adding listener to abortSignal

0a3ae9f

PR-URL: nodejs#48596 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Nagy <[email protected]>

http2: send RST code 8 on AbortController signal

71da5dc

Fixes: nodejs#47321 Refs: https://www.rfc-editor.org/rfc/rfc7540#section-7 PR-URL: nodejs#48573 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>

doc: mention git node release prepare

0d544b4

Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#48644 Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>

src: remove kEagerCompile for CompileFunction

0de99d2

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576 PR-URL: nodejs#48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

atlowChemi and others added 9 commits September 12, 2023 11:03

doc: add atlowChemi to collaborators

76c9f35

PR-URL: nodejs#48757 Fixes: nodejs#48603 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

permission: add debug log when inserting fs nodes

07eac0d

Signed-off-by: RafaelGSS <[email protected]> PR-URL: nodejs#48677 Reviewed-By: Paolo Insogna <[email protected]>

test: fix flaky test-string-decode.js on x86

1e87ce9

Removes flakiness from the mentioned test due to the x86 memory limit PR-URL: nodejs#48750 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

child_process: harden against prototype pollution

ee024cf

PR-URL: nodejs#48726 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

util: use primordials.ArrayPrototypeIndexOf instead of mutable method

66fb032

PR-URL: nodejs#48586 Reviewed-By: Antoine du Hamel <[email protected]>

sparist force-pushed the dcp/tickets/49595 branch from e48f6cb to c51e1b4 Compare September 12, 2023 21:48

sparist force-pushed the dcp/tickets/49595 branch from c51e1b4 to 7d8febc Compare September 12, 2023 21:49

ruyadorno force-pushed the v18.x-staging branch from b809016 to ef85828 Compare September 13, 2023 01:11

sparist force-pushed the dcp/tickets/49595 branch from 7d8febc to fde500e Compare September 13, 2023 06:24

node-api: treat V8 platform symbols same as base

28626c9

Export V8 platform symbols in the Node shared library build, in the same way as with V8 base, to allow linking on Windows. PR-URL: nodejs#49596

sparist force-pushed the dcp/tickets/49595 branch from fde500e to 28626c9 Compare September 13, 2023 06:39

ruyadorno force-pushed the v18.x-staging branch from ef85828 to 490fc00 Compare September 17, 2023 02:48

sparist mentioned this pull request Sep 20, 2023

node-api: treat V8 platform symbols same as base #49740

Open

sparist closed this Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Treat V8 platform symbols same as base #49596

Treat V8 platform symbols same as base #49596

Uh oh!

sparist commented Sep 10, 2023

Uh oh!

nodejs-github-bot commented Sep 10, 2023

Uh oh!

bnoordhuis Sep 11, 2023

Uh oh!

sparist Sep 12, 2023

Uh oh!

bnoordhuis Sep 12, 2023

Uh oh!

mhdawson Sep 12, 2023

Uh oh!

mhdawson commented Sep 12, 2023

Uh oh!

targos commented Sep 13, 2023

Uh oh!

sparist commented Sep 13, 2023

Uh oh!

targos commented Sep 15, 2023

Uh oh!

lux01 commented Sep 16, 2023

Uh oh!

mhdawson commented Sep 16, 2023

Uh oh!

sparist commented Sep 20, 2023

Uh oh!

Uh oh!

Uh oh!

Treat V8 platform symbols same as base #49596

Treat V8 platform symbols same as base #49596

Uh oh!

Conversation

sparist commented Sep 10, 2023

Uh oh!

nodejs-github-bot commented Sep 10, 2023

Uh oh!

bnoordhuis Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

sparist Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

mhdawson Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 12, 2023

Uh oh!

targos commented Sep 13, 2023

Uh oh!

sparist commented Sep 13, 2023

Uh oh!

targos commented Sep 15, 2023

Uh oh!

lux01 commented Sep 16, 2023

Uh oh!

mhdawson commented Sep 16, 2023

Uh oh!

sparist commented Sep 20, 2023

Uh oh!

Uh oh!