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

The path of node js is not correctly recognized on Windows #6069

Closed
Losses opened this issue Jul 18, 2021 · 20 comments · Fixed by #6071 or #6172
Closed

The path of node js is not correctly recognized on Windows #6069

Losses opened this issue Jul 18, 2021 · 20 comments · Fixed by #6071 or #6172

Comments

@Losses
Copy link
Contributor

Losses commented Jul 18, 2021

  • Electron-Builder Version: 22.11.7
  • Node Version: 14.17.3
  • Electron Version: 13.1.6
  • Electron Type (current, beta, nightly): current
  • Target: N/A

While running installing-app-deps or any other commands, following error occurs:

  • electron-builder  version=22.11.7
  • loaded configuration  file=package.json ("build" field)
  • installing production dependencies  platform=win32 arch=x64 appDir=D:\dev\cryptoElectron\src
  • spawning        command=C:\Users\Losse\AppData\Local\Temp\xfs-8e15e263\node yarn install --production cwd=D:\dev\cryptoElectron\src
  • exited          command=node code=-4058 pid=undefined
  ⨯ spawn C:\Users\Losse\AppData\Local\Temp\xfs-8e15e263\node ENOENT  failedTask=installAppDeps stackTrace=Error: spawn C:\Users\Losse\AppData\Local\Temp\xfs-8e15e263\node ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:269:19)
    at onErrorNT (internal/child_process.js:467:16)
    at processTicksAndRejections (internal/process/task_queues.js:82:21)

This may relate to the spawn method, the closest code I found is:

