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

cli: remove deprecated v8 flags #54761

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cli: remove deprecated v8 flags #54761

wants to merge 1 commit into from
+0 −9

Conversation

omerktz
Copy link

@omerktz omerktz commented Sep 4, 2024 •
edited
Loading

Node is currently exposing the --huge-max-old-generation-size V8 flag. That flag was recently deprecated (it currently remains as nop, see crrev.com/c/5831467) and will soon be completely removed.
This PR removes the flag from Node as well (cli, documentation and tests).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 4, 2024
@RedYetiDev RedYetiDev added the v8 engine Issues and PRs related to the V8 dependency. label Sep 4, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 4, 2024 •
edited
Loading

Node.js's V8 version hasn't deprecated the flag yet, the commit was just added today (https://chromium.googlesource.com/v8/v8/+/fb3c72c1fc9a36e833d01509e8c9d674a17cf097)

Should the tests be updated, or should they wait until the internal V8 version is?

@RedYetiDev RedYetiDev added the deprecations Issues and PRs related to deprecations. label Sep 4, 2024
@targos
Copy link
Member

targos commented Sep 4, 2024

This flag is also referenced in doc/api/cli.md and src/node_options.cc: https://github.com/search?q=repo%3Anodejs%2Fnode%20huge-max-old-generation-size&type=code

@omerktz
Copy link
Author

omerktz commented Sep 4, 2024

Node.js's V8 version hasn't deprecated the flag yet, the commit was just added today (https://chromium.googlesource.com/v8/v8/+/fb3c72c1fc9a36e833d01509e8c9d674a17cf097)

Should the tests be updated, or should they wait until the internal V8 version is?

Node is not yet using the latest V8 version, but it eventually will. On the V8 side we have a node bot that complains if we try to remove the flag right now. We could disable the test on our end or have this PR as a floating patch there, but I think it makes more sense to already fix Node.
The flag was anyway enabled by default, so even if any Node users were using it wouldn't have made any difference.

This flag is also referenced in doc/api/cli.md and src/node_options.cc: https://github.com/search?q=repo%3Anodejs%2Fnode%20huge-max-old-generation-size&type=code

Thanks. I missed these. Does Node have a deprecation process to remove flags that it exposed through src/node_options.cc?

@targos
Copy link
Member

targos commented Sep 4, 2024

There are no stability guarantees on the V8 flags that we expose for convenience (so we don't need a deprecation cycle).

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 4, 2024
@omerktz
Copy link
Author

omerktz commented Sep 4, 2024

Thanks.
I updated the PR to remove --huge-max-old-generation-size from doc/api/cli.md and src/node_options.cc as well.

@omerktz
Copy link
Author

omerktz commented Sep 4, 2024

Can someone run the checks on behalf? It says it's waiting for approval from a maintainer but I unfortunately am not one. Thanks.

@richardlau richardlau removed the deprecations Issues and PRs related to deprecations. label Sep 4, 2024
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@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/6173/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@codecov Codecov
Copy link

codecov bot commented Sep 4, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.04%. Comparing base (e21984b) to head (61f13a8).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54761      +/-   ##
==========================================
- Coverage   88.04%   88.04%   -0.01%     
==========================================
  Files         651      651              
  Lines      183409   183408       -1     
  Branches    35828    35824       -4     
==========================================
- Hits       161487   161479       -8     
+ Misses      15174    15165       -9     
- Partials     6748     6764      +16
Files with missing lines Coverage Δ
src/node_options.cc 88.01% <ø> (-0.02%) ⬇️

... and 32 files with indirect coverage changes

@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 4, 2024 •
edited
Loading

Seeing as this does more than originally intended, WDYT about updating the commit message to:
cli: remove deprecated v8 flags (or similar)?

@omerktz omerktz changed the title test: remove deprecated v8 flag from nodejs test cli: remove deprecated v8 flags Sep 4, 2024
@omerktz
Copy link
Author

omerktz commented Sep 4, 2024

Seeing as this does more than originally intended, WDYT about updating the commit message to: cli: remove deprecated v8 flags (or similar)?

Done

@omerktz
Copy link
Author

omerktz commented Sep 4, 2024

Is it normal for the checks to take this long?
There are also a couple of failed tests, but I can't see how these failures (based on their outputs) can result from this PR. Are the tests just flaky?
Asking since I'm not yet familiar with Node's tests and testing infrastructure.

@RedYetiDev
Copy link
Member

Is it normal for the checks to take this long?

Yea, tests can take a while. (For example, there's been some backlog on the MacOS runners leaving those tests taking >8hrs!)

There are also a couple of failed tests, but I can't see how these failures (based on their outputs) can result from this PR. Are the tests just flaky?

Sometimes. When a maintainers sees that, they may choose to restart the CI run for that machine.

Thanks for updating the PR title! Could you apply the same change to the commit message?

@omerktz
Copy link
Author

omerktz commented Sep 5, 2024 •
edited
Loading

Thanks for updating the PR title! Could you apply the same change to the commit message?

Done. Thanks for pointing out.

@omerktz
Copy link
Author

omerktz commented Sep 6, 2024

Hi folks.
Jenkins reports 5 failed tests. I believe these are unrelated to the actual PR, but afaict I don't have permissions to restart the tests.
Can we merge this PR despite the failures? I'm also not authorized to merge.
Can someone either restart the tests or merge the PR on my behalf?
Thanks!

@omerktz
Copy link
Author

omerktz commented Sep 10, 2024

Can someone advise on next steps?
There are 5 failing checks that appear to be unrelated, yet I (as far as I can tell) don't have permissions to rerun the tests.
Alternatively, if rerunning the tests is not necessary, I also don't have permissions to merge the PR.
Would appreciate it if someone can either rerun the tests or merge the PR on my behalf.
Thanks!

@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Sep 10, 2024

CI has been a bit flaky. I'm kicking off the run again but you might try rebasing this on tip of main again.

@omerktz
Copy link
Author

omerktz commented Sep 13, 2024

Thanks James. The run you kicked off was almost successful. There was only 1 failure left.
I rebased now like you suggested, but unfortunately I again need someone to approve the workflows.

@lpinca
Copy link
Member

lpinca commented Sep 13, 2024

Please remove the merge commit.

@omerktz
test: remove deprecated v8 flag from nodejs test
b2abfc4 
test-cli-node-options.js is checking the existance of a
`--huge-max-old-generation-size` V8 flag. That flag was recently
deprected (it currently remains as nop, see crrev.com/c/5831467)
and will soon be completely removed.
@omerktz
Copy link
Author

omerktz commented Sep 19, 2024

@lpinca I removed the merge commit (and merged the actual commits I made to a single commit).

Can someone kick off another CI run?
Thanks!

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

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

@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/6201/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@anonrig anonrig anonrig approved these changes

@targos targos targos approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. test Issues and PRs related to the tests. 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.
8 participants
@omerktz @RedYetiDev @targos @nodejs-github-bot @jasnell @lpinca @anonrig @richardlau