-
Notifications
You must be signed in to change notification settings - Fork 6k
fix(ext/process): tolerate unlinked cwd and accept top-level --interactive Node compat flag #33587
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
6265344
e0df4fb
933c756
55db3ed
07ece2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -576,7 +576,9 @@ fn create_command( | |
| command.args(args.args); | ||
| } | ||
|
|
||
| command.current_dir(run_env.cwd); | ||
| if let Some(cwd) = run_env.cwd { | ||
| command.current_dir(cwd); | ||
| } | ||
| command.env_clear(); | ||
| command.envs(run_env.envs.into_iter().map(|(k, v)| (k.into_inner(), v))); | ||
|
|
||
|
|
@@ -1138,7 +1140,10 @@ impl std::cmp::PartialEq for EnvVarKey { | |
|
|
||
| struct RunEnv { | ||
| envs: HashMap<EnvVarKey, OsString>, | ||
| cwd: PathBuf, | ||
| /// `None` means "inherit the parent's cwd". This happens when no cwd was | ||
| /// explicitly passed and the current working directory could not be | ||
| /// resolved (e.g. because it was unlinked, matching Node.js semantics). | ||
| cwd: Option<PathBuf>, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this change...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in e0df4fb:
|
||
| } | ||
|
|
||
| /// Computes the current environment, which will then be used to inform | ||
|
|
@@ -1155,11 +1160,21 @@ fn compute_run_env( | |
| clippy::disallowed_methods, | ||
| reason = "ok for now because launching a sub process requires the real fs" | ||
| )] | ||
| let cwd = | ||
| std::env::current_dir().map_err(ProcessError::FailedResolvingCwd)?; | ||
| let cwd = arg_cwd | ||
| .map(|cwd_arg| resolve_path(cwd_arg, &cwd)) | ||
| .unwrap_or(cwd); | ||
| let current_dir = std::env::current_dir(); | ||
| let cwd = match arg_cwd { | ||
| Some(cwd_arg) => { | ||
| let arg_path = Path::new(cwd_arg); | ||
| if arg_path.is_absolute() { | ||
| Some( | ||
| deno_path_util::normalize_path(Cow::Borrowed(arg_path)).into_owned(), | ||
| ) | ||
| } else { | ||
| let base = current_dir.map_err(ProcessError::FailedResolvingCwd)?; | ||
| Some(resolve_path(cwd_arg, &base)) | ||
| } | ||
| } | ||
| None => current_dir.ok(), | ||
| }; | ||
| let envs = if arg_clear_env { | ||
| arg_envs | ||
| .iter() | ||
|
|
@@ -1182,14 +1197,25 @@ fn resolve_cmd(cmd: &str, env: &RunEnv) -> Result<PathBuf, ProcessError> { | |
| #[cfg(windows)] | ||
| let is_path = is_path || cmd.contains('\\') || Path::new(&cmd).is_absolute(); | ||
| if is_path { | ||
| Ok(resolve_path(cmd, &env.cwd)) | ||
| let cmd_path = Path::new(cmd); | ||
| if cmd_path.is_absolute() { | ||
| Ok(deno_path_util::normalize_path(Cow::Borrowed(cmd_path)).into_owned()) | ||
| } else { | ||
| let cwd = env.cwd.as_deref().ok_or_else(|| { | ||
| ProcessError::FailedResolvingCwd(std::io::Error::from( | ||
| std::io::ErrorKind::NotFound, | ||
| )) | ||
| })?; | ||
| Ok(resolve_path(cmd, cwd)) | ||
| } | ||
| } else { | ||
| let path = env.envs.get(&EnvVarKey::new(OsString::from("PATH"))); | ||
| let lookup_cwd = env.cwd.clone().unwrap_or_else(|| PathBuf::from(".")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth a sanity check on let lookup_cwd = env.cwd.clone().unwrap_or_else(|| PathBuf::from("."));If the user spawns a binary by a non-PATH non-absolute name (e.g., The test ( Not blocking. Worth a one-line comment that the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in e0df4fb — added a comment above the |
||
| match deno_permissions::which::which_in( | ||
| sys_traits::impls::RealSys, | ||
| cmd, | ||
| path.cloned(), | ||
| env.cwd.clone(), | ||
| lookup_cwd, | ||
| ) { | ||
| Ok(cmd) => Ok(cmd), | ||
| Err(deno_permissions::which::Error::CannotFindBinaryPath) => { | ||
|
|
@@ -1665,7 +1691,9 @@ mod deprecated { | |
| for arg in args.iter().skip(1) { | ||
| c.arg(arg); | ||
| } | ||
| c.current_dir(run_env.cwd); | ||
| if let Some(cwd) = run_env.cwd { | ||
| c.current_dir(cwd); | ||
| } | ||
|
|
||
| c.env_clear(); | ||
| for (key, value) in run_env.envs { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this one... if it's needed for tests then we should improve our shim wrapper to handle that correctly (
libs/node_shim/)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shim wrapper (
libs/node_shim/) only sees the args when the user (or a child spawn) invokes thenode_shimbinary directly. The Node-compat test that motivates this PR (tests/node_compat/runner/suite/test/parallel/test-cwd-enoent-repl.js) does:process.execPathinside Deno isDeno.execPath()— thedenobinary, notnode_shim. So the args reach Deno's clap parser unmediated. Routing through the shim would mean either (a) shipping the shim alongsidedenoand pointingprocess.execPathat it in node-compat mode, or (b) having Deno embednode_shim::parse_argsas a pre-clap fallback for unknown args. Both are bigger architectural moves than this PR; happy to do them in a follow-up if you'd prefer.For this PR I kept the flag minimal: hidden, top-level only (not
.global()), and the parsing branch only forces REPL when no other meaningful args are present. That limits the surface area to what the test (and any analogous Node-mimicking spawn) actually exercises. WDYT — keep it as-is and move the shim integration to a follow-up, or block this on doing the shim integration first?