Check if binaries exists before attempting to load

vidhu commented 1 year ago

Is there an existing issue for this?

[X] I have checked for existing issues https://github.com/getsentry/profiling-node/issues
[X] I have reviewed the documentation https://docs.sentry.io/
[X] I am using the latest SDK release https://github.com/getsentry/profiling-node/releases

How do you use Sentry?

Sentry Saas (sentry.io)

SDK Version

0.3.0

Link to Sentry event

No response

What environment is your node script running in?

node v16.17.0
yarn pnp
M1 Mac

How is your code deployed and bundled?

Neither. Issue is on post install

Steps to Reproduce

Init repo with yarn pnp
yarn add @sentry/profiling-node

Expected Result

Binary for...-darwin-arm64 is missing so I expect it to be compiled during the postinstall step

Actual Result

Binary not build in yarn pnp

In the post install step, we try and require(target)

Since CI doesn't build binaries for M1 Mac, the require fails and enters the catch { } block.

In the catch block, we check the error messages and if it is a known error message, we attempt to compile the binaries.

This works fine in a typical npm / yarn set up since we end up matching on the Cannot find module error case.

In yarn 2 pnp, the error message is different

Qualified path resolution failed: we looked for the following paths, but none could be accessed. and so we end up not compiling the binaries.

Instead of enumerating the error messages for all the various package managers, I feel it would be better to first assert that the binary exists in the first place or not. If it doesn't, then compile, else try loading and catch the errors

Error Log

On postinstall failure, this is the error log

# This file contains the result of Yarn building a package (@sentry/profiling-node@npm:0.3.0)
# Script name: postinstall

@sentry/profiling-node: Precompiled binary found, attempting to load...
<local-path>/.pnp.cjs:75634
      Error.captureStackTrace(firstError);
            ^

Error: Qualified path resolution failed: we looked for the following paths, but none could be accessed.

Source path: <local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/binaries/sentry_cpu_profiler-v93-darwin-arm64.node
Not found: <local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/binaries/sentry_cpu_profiler-v93-darwin-arm64.node
Not found: <local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/binaries/sentry_cpu_profiler-v93-darwin-arm64.node.js
Not found: <local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/binaries/sentry_cpu_profiler-v93-darwin-arm64.node.json
Not found: <local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/binaries/sentry_cpu_profiler-v93-darwin-arm64.node.node

Require stack:
- <local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/scripts/check-build.js
    at Function.require$$0.Module._resolveFilename (<local-path>/.pnp.cjs:75634:13)
    at Function.require$$0.Module._load (<local-path>/.pnp.cjs:75484:42)
    at Module.require (node:internal/modules/cjs/loader:1028:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (<local-path>/.yarn/unplugged/@sentry-profiling-node-npm-0.3.0-25372e22a8/node_modules/@sentry/profiling-node/scripts/check-build.js:24:3)
    at Module._compile (node:internal/modules/cjs/loader:1126:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1180:10)
    at Object.require$$0.Module._extensions..js (<local-path>/.pnp.cjs:75678:33)
    at Module.load (node:internal/modules/cjs/loader:1004:32)
    at Function.require$$0.Module._load (<local-path>/.pnp.cjs:75497:22)

vidhu commented 1 year ago

I've put up a PR for a potential fix https://github.com/getsentry/profiling-node/pull/122

Let me know if this is an acceptable approach!

JonasBa commented 1 year ago

@vidhu thanks for the detailed writeup and PR, definitely setting a great example of how to contribute 🏆 I left a tiny comment on your PR and we can be good to merge this.

JonasBa commented 1 year ago

@vidhu closing this issue as we merged the fix - we'll prepare a release sometime next week, thanks for your contribution!

getsentry / profiling-node