return spawn(execPath, execArgs, {

@Losses
Copy link
Contributor Author

Losses commented Jul 18, 2021

Donation attached:
image

@mmaietta
Copy link
Collaborator

Hi there!
Can you please rerun with env var DEBUG=electron-builder and post the logs back?

@Losses
Copy link
Contributor Author

Losses commented Jul 19, 2021

@mmaietta Nothing happened
image

@Losses
Copy link
Contributor Author

Losses commented Jul 19, 2021

@mmaietta Okay, it's a pretty comperhensive bug:

  1. Yarn 3 was released, but isYarn2 didn't considered it, related code:
    const isYarn2 = npmUserAgent != null && npmUserAgent.startsWith("yarn/2.")

It's recommend to rename it with isYarnBerry which will reflect what really happened.

Another suggestion is: there will be yarn 4, 5, and more, we can't writing things like this:

npmUserAgent.startsWith("yarn/2.") || npmUserAgent.startsWith("yarn/3.") || npmUserAgent.startsWith("yarn/4.") || npmUserAgent.startsWith("yarn/5.") // ...

The first solution comes to my mind is using something like app-root-path, trying to find if there's a folder named .yarn, if there's one, it is running yarn berry.

Or, just simply use a regex, like this:

const MOCK_UA = `yarn/3.0.0-rc.9 npm/? node/14.17.3 win32 x64`;
const regex = /yarn\/(\d+)\./gm;

const yarnVersionMatch = regex.exec(MOCK_UA);
const yarnMajorVersion = yarnVersionMatch?.[1];
const isYarnBerry = yarnMajorVersion >= 2;

The second bug is, process.env.npm_execpath didn't give the correct executable path on Windows.

let execPath = process.env.npm_execpath || process.env.NPM_CLI_JS

What was given:

'C:\\Users\\Losse\\AppData\\Local\\Temp\\xfs-3b46ab91\\yarn'

This didn't work on my machine.

what I did is using which, this package give me the correct executable path of yarn, like this:

    if (isYarn2) {
      const yarnPath = which.sync('yarn');
      execPath = yarnPath;
    }

The new path I got:

'C:\\Users\\Losse\\AppData\\Local\\Temp\\xfs-5f0bf6c0\\yarn.CMD'

With these two patches, the problem was fixed, we should have a discuss about how to make a proper fix about this bug, since many windows users may face the same problem.

image

@mmaietta
Copy link
Collaborator

Wowza, that's quite the investigative results. I definitely would not have picked up on it being a new version of yarn. I probably need to add that into the New Issue template.

We can definitely get the yarn berry support into a PR.

I'm not opposed to using which, but the downside is that being able to specify via env would need to remain to be backward compatible, particularly for CI environments.
Are you able to set/override NPM_CLI_JS for your work environment?

@Losses
Copy link
Contributor Author

Losses commented Jul 19, 2021

@mmaietta since process.env.npm_execpath already have a value, process.env.NPM_CLI_JS won't be used.

I'm really headache about how to handle this, adding another env variable like USE_YARN_COMMAND which will trigger which may be a solution? How can we name this variable is another problem...

@mmaietta
Copy link
Collaborator

Yeah, clearly something is needed, I'm just confused, why is the default process.env.npm_execpath path not having the extension suffix .CMD? That seems more local env related tbh. Do you have another machine you can test with?

@Losses
Copy link
Contributor Author

Losses commented Jul 19, 2021

@mmaietta Ehhhh.... It's hard but I'll try to find one...

@mmaietta
Copy link
Collaborator

Or a VM could work?

If you can put together a sample repo, I can also bootcamp into windows and try it out.

mmaietta added a commit to mmaietta/electron-builder that referenced this issue Jul 19, 2021
@Losses
Copy link
Contributor Author

Losses commented Jul 19, 2021

Or a VM could work?

If you can put together a sample repo, I can also bootcamp into windows and try it out.

I'll have a try

develar pushed a commit that referenced this issue Jul 20, 2021
@Losses
Copy link
Contributor Author

Losses commented Jul 21, 2021

Or a VM could work?

If you can put together a sample repo, I can also bootcamp into windows and try it out.

@mmaietta

I've tried on another Windows machine, same thing happens, just start with a blank electron-react-boilerplate and you can see the bug

@Losses
Copy link
Contributor Author

Losses commented Jul 21, 2021

Maybe we should reopen this issue

@mmaietta mmaietta reopened this Jul 21, 2021
@mmaietta
Copy link
Collaborator

Correct. The yarn3 issue was resolved. But the nodejs path still is the main topic here

@mmaietta
Copy link
Collaborator

mmaietta commented Jul 24, 2021

Can you try setting the value to a proper path? Depending on npm version:

npm config set npm_execpath <value> [-g|--global]
npm config set npm_execpath=<value>

If that doesn't work, then:

process.env.npm_execpath already have a value, process.env.NPM_CLI_JS won't be used.

What if we just switched the logic to check NPM_CLI_JS first? Seems to me that an env override would be better than the standard exec path, right? Not sure on external impact, but if npm_execpath always has a value, then the current env override simply will never work.
That specific logic was added 5 years ago in the initial integration/support for yarn.

@mmaietta
Copy link
Collaborator

mmaietta commented Aug 1, 2021

Hey @Losses, wanted to follow up here. Did any of my proposals work for you?

If the npm config route didn't work, then, here's a patch file you can commit to your repo and persist with patch-package
app-builder-lib+22.11.10.patch

diff --git a/node_modules/app-builder-lib/out/util/yarn.js b/node_modules/app-builder-lib/out/util/yarn.js
index ee0d8db..931c89c 100644
--- a/node_modules/app-builder-lib/out/util/yarn.js
+++ b/node_modules/app-builder-lib/out/util/yarn.js
@@ -75,7 +75,7 @@ function installDependencies(appDir, options) {
     const arch = options.arch || process.arch;
     const additionalArgs = options.additionalArgs;
     builder_util_1.log.info({ platform, arch, appDir }, `installing production dependencies`);
-    let execPath = process.env.npm_execpath || process.env.NPM_CLI_JS;
+    let execPath = process.env.NPM_CLI_JS || process.env.npm_execpath;
     const execArgs = ["install"];
     const isYarnBerry = checkYarnBerry();
     if (!isYarnBerry) {

@Losses
Copy link
Contributor Author

Losses commented Aug 8, 2021

@mmaietta Wow, thanks!

@Losses
Copy link
Contributor Author

Losses commented Aug 21, 2021

@mmaietta Okay, after some research, I think I got the reason, this is related to:

https://github.com/electron-userland/electron-builder/blob/master/packages/builder-util/src/util.ts#L174

The spawn function uses child_process.spawn which don't support PATHEXT, based on the following issues:
nodejs/node-v0.x-archive#2318
nodejs/node#6671

Replace it with cross-spawn will fix the problem, I've confirmed it on my machine.

@Losses
Copy link
Contributor Author

Losses commented Aug 21, 2021

For people who can't wait for a new version and using yarn 2 / yarn 3 on Windows machine:

  1. Create a new patch file under ROOT_PATH_OF_YOUR_PROJECT/patches/builder-util.patch with following content:
diff --git a/out/util.js b/out/util.js
index fbcf30f99ef3d879874b730b349c0bf2ec710734..fe9e1828ff583a7d098cbc8e20a565514e9809ab 100644
--- a/out/util.js
+++ b/out/util.js
@@ -11,6 +11,7 @@ const debug_1 = require("debug");
 const js_yaml_1 = require("js-yaml");
 const path = require("path");
 const log_1 = require("./log");
+const crossSpawn = require("cross-spawn");
 const source_map_support_1 = require("source-map-support");
 if (process.env.JEST_WORKER_ID == null) {
     source_map_support_1.install();
@@ -163,7 +164,7 @@ function doSpawn(command, args, options, extraOptions) {
     }
     logSpawn(command, args, options);
     try {
-        return child_process_1.spawn(command, args, options);
+        return crossSpawn(command, args, options);
     }
     catch (e) {
         throw new Error(`Cannot spawn ${command}: ${e.stack || e}`);
  1. Install cross-spawn:

yarn add cross-spawn

  1. add the following section to your package.json
  "resolutions": {
    "builder-util": "patch:[email protected]#patches/builder-util.patch"
  },

All the things would work now.

@mmaietta
Copy link
Collaborator

I did not realize that this was possible to do!

"resolutions": {
"builder-util": "patch:[email protected]#patches/builder-util.patch"
},

Well played.

Left a small comment on the PR, we can probably get a new version released by end of tomorrow.
The "Release" PR bot triggers an auto-publish on merge 🙂

@Losses
Copy link
Contributor Author

Losses commented Aug 23, 2021

Thank God everything is working now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants