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.

Sign up for GitHub

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

fix(profiling-node): Add binary path to support @electron/rebuild #15112

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

fix(profiling-node): Add binary path to support @electron/rebuild #15112

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
326832f
fix(profiling-node): Add binary path to support `@electron/rebuild`
timfish Jan 21, 2025
7e674f8
Move before non-electron paths
timfish Jan 21, 2025
ef338be
Add Electron check and warning
timfish Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/profiling-node/src/cpu_profiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

Does it actually place it here? I though it moves it into pkg/bin/$platform-$abi/smth.node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes. Maybe we do a check for Electron first?

Checking for process.versions.electron is probably enough...

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@timfish timfish Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

} catch (_) {
// ignore
}

// We need the fallthrough so that in the end, we can fallback to the dynamic require.
// This is for cases where precompiled binaries were not provided, but may have been compiled from source.
if (platform === 'darwin') {
Expand Down
Loading