fix(profiling-node): Add binary path to support @electron/rebuild
#15112
Conversation
| @@ -54,6 +54,13 @@ export function importCppBindingsModule(): PrivateV8CpuProfilerBindings { | |||
| return createdRequire(`${binaryPath}.node`); | |||
| } | |||
|
|
|||
| // @electron/rebuild generates the binary in the generic node-gyp directory | |||
| try { | |||
| return createdRequire('../../build/Release/sentry_cpu_profiler.node'); | |||
There was a problem hiding this comment.
Does it actually place it here? I though it moves it into pkg/bin/$platform-$abi/smth.node?
There was a problem hiding this comment.
It actually outputs binaries in both locations!
On macOS I get a binaries at:
./bin/darwin-arm64-132/profiling-node.node./build/Release/sentry_cpu_profiler.node
The binaries are identical and work in Electron and the build/Release path doesn't vary by platform/arch so I chose that one 🤷♂️
The @electron/rebuild tests at least point to similar paths:
https://github.com/electron/rebuild/blob/8e8e6f94f8bac41044cf30b7898bdcaa04df0a2f/test/rebuild.ts#L150
There was a problem hiding this comment.
Ahh, that is good to know! I dont have a clean cmd so I usually always have some version of the binary that is built via postinstall or something in the build/release repo.
There was a problem hiding this comment.
I think this condition that you added here will mean that we always prefer this freshly built binary, which may have been built outside of just electron/rebuild, like for example someone running node-gyp directly. In any case, I think this is fine.
There was a problem hiding this comment.
Ah yes. Maybe we do a check for Electron first?
Checking for process.versions.electron is probably enough...
There was a problem hiding this comment.
I think we should, yes.
We should check to ensure that we default ship this build/release binding or that one is always generated as part of postintall, as it will otherwise break setups that bundle .node files as the path is statically analyzable, but the file wont actually exist which will result in an error
There was a problem hiding this comment.
or that one is always generated as part of postintall
A binary isn't always generated
otherwise break setups that bundle .node files as the path is statically analyzable, but the file wont actually exist
Mmmm... not sure what to do about that. Can we rely on the install script running? I guess not always!
Maybe we could already have a zero byte file at ./build/Release/sentry_cpu_profiler.node so that bundlers don't complain about it missing but it gets overwritten.
Then if you're running in Electron and @electron/rebuild hasn't run, the loading with throw and we catch it and explain in the debug message that you need to run @electron/rebuild?
|
@JonasBa are there any tests for the bundler issues/warnings? If not I might add those now! |
|
We need to backport this as well right? |
Possibly, depends on how long it takes to get it ready to merge 🤣 I'd want to add some bundler tests to ensure this doesn't regress any bundler issues. |
|
I'm not convinced the existing bundler tests actually cover all potential bundling issues. esbuild does not support resolving |
To work with Electron, native modules need to be rebuilt with Electrons modified Node.js headers. This can be achieved using
@electron/rebuildbut this results in the binary being outputted into the standard node-gyp output directory.This PR ensures that
@electron/rebuildoutput path is attempted first.