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

fix(ext/node): set correct process.argv0 #22555

Merged
merged 3 commits into from
Feb 23, 2024
Merged

fix(ext/node): set correct process.argv0 #22555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc891d2
fix(ext/node): set correct process.argv0
satyarohith Feb 23, 2024
6d8b1fc
update test to match exact value too
satyarohith Feb 23, 2024
0aeef4a
fix: set argv0 during bootstrap
satyarohith Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cli/factory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,9 +855,10 @@ impl CliFactory {
location: self.options.location_flag().clone(),
// if the user ran a binary command, we'll need to set process.argv[0]
// to be the name of the binary command instead of deno
maybe_binary_npm_command_name: self
argv0: self
.options
.take_binary_npm_command_name(),
.take_binary_npm_command_name()
.or(std::env::args().next()),
origin_data_folder_path: Some(self.deno_dir()?.origin_data_folder_path()),
seed: self.options.seed(),
unsafely_ignore_certificate_errors: self
Expand Down
9 changes: 4 additions & 5 deletions cli/standalone/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,11 +536,10 @@ pub async fn run(
is_npm_main: main_module.scheme() == "npm",
skip_op_registration: true,
location: metadata.location,
maybe_binary_npm_command_name: NpmPackageReqReference::from_specifier(
main_module,
)
.ok()
.map(|req_ref| npm_pkg_req_ref_to_binary_command(&req_ref)),
argv0: NpmPackageReqReference::from_specifier(main_module)
.ok()
.map(|req_ref| npm_pkg_req_ref_to_binary_command(&req_ref))
.or(std::env::args().next()),
origin_data_folder_path: None,
seed: metadata.seed,
unsafely_ignore_certificate_errors: metadata
Expand Down
12 changes: 3 additions & 9 deletions cli/worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ pub struct CliMainWorkerOptions {
pub is_inspecting: bool,
pub is_npm_main: bool,
pub location: Option<Url>,
pub maybe_binary_npm_command_name: Option<String>,
pub argv0: Option<String>,
pub origin_data_folder_path: Option<PathBuf>,
pub seed: Option<u64>,
pub unsafely_ignore_certificate_errors: Option<Vec<String>>,
Expand Down Expand Up @@ -608,10 +608,7 @@ impl CliMainWorkerFactory {
user_agent: version::get_user_agent().to_string(),
inspect: shared.options.is_inspecting,
has_node_modules_dir: shared.options.has_node_modules_dir,
maybe_binary_npm_command_name: shared
.options
.maybe_binary_npm_command_name
.clone(),
argv0: shared.options.argv0.clone(),
node_ipc_fd: shared.node_ipc,
disable_deprecated_api_warning: shared.disable_deprecated_api_warning,
verbose_deprecated_api_warning: shared.verbose_deprecated_api_warning,
Expand Down Expand Up @@ -815,10 +812,7 @@ fn create_web_worker_callback(
user_agent: version::get_user_agent().to_string(),
inspect: shared.options.is_inspecting,
has_node_modules_dir: shared.options.has_node_modules_dir,
maybe_binary_npm_command_name: shared
.options
.maybe_binary_npm_command_name
.clone(),
argv0: shared.options.argv0.clone(),
node_ipc_fd: None,
disable_deprecated_api_warning: shared.disable_deprecated_api_warning,
verbose_deprecated_api_warning: shared.verbose_deprecated_api_warning,
Expand Down
15 changes: 3 additions & 12 deletions ext/node/polyfills/process.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ import { isWindows } from "ext:deno_node/_util/os.ts";
import * as io from "ext:deno_io/12_io.js";
import { Command } from "ext:runtime/40_process.js";

let argv0Getter = () => "";
export let argv0 = "deno";
export let argv0 = "";

// TODO(kt3k): This should be set at start up time
export let arch = "";
Expand Down Expand Up @@ -395,9 +394,6 @@ class Process extends EventEmitter {
argv = argv;

get argv0() {
if (!argv0) {
argv0 = argv0Getter();
}
return argv0;
}

Expand Down Expand Up @@ -887,19 +883,14 @@ internals.__bootstrapNodeProcess = function (
) {
// Overwrites the 1st item with getter.
if (typeof argv0Val === "string") {
argv0 = argv0Val;
Object.defineProperty(argv, "0", {
get: () => {
return argv0Val;
},
});
argv0Getter = () => argv0Val;
} else {
Object.defineProperty(argv, "0", {
get: () => {
return Deno.execPath();
},
});
argv0Getter = () => Deno.execPath();
Object.defineProperty(argv, "0", { get: () => argv0 });
}

// Overwrites the 2st item with getter.
Expand Down
8 changes: 4 additions & 4 deletions runtime/js/99_main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ function bootstrapMainRuntime(runtimeOptions) {
2: unstableFeatures,
3: inspectFlag,
5: hasNodeModulesDir,
6: maybeBinaryNpmCommandName,
6: argv0,
7: shouldDisableDeprecatedApiWarning,
8: shouldUseVerboseDeprecatedApiWarning,
9: future,
Expand Down Expand Up @@ -768,7 +768,7 @@ function bootstrapMainRuntime(runtimeOptions) {
ObjectDefineProperty(globalThis, "Deno", core.propReadOnly(finalDenoNs));

if (nodeBootstrap) {
nodeBootstrap(hasNodeModulesDir, maybeBinaryNpmCommandName);
nodeBootstrap(hasNodeModulesDir, argv0);
}

if (future) {
Expand All @@ -793,7 +793,7 @@ function bootstrapWorkerRuntime(
2: unstableFeatures,
4: enableTestingFeaturesFlag,
5: hasNodeModulesDir,
6: maybeBinaryNpmCommandName,
6: argv0,
7: shouldDisableDeprecatedApiWarning,
8: shouldUseVerboseDeprecatedApiWarning,
} = runtimeOptions;
Expand Down Expand Up @@ -897,7 +897,7 @@ function bootstrapWorkerRuntime(
ObjectDefineProperty(globalThis, "Deno", core.propReadOnly(finalDenoNs));

if (nodeBootstrap) {
nodeBootstrap(hasNodeModulesDir, maybeBinaryNpmCommandName);
nodeBootstrap(hasNodeModulesDir, argv0);
}
}

Expand Down
8 changes: 4 additions & 4 deletions runtime/worker_bootstrap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub struct BootstrapOptions {
pub user_agent: String,
pub inspect: bool,
pub has_node_modules_dir: bool,
pub maybe_binary_npm_command_name: Option<String>,
pub argv0: Option<String>,
pub node_ipc_fd: Option<i64>,
pub disable_deprecated_api_warning: bool,
pub verbose_deprecated_api_warning: bool,
Expand Down Expand Up @@ -89,7 +89,7 @@ impl Default for BootstrapOptions {
inspect: Default::default(),
args: Default::default(),
has_node_modules_dir: Default::default(),
maybe_binary_npm_command_name: None,
argv0: None,
node_ipc_fd: None,
disable_deprecated_api_warning: false,
verbose_deprecated_api_warning: false,
Expand Down Expand Up @@ -121,7 +121,7 @@ struct BootstrapV8<'a>(
bool,
// has_node_modules_dir
bool,
// maybe_binary_npm_command_name
// argv0
Option<&'a str>,
// disable_deprecated_api_warning,
bool,
Expand All @@ -147,7 +147,7 @@ impl BootstrapOptions {
self.inspect,
self.enable_testing_features,
self.has_node_modules_dir,
self.maybe_binary_npm_command_name.as_deref(),
self.argv0.as_deref(),
self.disable_deprecated_api_warning,
self.verbose_deprecated_api_warning,
self.future,
Expand Down
12 changes: 11 additions & 1 deletion tests/unit_node/process_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,17 @@ Deno.test({

Deno.test({
name: "process.argv0",
fn() {
async fn() {
const { stdout } = await new Deno.Command(Deno.execPath(), {
args: [
"eval",
`import process from "node:process";console.log(process.argv0);`,
],
stdout: "piped",
Comment on lines +273 to +277
Copy link
Member

Choose a reason for hiding this comment

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

Can you add another test that does: import { argv0 } from "node:process" and checks the value?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bartlomieju opened another PR #22568 as I had auto-merge enabled for this one.

stderr: "null",
}).output();
assertEquals(new TextDecoder().decode(stdout).trim(), Deno.execPath());

assertEquals(typeof process.argv0, "string");
assert(
process.argv0.match(/[^/\\]*deno[^/\\]*$/),
Expand Down