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

node: need a way to spawn process.execPath with no deno specific flags #2674

Closed
cjihrig opened this issue Sep 20, 2022 · 6 comments
Closed
Labels
bug Something isn't working

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Sep 20, 2022

Describe the bug
Many Node.js tests spawn the node executable in a child process. This is separate from child_process.fork(). As an example, see this test. The problem in that test is that in Deno's compat mode, the spawnSync() call needs to be updated to pass in necessary Deno CLI flags (-A, --unstable, etc.).

For users, this is a problem because they can't run existing Node code as is. For us, it's also a problem because by modifying the Node tests, we lose the ability to automatically upgrade them to newer versions of Node.

A few ideas on how to approach this:

  • In the compat code, detect when we are spawning the current executable and set an environment variable that allows Deno to start up properly with no CLI flags. This would require changes in core.
  • In the compat code, detect when we are spawning the current executable and add the CLI flags. This could be done in std alone, but might have some edge cases.

Steps to Reproduce

Spawn process.execPath in Node compat mode without changing any of the CLI flags.

Expected behavior

Node compat code runs without modifications.

Environment

  • OS: all
  • deno version: 1.25.3
  • std version: 0.156.0
@cjihrig cjihrig added bug Something isn't working needs triage and removed needs triage labels Sep 20, 2022
@kt3k
Copy link
Member

kt3k commented Sep 20, 2022

In the compat code, detect when we are spawning the current executable and set an environment variable that allows Deno to start up properly with no CLI flags. This would require changes in core.
In the compat code, detect when we are spawning the current executable and add the CLI flags. This could be done in std alone, but might have some edge cases.

The first solution might be ideal, but sounds adding lot of complexity to CLI entrypoint.

I think we should try the 2nd solution first and see how many cases it can cover.

cc @bartlomieju what do you think?

@cmoog
Copy link
Contributor

cmoog commented Sep 21, 2022

I also encountered this here: denoland/deno#15836
With an incomplete implementation of "option 2": https://github.com/cmoog/deno_std/commit/638f59db57fec73ce49125ea58305e7cfd1567eb

@bartlomieju
Copy link
Member

In the compat code, detect when we are spawning the current executable and set an environment variable that allows Deno to start up properly with no CLI flags. This would require changes in core.
In the compat code, detect when we are spawning the current executable and add the CLI flags. This could be done in std alone, but might have some edge cases.

The first solution might be ideal, but sounds adding lot of complexity to CLI entrypoint.

I think we should try the 2nd solution first and see how many cases it can cover.

cc @bartlomieju what do you think?

I agree with that, we should go with 2nd solution for now.

@cjihrig cjihrig self-assigned this Sep 21, 2022
cjihrig added a commit to cjihrig/deno_std that referenced this issue Sep 21, 2022
This commit updates test-child-process-spawnsync-env.js to match
the test from Node.js with two exceptions noted in a TODO
comment. These TODOs should be addressable once Deno <--> Node
CLI argument mapping is implemented and there is a way to
start Deno with require() available.

Refs: denoland#2674
cjihrig added a commit that referenced this issue Sep 21, 2022
This commit updates test-child-process-spawnsync-env.js to match
the test from Node.js with two exceptions noted in a TODO
comment. These TODOs should be addressable once Deno <--> Node
CLI argument mapping is implemented and there is a way to
start Deno with require() available.

Refs: #2674
cjihrig added a commit to cjihrig/deno_std that referenced this issue Sep 22, 2022
This commit adds initial support for mapping Node CLI flags to
Deno CLI flags. This is done to better support the case where
Node compat mode is used to spawn another Node subprocess (that
is actually Deno in compat mode - not at all confusing).

Refs: denoland#2674
cjihrig added a commit to cjihrig/deno_std that referenced this issue Sep 22, 2022
This commit adds initial support for mapping Node CLI flags to
Deno CLI flags. This is done to better support the case where
Node compat mode is used to spawn another Node subprocess (that
is actually Deno in compat mode - not at all confusing).

Refs: denoland#2674
cjihrig added a commit to cjihrig/deno_std that referenced this issue Sep 22, 2022
This commit adds initial support for mapping Node CLI flags to
Deno CLI flags. This is done to better support the case where
Node compat mode is used to spawn another Node subprocess (that
is actually Deno in compat mode - not at all confusing).

Refs: denoland#2674
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 22, 2022

I created an initial version of the CLI argument mapper (option 2) in #2688. The CI is passing, but I personally think that option 1 is the better solution in the long term due to the many edge cases in option 2.

We also need a way to spawn a Deno child process with require() available. See node/_tools/test/parallel/test-child-process-spawnsync-env.js as an example of why we need that.

cjihrig added a commit to cjihrig/deno_std that referenced this issue Sep 27, 2022
This commit adds initial support for mapping Node CLI flags to
Deno CLI flags. This is done to better support the case where
Node compat mode is used to spawn another Node subprocess (that
is actually Deno in compat mode - not at all confusing).

Refs: denoland#2674
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 27, 2022

Discussed this internally today. The plan is to land #2688 and begin working on a Node argument parser in Deno. Once that is ready, the mapping logic in #2688 can be ripped back out.

cjihrig added a commit that referenced this issue Sep 27, 2022
This commit adds initial support for mapping Node CLI flags to
Deno CLI flags. This is done to better support the case where
Node compat mode is used to spawn another Node subprocess (that
is actually Deno in compat mode - not at all confusing).

Refs: #2674
@lino-levan
Copy link
Contributor

lino-levan commented Oct 30, 2022

Compat mode no longer exists, is this still relevant? #2688 landed a while ago and I haven't seen a lot of movement on this issue. If there is something to be done, I'd love to pick it up.

@cjihrig cjihrig removed their assignment Dec 6, 2022
@cjihrig cjihrig closed this as not planned Won't fix, can't repro, duplicate, stale Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants