cli: ensure --run has proper pwd by bmeck · Pull Request #53600 · nodejs/node · GitHub

No description provided.

Small comments. The current test failure is due to test/message/node_run_non_existent.out.

After these small changes, I think we can ship it! Thank you for your contribution (and welcome!)

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

CI: https://ci.nodejs.org/job/node-test-pull-request/60044/

CI: https://ci.nodejs.org/job/node-test-pull-request/60179/

Compilation error on Windows:

  node_task_runner.cc
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(210,27): error C2440: '=': cannot convert from 'const std::filesystem::path::value_type *' to 'const char *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(210,27): message : Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-style cast [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(259,21): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 1 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(259,21): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(259,21): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(259,21): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(259,21): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(276,21): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 1 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(276,21): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(276,21): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(276,21): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(276,21): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(285,15): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 1 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(285,15): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(285,15): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(285,15): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(285,15): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(288,23): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 1 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(288,23): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(288,23): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(288,23): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(288,23): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(297,21): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 1 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(297,21): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(297,21): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(297,21): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(297,21): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(308,15): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 3 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(308,15): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(308,15): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(308,15): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(308,15): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(314,15): warning C4477: 'fprintf' : format string '%s' requires an argument of type 'char *', but variadic argument 3 has type 'const std::filesystem::path::value_type *' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(314,15): message : consider using '%ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(314,15): message : consider using '%lls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(314,15): message : consider using '%Ls' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]
C:\workspace\node-compile-windows\node\src\node_task_runner.cc(314,15): message : consider using '%ws' in the format string [C:\workspace\node-compile-windows\node\libnode.vcxproj]

I'll check these compilation errors.

@StefanStojanovic I believe these likely came from removing some .c_str() calls

The issue here is the character type of std::filesystem::path (value_type) differs on Windows, where it is a wchar_t, compared to other platforms where it is char (Source: https://en.cppreference.com/w/cpp/filesystem/path). So, since that is the reason I've added an ifdef _WIN32 and added code to convert data to the correct format. I was able to compile with this change locally. @bmeck if this change makes sense to you feel free to apply the patch I provided in this comment to your branch.

From fc0f3ed6c020eb0180065bc89bce026b68e2216f Mon Sep 17 00:00:00 2001
From: StefanStojanovic <stefan.stojanovic@janeasystems.com>
Date: Tue, 9 Jul 2024 20:23:48 +0200
Subject: [PATCH 1/1] ProcessRunner::Run Windows fix

---
 src/node_task_runner.cc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc
index cb2defee973..0c37b1072d6 100644
--- a/src/node_task_runner.cc
+++ b/src/node_task_runner.cc
@@ -207,7 +207,15 @@ void ProcessRunner::OnExit(int64_t exit_status, int term_signal) {
 void ProcessRunner::Run() {
   // keeps the string alive until destructor
   auto cwd = package_json_path_.parent_path();
+#ifdef _WIN32
+  std::wstring cwd_wstr = cwd.wstring();
+  int size = WideCharToMultiByte(CP_UTF8, 0, cwd_wstr.c_str(), -1, nullptr, 0, nullptr, nullptr);
+  std::string cwd_string(size, 0);
+  WideCharToMultiByte(CP_UTF8, 0, cwd_wstr.c_str(), -1, &cwd_string[0], size, nullptr, nullptr);
+  options_.cwd = cwd_string.c_str();
+#else
   options_.cwd = cwd.c_str();
+#endif
   if (int r = uv_spawn(loop_, &process_, &options_)) {
     fprintf(stderr, "Error: %s\n", uv_strerror(r));
   }
-- 
2.45.2.windows.1

@StefanStojanovic I'm ok with an ifdef but we would want to add it to quite a few places according to above

@StefanStojanovic I'm ok with an ifdef but we would want to add it to quite a few places according to above

If you are referring to the #53600 (comment), other logs are just warnings so at least the compilation would succeed. A potential problem is that those logs would be unusable. The good thing is that all of the warnings are from the same function RunTask and most of them (all except one) use the same value, so it shouldn't be too hard to fix them all in one place, or simply use %ls instead of %s on Windows.

One question I have though - what would be the easiest way to reproduce showing some of these logs?

You can run path.string() and convert the filesystem path to a std::string. It will create an unnecessary string and copy operation but it will work for both operating systems without any ifdef.

@StefanStojanovic is currently on vacation so I will be handling this in the meantime.

Thanks @anonrig, I've used the function that you've provided.

@bmeck I'm able to compile with the patch below locally. If this fix makes sense to you, feel free to apply it.

diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc
index cb2defee973..429b72e3e5f 100644
--- a/src/node_task_runner.cc
+++ b/src/node_task_runner.cc
@@ -207,7 +207,7 @@ void ProcessRunner::OnExit(int64_t exit_status, int term_signal) {
 void ProcessRunner::Run() {
   // keeps the string alive until destructor
   auto cwd = package_json_path_.parent_path();
-  options_.cwd = cwd.c_str();
+  options_.cwd = cwd.string().c_str();
   if (int r = uv_spawn(loop_, &process_, &options_)) {
     fprintf(stderr, "Error: %s\n", uv_strerror(r));
   }
@@ -256,7 +256,7 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
   auto package_json = FindPackageJson(cwd);
 
   if (!package_json.has_value()) {
-    fprintf(stderr, "Can't find package.json for directory %s\n", cwd.c_str());
+    fprintf(stderr, "Can't find package.json for directory %s\n", cwd.string().c_str());
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
@@ -273,7 +273,7 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
   simdjson::error_code error = json_parser.iterate(raw_json).get(document);
 
   if (error) {
-    fprintf(stderr, "Can't parse %s\n", path.c_str());
+    fprintf(stderr, "Can't parse %s\n", path.string().c_str());
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
@@ -283,9 +283,9 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
     if (root_error == simdjson::error_code::INCORRECT_TYPE) {
       fprintf(stderr,
               "Root value unexpected not an object for %s\n\n",
-              path.c_str());
+              path.string().c_str());
     } else {
-      fprintf(stderr, "Can't parse %s\n", path.c_str());
+      fprintf(stderr, "Can't parse %s\n", path.string().c_str());
     }
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
@@ -294,7 +294,7 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
   // 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 %s\n", path.c_str());
+    fprintf(stderr, "Can't find \"scripts\" field in %s\n", path.string().c_str());
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
@@ -308,13 +308,13 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
               "Script \"%.*s\" is unexpectedly not a string for %s\n\n",
               static_cast<int>(command_id.size()),
               command_id.data(),
-              path.c_str());
+              path.string().c_str());
     } else {
       fprintf(stderr,
               "Missing script: \"%.*s\" for %s\n\n",
               static_cast<int>(command_id.size()),
               command_id.data(),
-              path.c_str());
+              path.string().c_str());
       fprintf(stderr, "Available scripts are:\n");
 
       // Reset the object to iterate over it again

Needs to be rebased with new CI runs. Removed the author ready and commit queue labels.

PR-URL: #54949 Refs: #53600 Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

bmeck added 3 commits June 26, 2024 15:32

cli: ensure --run has proper pwd

0921a18

nit

fb4faed

doc

c0d33ee

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. labels Jun 26, 2024

bmeck marked this pull request as draft June 26, 2024 21:26

anonrig approved these changes Jun 27, 2024

View reviewed changes

Comment thread test/fixtures/run-script/package.json Outdated

anonrig reviewed Jun 27, 2024

View reviewed changes

Update test/fixtures/run-script/package.json

1726d9f

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

bmeck commented Jun 27, 2024

View reviewed changes

Comment thread test/fixtures/run-script/package.json Outdated

bmeck and others added 3 commits June 27, 2024 09:32

Update test/fixtures/run-script/package.json

d167213

Apply suggestions from code review

6f43ae7

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

review nits

250523d

anonrig reviewed Jun 27, 2024

View reviewed changes

Comment thread src/node_task_runner.cc Outdated

anonrig reviewed Jun 27, 2024

View reviewed changes

Comment thread src/node_task_runner.cc Outdated

nits

7cb49aa

anonrig approved these changes Jun 28, 2024

View reviewed changes

Update src/node_task_runner.cc

6b0d81b

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

bmeck marked this pull request as ready for review June 28, 2024 14:11

anonrig approved these changes Jun 28, 2024

View reviewed changes

bmeck added 2 commits June 28, 2024 10:36

remove duplicate test

6ab8d56

fixup

3db4c32

jasnell approved these changes Jun 30, 2024

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024

anonrig mentioned this pull request Jul 8, 2024

src: mark node --run as stable #53763

Closed

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024

StefanStojanovic mentioned this pull request Jul 17, 2024

fs: optimize fs.cpSync js calls #53614

Merged

jasnell removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 8, 2024

anonrig mentioned this pull request Sep 14, 2024

cli: ensure --run has proper pwd #54949

Merged

nodejs-github-bot pushed a commit that referenced this pull request Sep 18, 2024

cli: ensure --run has proper pwd

0c5fa57

PR-URL: #54949 Refs: #53600 Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

targos pushed a commit that referenced this pull request Oct 4, 2024

cli: ensure --run has proper pwd

048a1ab

PR-URL: #54949 Refs: #53600 Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

bmeck closed this Feb 12, 2025

cli: ensure --run has proper pwd by bmeck · Pull Request #53600 · nodejs/node · GitHub

Uh oh!

Uh oh!

cli: ensure --run has proper pwd - #53600

cli: ensure --run has proper pwd#53600bmeck wants to merge 11 commits intonodejs:mainnodejs/node:mainfrom bmeck:bmeck/task-runner-pwdbmeck/node:bmeck/task-runner-pwdCopy head branch name to clipboard

Conversation

bmeck commented Jun 26, 2024

Uh oh!

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 4, 2024

Uh oh!

nodejs-github-bot commented Jul 8, 2024

Uh oh!

aduh95 commented Jul 8, 2024

Uh oh!

StefanStojanovic commented Jul 9, 2024

Uh oh!

bmeck commented Jul 9, 2024

Uh oh!

StefanStojanovic commented Jul 9, 2024

Uh oh!

bmeck commented Jul 9, 2024

Uh oh!

StefanStojanovic commented Jul 9, 2024

Uh oh!

anonrig commented Jul 9, 2024

Uh oh!

huseyinacacak-janea commented Jul 11, 2024

Uh oh!

jasnell commented Sep 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cli: ensure --run has proper pwd#53600
bmeck wants to merge 11 commits into
nodejs:mainfrom
bmeck:bmeck/task-runner-pwd