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.

Sign up for GitHub

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

Jump to bottom

[v20.x] deps: V8: remove references to non-existent flags #52256

Closed
wants to merge 1 commit into from
Closed

[v20.x] deps: V8: remove references to non-existent flags #52256

wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

#49703 cherry-picked a commit from upstream V8 which references some maglev-related flags that are not present in the version of V8 in Node.js 20.

V8 CI on x64 is currently broken, e.g. https://ci.nodejs.org/job/node-test-commit-v8-linux/5892/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/console

16:38:36 ../../src/flags/flags.cc:590:33: error: no member named 'maglev_deopt_data_on_background' in 'v8::internal::FlagValues'
16:38:36         flag.PointsTo(&v8_flags.maglev_deopt_data_on_background) ||
16:38:36                        ~~~~~~~~ ^
16:38:36 ../../src/flags/flags.cc:591:33: error: no member named 'maglev_build_code_on_background' in 'v8::internal::FlagValues'
16:38:36         flag.PointsTo(&v8_flags.maglev_build_code_on_background) ||
16:38:36                        ~~~~~~~~ ^
16:38:36 2 errors generated.

This comes from #49703 which went out in Node.js 20.8.0 and was presumably missed because the x64 V8 CI has been unreliable due to networking issues on the machine. The V8 CI has been moved onto new x64 machines, which have revealed this compilation failure. (Other release lines successfully compile but are failing perf-related tests.)

Other platforms tested in the V8 CI, Linux on ppc64le and s390x, do not enable maglev so didn't surface this.

This is a fairly naive attempt to fix the compilation failure -- I don't know if there are alternate flags for the version of V8 in Node.js 20 that should be implied by --predictable. cc @joyeecheung

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Mar 28, 2024
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 28, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57992/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

joyeecheung
joyeecheung approved these changes Mar 28, 2024
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

This is just to ignore flags implied by —predictable when computing the hash of flags. If the flags are not present it’s safe to just not add code to…ignore rhem.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57993/

@richardlau
deps: V8: remove references to non-existent flags
eb7d9b7 
nodejs#49703 cherry-picked a commit from
upstream V8 which references some maglev-related flags that are not
present in the version of V8 in Node.js 20.
@richardlau
Copy link
Member Author

Rebased

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58247/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58275/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58288/

richardlau added a commit that referenced this pull request Apr 17, 2024
@richardlau
deps: V8: remove references to non-existent flags
ecadd63 
#49703 cherry-picked a commit from
upstream V8 which references some maglev-related flags that are not
present in the version of V8 in Node.js 20.

PR-URL: #52256
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
@richardlau
Copy link
Member Author

richardlau commented Apr 17, 2024 •
edited
Loading

Landed in ecadd63.

@richardlau richardlau closed this Apr 17, 2024
@richardlau richardlau deleted the maglevflags branch April 17, 2024 11:54
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@richardlau @nodejs-github-bot @lpinca @gengjiawen @joyeecheung @RafaelGSS @marco-ippolito