GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — v8,tools: expose necessary V8 defines by zcbenz · Pull Request #50820 · nodejs/node

Conversation

@zcbenz

@zcbenz zcbenz commented Nov 20, 2023

Copy link
Copy Markdown
Contributor

There are a few C++ defines in V8's public headers and for native modules to work correctly the defines must be exactly the same when building Node.js and native modules. V8 has a list of the C++ defines that appear in public headers in V8/BUILD.gn and Node's GYP files are currently out of sync with the list, which causes problems when building native modules targeting Electron and the GN build of Node.

In this PR I have updated the GYP files to add necessary C++ defines based on the feature flags, there is also changes on the tools/generate_config_gypi.py script to make GN configurations include necessary feature flags in config.gypi.

The defines are currently manually synchronized between GYP and V8's GN files, and I am making changes on V8's side to get rid of it: https://chromium-review.googlesource.com/c/v8/v8/+/5040612.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Nov 20, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Nov 20, 2023
@zcbenz zcbenz mentioned this pull request Nov 20, 2023
Things blocking a full-featured GN build of upstream Node.js photoionization/node_with_gn#1
Closed
26 of 27 tasks
@zcbenz
v8,tools: expose necessary V8 defines
7919c10
@zcbenz
zcbenz force-pushed the v8-defines branch from 1a0ad04 to 7919c10 Compare November 20, 2023 10:18
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 21, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55803/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2023

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5679/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2023

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5679/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2023

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5679/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 22, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55845/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gengjiawen
gengjiawen reviewed Nov 22, 2023
View reviewed changes
Comment thread common.gypi Resolved

Uh oh!

There was an error while loading. Please reload this page.
gengjiawen
gengjiawen approved these changes Nov 22, 2023
View reviewed changes
jasnell
jasnell approved these changes Nov 24, 2023
View reviewed changes
@gengjiawen gengjiawen added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 25, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 0bb5d88 into nodejs:main Nov 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 25, 2023

Copy link
Copy Markdown
Collaborator

Landed in 0bb5d88
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zcbenz
zcbenz deleted the v8-defines branch November 26, 2023 23:48
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@zcbenz Lei Shi
v8,tools: expose necessary V8 defines
c185b8d 
PR-URL: nodejs#50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 27, 2023
@zcbenz @RafaelGSS
v8,tools: expose necessary V8 defines
c1d68f7 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
v21.3.0 proposal #50954
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
@zcbenz @RafaelGSS
v8,tools: expose necessary V8 defines
a5d959b 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@zcbenz @UlisesGascon
v8,tools: expose necessary V8 defines
d7421ba 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 13, 2023
@zcbenz @UlisesGascon
v8,tools: expose necessary V8 defines
68375ef 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
@zcbenz @UlisesGascon
v8,tools: expose necessary V8 defines
4770028 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
@zcbenz @UlisesGascon
v8,tools: expose necessary V8 defines
2181b58 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Jan 9, 2024
@zcbenz @UlisesGascon
v8,tools: expose necessary V8 defines
3b6b1af 
PR-URL: #50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Jan 10, 2024
@codebytere
v8,tools: expose necessary V8 defines
18a3f55 
nodejs/node#50820
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 12, 2024
@codebytere @zcbenz
v8,tools: expose necessary V8 defines
a717779 
nodejs/node#50820
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 12, 2024
@codebytere @zcbenz
v8,tools: expose necessary V8 defines
9783070 
nodejs/node#50820
codebytere added a commit to electron/electron that referenced this pull request Jan 15, 2024
@codebytere
v8,tools: expose necessary V8 defines
ac4d5b1 
nodejs/node#50820
codebytere added a commit to electron/electron that referenced this pull request Jan 15, 2024
@codebytere
v8,tools: expose necessary V8 defines
a06a9ee 
nodejs/node#50820
codebytere added a commit to electron/electron that referenced this pull request Jan 16, 2024
@codebytere
v8,tools: expose necessary V8 defines
75baeb6 
nodejs/node#50820
codebytere added a commit to electron/electron that referenced this pull request Jan 18, 2024
@codebytere
v8,tools: expose necessary V8 defines
19f2e6e 
nodejs/node#50820
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 18, 2024
@codebytere @jkleinsc
v8,tools: expose necessary V8 defines
d0d5b93 
nodejs/node#50820
codebytere added a commit to electron/electron that referenced this pull request Jan 18, 2024
@codebytere
v8,tools: expose necessary V8 defines
dc666c8 
nodejs/node#50820
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 18, 2024
@electron-roller @codebytere @zcbenz
chore: bump node to v20.11.0 (main) (#40941)
f4ee3c1 
* chore: bump node in DEPS to v20.11.0

* module: bootstrap module loaders in shadow realm

nodejs/node#48655

* src: add commit hash shorthand in zlib version

nodejs/node#50158

* v8,tools: expose necessary V8 defines

nodejs/node#50820

* esm: do not call getSource when format is commonjs

nodejs/node#50465

* esm: fallback to readFileSync when source is nullish

nodejs/node#50825

* vm: allow dynamic import with a referrer realm

nodejs/node#50360

* test: skip test-diagnostics-channel-memory-leak.js

nodejs/node#50327

* esm: do not call getSource when format is commonjs

nodejs/node#50465

* lib: fix assert throwing different error messages in ESM and CJS

nodejs/node#50634

* src: fix compatility with upcoming V8 12.1 APIs

nodejs/node#50709

* deps: update base64 to 0.5.1

nodejs/node#50629

* src: avoid silent coercion to signed/unsigned int

nodejs/node#50663

* src: fix compatility with upcoming V8 12.1 APIs

nodejs/node#50709

* chore: fix patch indices

* chore: update patches

* test: disable TLS cipher test

This can't be enabled owing to BoringSSL incompatibilities.

nodejs/node#50186

* fix: check for Buffer and global definition in shadow realm

nodejs/node#51239

* test: disable parallel/test-shadow-realm-custom-loader

Incompatible with our asar logic, resulting in the following failure:

> Failed to CompileAndCall electron script: electron/js2c/asar_bundle

* chore: remove deleted parallel/test-crypto-modp1-error test

* test: make test-node-output-v8-warning generic

nodejs/node#50421

* chore: fixup ModuleWrap patch

* test: match wpt/streams/transferable/transform-stream-members.any.js to upstream

* fix: sandbox is not enabled on arm

* chore: disable v8 sandbox on ia32/arm

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: Cheng Zhao <[email protected]>
@codebytere codebytere mentioned this pull request Jul 17, 2024
build: ensure v8_pointer_compression_sandbox is enabled on 64bit #53884
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@gengjiawen gengjiawen gengjiawen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@zcbenz @nodejs-github-bot @jasnell @gengjiawen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.