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

Update Windows install script workaround npm issue #1654

Merged
merged 17 commits into from
Dec 6, 2024
Merged

Conversation

longzheng
Copy link
Contributor

I was just testing the Windows install script recently and noticed that it would actually fail on the npx commands

After some digging it's due to Node.js removing creating the %APPDATA%\npm folder in the MSI installer nodejs/node@0ae8bf8 which then causes the npx command to fail nodejs/node#53538

This was fixed in npm npm/cli#7640 but it's not included with the npm installed in Node 20.11.1 specified in the install script

This npm i -g npm is a workaround nodejs/node#53538 (comment)

Also added #Requires -RunAsAdministrator so the script will error if it's not run under admin

@koush
Copy link
Owner

koush commented Dec 5, 2024

@longzheng just to confirm, this still installs/runs the service itself as user account right?

@longzheng
Copy link
Contributor Author

longzheng commented Dec 5, 2024

@koush Correct, this should keep the service as user account.

However I'm a bit confused why

I'm going to do a bit more testing with a local VM and report back

@longzheng
Copy link
Contributor Author

OK I confirmed on a brand new Windows 11 VM, the current script (without the fix) definitely errors when running the npx -y scrypted@latest install-server command due to the aforementioned change to the MSI installer

image

However, the server continues to work

image

More questions

  • what is the effect of not completing npx -y scrypted@latest install-server on the install
  • why does fixing the command then break the server

@longzheng
Copy link
Contributor Author

Hmm I confirmed in a fresh VM that the new script fixes the npx error

image

And the server still works

image

Now I'm extra confused

@longzheng
Copy link
Contributor Author

That is so weird, removing the npm i -g npm fixes the problem for the test, but I can't replicate that in my VM.

@koush
Copy link
Owner

koush commented Dec 5, 2024

Maybe the easier fix is to simply create the missing directory?

@longzheng
Copy link
Contributor Author

Yeah good idea. The fact that I can't repro it is kind of weird, I wonder how else is the test runner different.

@longzheng
Copy link
Contributor Author

Although if the server still runs even with that command failing, is the command necessary? How can the service/server run without the install?

@koush
Copy link
Owner

koush commented Dec 5, 2024

The npx command that runs it will also install it if it's missing. Used to handle updates too.

@longzheng
Copy link
Contributor Author

The npx command that runs it will also install it if it's missing. Used to handle updates too.

Do you think then maybe it's simpler to not run the npx command in the install script and just let the serve service handle it?

@koush
Copy link
Owner

koush commented Dec 6, 2024

the reason the install is done is because if would cause a long first run after install. and installation failures would be opaque.

@longzheng
Copy link
Contributor Author

OK that's fair enough.

I'm super curious then why the test server would fail with this, I need to get the logs. I might try modify the workflow to dump the output of the daemon logs.

@longzheng
Copy link
Contributor Author

@koush sorry to be a bother could you kill this run since it'll go on forever https://github.com/koush/scrypted/actions/runs/12193672987/job/34016265645

@longzheng
Copy link
Contributor Author

longzheng commented Dec 6, 2024

I logged npm -v before the script installs Node and I figured out the reason why the test server doesn't have the NPM problem, it's because it has NPM built-in to the image (and it's version v18.20.5 so it doesn't have the installer change), so the %APPDATA%\npm folder must exist by default

https://github.com/koush/scrypted/actions/runs/12194017927/job/34017187505?pr=1654#step:4:12

EDIT: I also confirmed with a second npm -v after choco upgrade -y nodejs-lts --version=20.11.1 that it's still v18.20.5 so Node is not actually getting updated at all.

EDIT 2: Confirmed the Windows image has Node 18 built-in https://github.com/actions/runner-images/blob/main/images/windows/Windows2022-Readme.md

@longzheng
Copy link
Contributor Author

OK I finally figured out what's happening.

Before the change:

  • test server has Node 18
  • install script did not upgrade to Node 20
  • the server runs correctly

After the npm i -g npm change:

  • test server still has Node 18
  • npm was updated to latest version
  • latest version of npm includes a npx.ps1 file for Powershell (whereas the npm installed on the server only had npm.cmd)
  • the $NPX_PATH = (Get-Command npx).Path command would return npx.ps1 since we're in a Powershell install script
  • The service.js was trying to spawn a child process with npx.ps1 which does not work since the shell is not Powershell

New change:

  • replace the $NPX_PATH_ESCAPED .ps1 with .cmd

@koush koush merged commit a7424b3 into koush:main Dec 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants