fix: Manually download binary if optional dependency binary can't be found after installation #1874
Conversation
…found after installation
| const parts = []; | ||
| parts.push(__dirname); | ||
| parts.push('..'); | ||
| parts.push(`sentry-cli${process.platform === 'win32' ? '.exe' : ''}`); | ||
| return path.resolve(...parts); |
There was a problem hiding this comment.
| const parts = []; | |
| parts.push(__dirname); | |
| parts.push('..'); | |
| parts.push(`sentry-cli${process.platform === 'win32' ? '.exe' : ''}`); | |
| return path.resolve(...parts); | |
| return path.resolve(__dirname, '..', `sentry-cli${process.platform === 'win32' ? '.exe' : ''}`); |
There was a problem hiding this comment.
As per the comment above this function is convoluted like this so that static code analyzers (like Vercel's nft, and pretty much only Vercel's nft) cannot detect the binary as a "dependency" (dependency here meaning a thing that needs to be included in an AWS lambda for the application to work), because generally speaking Sentry CLI is not a runtime dependency but one that is only required during builds.
The binary is very large (~15mb) and will sometimes cause customer's lambdas to be over the limit (50mb) if we include it.
| * | ||
| * @returns {string} The path to the sentry-cli binary | ||
| */ | ||
| function getFallbackBinaryPath() { |
There was a problem hiding this comment.
Does this need to be a function? Can we convert this to a variable?
There was a problem hiding this comment.
See other comment. This function is just this way so that Vercel's nft cannot resolve the binaries location.
| @@ -222,7 +250,7 @@ function prepareCommand(command, schema, options) { | |||
| * @returns {string} | |||
| */ | |||
| function getPath() { | |||
| return binaryPath; | |||
| return mockedBinaryPath !== undefined ? mockedBinaryPath : getBinaryPath(); | |||
There was a problem hiding this comment.
| return mockedBinaryPath !== undefined ? mockedBinaryPath : getBinaryPath(); | |
| return mockedBinaryPath || getBinaryPath(); |
There was a problem hiding this comment.
I am intentional with the undefined check here because of empty strings.
| @@ -39,6 +39,7 @@ | |||
| "@sentry/cli-win32-x64": "2.23.1" | |||
| }, | |||
| "scripts": { | |||
| "postinstall": "node ./scripts/install.js", | |||
There was a problem hiding this comment.
Since we have usr/bin/env node in the first line of the script, we don't need node here:
| "postinstall": "node ./scripts/install.js", | |
| "postinstall": "./scripts/install.js", |
There was a problem hiding this comment.
I am not sure if the shebang will work on windows. Both SWC and esbuild have their postinstall scripts defined like this and it doesn't seem it's like that by accident so I will keep it.
There was a problem hiding this comment.
Actually, see #1151 where we tried this and ended up breaking people.
There was a problem hiding this comment.
@lforst It break in Amplify environment. Please advise the solution
npm ERR! path /root/.nvm/versions/node/v16.20.2/lib/node_modules/@sentry/cli
npm ERR! command failed
npm ERR! command sh -c -- node ./scripts/install.js
npm ERR! sh: node: command not found
| } catch (e) { | ||
| // Optional dependencies likely didn't get installed - proceed with fallback downloading manually | ||
| logger.log( | ||
| `Sentry CLI binary installation via optional dependencies was unsuccessful. Downloading manually instead.` |
There was a problem hiding this comment.
Should we at least log the error here?
There was a problem hiding this comment.
The error would just be Node's default "Cannot resolve module @sentry/cli-whatever-architecture" which is not helpful at all here - so likely not. During runtime (which is the actually important part because we cannot control what happens between installation and runtime - people do random shit like moving the platform specific binary from Windows to Debian in Docker) we will print a more specific error message with better instructions.
| process.exit(0); | ||
| }) | ||
| .catch((e) => { | ||
| console.error(e); |
There was a problem hiding this comment.
Should this be logger.error? I think we also need eslint-disable-next-line no-console in here.
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Fixes #1849 to some degree.
With this PR we re-add a post-install script that will manually download the Sentry CLI binary from our CDN if we cannot find the npm binary distributions that would be installed via optional dependencies.
This will improve the reliability for situations where optional dependencies are not installed (like Vercel apparently).
We still require either postinstall scripts to be enabled, OR optional dependencies to be installed.