Skip to content

ref: Dont run install script using node binary#1151

Merged
kamilogorek merged 1 commit intomasterfrom
install-script
Mar 17, 2022
Merged

ref: Dont run install script using node binary#1151
kamilogorek merged 1 commit intomasterfrom
install-script

Conversation

@kamilogorek
Copy link
Copy Markdown
Contributor

Instead of relying on some random node, which is decided by the npm on runtime (eg. npm adds everything from node_modules/.bin to $PATH for its lifecycle-scripts), let #!/usr/bin/env node from scripts itself decide. ./install/script.js has all -x chmod flags set correctly, so it won't be a problem executing it.

Copy link
Copy Markdown
Contributor

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Good change. We should enforce this in the SDKs as well.

@kamilogorek kamilogorek merged commit f0ce780 into master Mar 17, 2022
@kamilogorek kamilogorek deleted the install-script branch March 17, 2022 10:53
@beheh
Copy link
Copy Markdown

beheh commented Mar 17, 2022

This PR broke the install of @sentry/cli under Windows (on GitHub Actions) for us:

> @sentry/cli@1.74.1 install C:\npm\prefix\node_modules@sentry\cli
> ./scripts/install.js

'.' is not recognized as an internal or external command,

@victoriaNine
Copy link
Copy Markdown

I'm having the same issue on Windows, it can no longer be installed.

error command C:\WINDOWS\system32\cmd.exe /d /s /c ./scripts/install.js
error '.' is not recognized as an internal or external command

@kamilogorek
Copy link
Copy Markdown
Contributor Author

Oh, excuse me. Fixing now (although gh has some issues so might not be able to release atm)

@kamilogorek
Copy link
Copy Markdown
Contributor Author

@beheh @victoriaNine released patch in 1.74.2

@victoriaNine
Copy link
Copy Markdown

Perfect, working again. Thanks for the quick fix 🙌

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.

4 participants