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

Revert "src: make sure pass the argv to worker threads" #53021

Closed

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented May 16, 2024

aba4a00 introduced a regression, making it impossible to use Worker's env option when the process is start with process-specific flags, such as --title or --expose_gc. See the new test case, in which all the three new Worker() calls fail.

aba4a00 was released yesterday in 22.2.0.

Commit 1:

worker: Add test for env option when using process-only cli flag

It should be possible to pass the env option to a worker
even when the parent process is using a process-level flag, such
as --title or a V8-specific flag.

This test is currently failing in Node.js 22.2.0

Commit 2:

Revert "src: make sure pass the argv to worker threads"

This reverts commit aba4a00.

Fixes #53011, cc @theanarkh

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels May 16, 2024
@nicolo-ribaudo nicolo-ribaudo force-pushed the fix-22.2-worker-regression branch 2 times, most recently from dd4cd0c to 33bbb6c Compare May 16, 2024 09:41
@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented May 16, 2024

What should I do about the cpp formatter failure? Edit the revert commit so that it doesn't only revert, but also fix formatting? Or add a new commit just to format?

It should be possible to pass the `env` option to a worker
even when the parent process is using a process-level flag, such
as `--title` or a V8-specific flag.

This test is currently failing in Node.js 22.2.0
@RedYetiDev
Copy link
Member

RedYetiDev commented May 16, 2024

What should I do about the cpp formatter failure? Edit the revert commit so that it doesn't only revert, but also fix formatting? Or add a new commit just to format?

According to the error: CLANG_FORMAT_START=$(git merge-base HEAD main) make format-cpp

I misread what you were asking. I'm not in any way the person to give advice on this, but I'd add a new commit for now, they can always be changed later

@RedYetiDev RedYetiDev added the revert PRs that revert previously landed PRs. label May 16, 2024
@aduh95
Copy link
Contributor

aduh95 commented May 17, 2024

What should I do about the cpp formatter failure? Edit the revert commit so that it doesn't only revert, but also fix formatting? Or add a new commit just to format?

I would add a fixup commit (git commit --fixup e05e0724): that simplifies the work of reviewers, and it will land as a single commit.

Shouldn't the revert comes first, then the added test can be applied? Otherwise tests won't be passing on that first commit IIUC

@theanarkh
Copy link
Contributor

I have opened an PR and try to fix this.

@nicolo-ribaudo
Copy link
Contributor Author

Closing in favor of #53029

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. revert PRs that revert previously landed PRs. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v22.2.0 regression] Some Worker use cases are broken
5 participants