cli: ensure --run has proper pwd

PR-URL: nodejs#54949 Refs: nodejs#53600 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
louwers · Nov 2, 2024 · 4c2b45e · 4c2b45e
1 parent b0f8878
commit 4c2b45e
Show file tree

Hide file tree

Showing 9 changed files with 180 additions and 36 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -2110,6 +2110,8 @@ the current directory, to the `PATH` in order to execute the binaries from
 different folders where multiple `node_modules` directories are present, if
 `ancestor-folder/node_modules/.bin` is a directory.
 
+`--run` executes the command in the directory containing the related `package.json`.
+
 For example, the following command will run the `test` script of
 the `package.json` in the current folder:
 

diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc
@@ -149,7 +149,7 @@ std::string EscapeShell(const std::string_view input) {
 #endif
   }
 
-  static const std::string_view forbidden_characters =
+  static constexpr std::string_view forbidden_characters =
       "[\t\n\r \"#$&'()*;<>?\\\\`|~]";
 
   // Check if input contains any forbidden characters
@@ -191,7 +191,7 @@ std::string EscapeShell(const std::string_view input) {
 void ProcessRunner::ExitCallback(uv_process_t* handle,
                                  int64_t exit_status,
                                  int term_signal) {
-  auto self = reinterpret_cast<ProcessRunner*>(handle->data);
+  const auto self = static_cast<ProcessRunner*>(handle->data);
   uv_close(reinterpret_cast<uv_handle_t*>(handle), nullptr);
   self->OnExit(exit_status, term_signal);
 }
@@ -205,6 +205,9 @@ void ProcessRunner::OnExit(int64_t exit_status, int term_signal) {
 }
 
 void ProcessRunner::Run() {
+  // keeps the string alive until destructor
+  cwd = package_json_path_.parent_path().string();
+  options_.cwd = cwd.c_str();
   if (int r = uv_spawn(loop_, &process_, &options_)) {
     fprintf(stderr, "Error: %s\n", uv_strerror(r));
   }
@@ -246,14 +249,16 @@ FindPackageJson(const std::filesystem::path& cwd) {
   return {{package_json_path, raw_content, path_env_var}};
 }
 
-void RunTask(std::shared_ptr<InitializationResultImpl> result,
+void RunTask(const std::shared_ptr<InitializationResultImpl>& result,
              std::string_view command_id,
              const std::vector<std::string_view>& positional_args) {
   auto cwd = std::filesystem::current_path();
   auto package_json = FindPackageJson(cwd);
 
   if (!package_json.has_value()) {
-    fprintf(stderr, "Can't read package.json\n");
+    fprintf(stderr,
+            "Can't find package.json for directory %s\n",
+            cwd.string().c_str());
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
@@ -267,46 +272,67 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
   simdjson::ondemand::parser json_parser;
   simdjson::ondemand::document document;
   simdjson::ondemand::object main_object;
-  simdjson::error_code error = json_parser.iterate(raw_json).get(document);
 
+  if (json_parser.iterate(raw_json).get(document)) {
+    fprintf(stderr, "Can't parse %s\n", path.string().c_str());
+    result->exit_code_ = ExitCode::kGenericUserError;
+    return;
+  }
   // If document is not an object, throw an error.
-  if (error || document.get_object().get(main_object)) {
-    fprintf(stderr, "Can't parse package.json\n");
+  if (auto root_error = document.get_object().get(main_object)) {
+    if (root_error == simdjson::error_code::INCORRECT_TYPE) {
+      fprintf(stderr,
+              "Root value unexpected not an object for %s\n\n",
+              path.string().c_str());
+    } else {
+      fprintf(stderr, "Can't parse %s\n", path.string().c_str());
+    }
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
 
   // If package_json object doesn't have "scripts" field, throw an error.
   simdjson::ondemand::object scripts_object;
   if (main_object["scripts"].get_object().get(scripts_object)) {
-    fprintf(stderr, "Can't find \"scripts\" field in package.json\n");
+    fprintf(
+        stderr, "Can't find \"scripts\" field in %s\n", path.string().c_str());
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
 
   // If the command_id is not found in the scripts object, throw an error.
   std::string_view command;
-  if (scripts_object[command_id].get_string().get(command)) {
-    fprintf(stderr,
-            "Missing script: \"%.*s\"\n\n",
-            static_cast<int>(command_id.size()),
-            command_id.data());
-    fprintf(stderr, "Available scripts are:\n");
-
-    // Reset the object to iterate over it again
-    scripts_object.reset();
-    simdjson::ondemand::value value;
-    for (auto field : scripts_object) {
-      std::string_view key_str;
-      std::string_view value_str;
-      if (!field.unescaped_key().get(key_str) && !field.value().get(value) &&
-          !value.get_string().get(value_str)) {
-        fprintf(stderr,
-                "  %.*s: %.*s\n",
-                static_cast<int>(key_str.size()),
-                key_str.data(),
-                static_cast<int>(value_str.size()),
-                value_str.data());
+  if (auto command_error =
+          scripts_object[command_id].get_string().get(command)) {
+    if (command_error == simdjson::error_code::INCORRECT_TYPE) {
+      fprintf(stderr,
+              "Script \"%.*s\" is unexpectedly not a string for %s\n\n",
+              static_cast<int>(command_id.size()),
+              command_id.data(),
+              path.string().c_str());
+    } else {
+      fprintf(stderr,
+              "Missing script: \"%.*s\" for %s\n\n",
+              static_cast<int>(command_id.size()),
+              command_id.data(),
+              path.string().c_str());
+      fprintf(stderr, "Available scripts are:\n");
+
+      // Reset the object to iterate over it again
+      scripts_object.reset();
+      simdjson::ondemand::value value;
+      for (auto field : scripts_object) {
+        std::string_view key_str;
+        std::string_view value_str;
+        if (!field.unescaped_key().get(key_str) && !field.value().get(value) &&
+            !value.get_string().get(value_str)) {
+          fprintf(stderr,
+                  "  %.*s: %.*s\n",
+                  static_cast<int>(key_str.size()),
+                  key_str.data(),
+                  static_cast<int>(value_str.size()),
+                  value_str.data());
+        }
       }
     }
     result->exit_code_ = ExitCode::kGenericUserError;

diff --git a/src/node_task_runner.h b/src/node_task_runner.h
@@ -44,6 +44,7 @@ class ProcessRunner {
   std::vector<std::string> env_vars_{};
   std::unique_ptr<char* []> env {};  // memory for options_.env
   std::unique_ptr<char* []> arg {};  // memory for options_.args
+  std::string cwd;
 
   // OnExit is the callback function that is called when the process exits.
   void OnExit(int64_t exit_status, int term_signal);
@@ -78,7 +79,7 @@ class ProcessRunner {
 std::optional<std::tuple<std::filesystem::path, std::string, std::string>>
 FindPackageJson(const std::filesystem::path& cwd);
 
-void RunTask(std::shared_ptr<InitializationResultImpl> result,
+void RunTask(const std::shared_ptr<InitializationResultImpl>& result,
              std::string_view command_id,
              const PositionalArgs& positional_args);
 PositionalArgs GetPositionalArgs(const std::vector<std::string>& args);

diff --git a/test/fixtures/run-script/invalid-json/package.json b/test/fixtures/run-script/invalid-json/package.json
@@ -0,0 +1,3 @@
+{
+  "scripts": {},
+}
diff --git a/test/fixtures/run-script/invalid-schema/package.json b/test/fixtures/run-script/invalid-schema/package.json
@@ -0,0 +1,9 @@
+{
+  "scripts": {
+    "array": [],
+    "boolean": true,
+    "null": null,
+    "number": 1.0,
+    "object": {}
+  }
+}
diff --git a/test/fixtures/run-script/missing-scripts/package.json b/test/fixtures/run-script/missing-scripts/package.json
@@ -0,0 +1 @@
+{}
diff --git a/test/fixtures/run-script/package.json b/test/fixtures/run-script/package.json
@@ -10,6 +10,8 @@
     "path-env": "path-env",
     "path-env-windows": "path-env.bat",
     "special-env-variables": "special-env-variables",
-    "special-env-variables-windows": "special-env-variables.bat"
+    "special-env-variables-windows": "special-env-variables.bat",
+    "pwd": "pwd",
+    "pwd-windows": "cd"
   }
 }
diff --git a/test/message/node_run_non_existent.out b/test/message/node_run_non_existent.out
@@ -1,4 +1,4 @@
-Missing script: "non-existent-command"
+Missing script: "non-existent-command" for *
 
 Available scripts are:
   test: echo "Error: no test specified" && exit 1
@@ -12,3 +12,5 @@ Available scripts are:
   path-env-windows: path-env.bat
   special-env-variables: special-env-variables
   special-env-variables-windows: special-env-variables.bat
+  pwd: pwd
+  pwd-windows: cd
diff --git a/test/parallel/test-node-run.js b/test/parallel/test-node-run.js
@@ -1,6 +1,8 @@
 'use strict';
 
 const common = require('../common');
+common.requireNoPackageJSONAbove();
+
 const { it, describe } = require('node:test');
 const assert = require('node:assert');
 
@@ -15,7 +17,6 @@ describe('node --run [command]', () => {
       { cwd: __dirname },
     );
     assert.match(child.stderr, /ExperimentalWarning: Task runner is an experimental feature and might change at any time/);
-    assert.match(child.stderr, /Can't read package\.json/);
     assert.strictEqual(child.stdout, '');
     assert.strictEqual(child.code, 1);
   });
@@ -26,7 +27,9 @@ describe('node --run [command]', () => {
       [ '--no-warnings', '--run', 'test'],
       { cwd: __dirname },
     );
-    assert.match(child.stderr, /Can't read package\.json/);
+    assert.match(child.stderr, /Can't find package\.json[\s\S]*/);
+    // Ensure we show the path that starting path for the search
+    assert(child.stderr.includes(__dirname));
     assert.strictEqual(child.stdout, '');
     assert.strictEqual(child.code, 1);
   });
@@ -53,6 +56,101 @@ describe('node --run [command]', () => {
     assert.strictEqual(child.code, 0);
   });
 
+  it('chdirs into package directory', async () => {
+    const child = await common.spawnPromisified(
+      process.execPath,
+      [ '--no-warnings', '--run', `pwd${envSuffix}`],
+      { cwd: fixtures.path('run-script/sub-directory') },
+    );
+    assert.strictEqual(child.stdout.trim(), fixtures.path('run-script'));
+    assert.strictEqual(child.stderr, '');
+    assert.strictEqual(child.code, 0);
+  });
+
+  it('includes actionable info when possible', async () => {
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'missing'],
+        { cwd: fixtures.path('run-script') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/package.json')));
+      assert(child.stderr.includes('no test specified'));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'test'],
+        { cwd: fixtures.path('run-script/missing-scripts') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/missing-scripts/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'test'],
+        { cwd: fixtures.path('run-script/invalid-json') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/invalid-json/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'array'],
+        { cwd: fixtures.path('run-script/invalid-schema') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'boolean'],
+        { cwd: fixtures.path('run-script/invalid-schema') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'null'],
+        { cwd: fixtures.path('run-script/invalid-schema') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'number'],
+        { cwd: fixtures.path('run-script/invalid-schema') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+    {
+      const child = await common.spawnPromisified(
+        process.execPath,
+        [ '--no-warnings', '--run', 'object'],
+        { cwd: fixtures.path('run-script/invalid-schema') },
+      );
+      assert.strictEqual(child.stdout, '');
+      assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json')));
+      assert.strictEqual(child.code, 1);
+    }
+  });
+
   it('appends positional arguments', async () => {
     const child = await common.spawnPromisified(
       process.execPath,
@@ -120,7 +218,7 @@ describe('node --run [command]', () => {
       [ '--no-warnings', '--run', 'test'],
       { cwd: fixtures.path('run-script/cannot-parse') },
     );
-    assert.match(child.stderr, /Can't parse package\.json/);
+    assert.match(child.stderr, /Can't parse/);
     assert.strictEqual(child.stdout, '');
     assert.strictEqual(child.code, 1);
   });
@@ -131,7 +229,7 @@ describe('node --run [command]', () => {
       [ '--no-warnings', '--run', 'test'],
       { cwd: fixtures.path('run-script/cannot-find-script') },
     );
-    assert.match(child.stderr, /Can't find "scripts" field in package\.json/);
+    assert.match(child.stderr, /Can't find "scripts" field in/);
     assert.strictEqual(child.stdout, '');
     assert.strictEqual(child.code, 1);
   });