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.

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

Build binaries for Alpine #16

Merged
merged 1 commit into from
Jun 9, 2024
Merged

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Jan 28, 2021

No description provided.

@paulrutter
Copy link
Contributor

Thanks for the pr, will check next week!

@orgads
Copy link
Contributor Author

orgads commented Feb 7, 2021

Thank you. This is incomplete, because there are no alpine binaries for gcstats. I'm looking into that too, but it might take some time. And the original gcstats is not maintained, so we'll need to use an alternative fork, as proposed by @adnanraic. Will you consider that?

@paulrutter
Copy link
Contributor

paulrutter commented Feb 8, 2021

I'll think about that. Another way to go is to drop the need for gc-stats altogether, as it's only needed for
OOMImplementation: GC_MONITORING. We could release a new major version that only supports NATIVE_HOOK, making gc-stats no longer relevant.

@orgads
Copy link
Contributor Author

orgads commented Feb 8, 2021

Or you can change it to an optionalDependency (with proper protection on require), then I'll be able to install oom-heapdump without build tools, as long as I don't need gc monitoring.

@paulrutter
Copy link
Contributor

Yes, that's possible as well. But it might be unexpected that GC_MONITORING won't work then, but that can be "solved" by logging a warning.

@paulrutter
Copy link
Contributor

Did you run this on your repo? I see no github actions there.

@orgads
Copy link
Contributor Author

orgads commented Jun 9, 2024

I did this 3 years ago, I no longer remember. :)

Just stumbled upon the repo on my disk and remembered.

@paulrutter
Copy link
Contributor

Is this pr still relevant? E.g. Are prebuilt binaries still requested for alpine?

@paulrutter paulrutter merged commit 5d5c6b4 into blueconic:master Jun 9, 2024
@orgads orgads deleted the build-alpine branch June 9, 2024 19:36
@orgads
Copy link
Contributor Author

orgads commented Jun 9, 2024

Looks like the existing binaries are linked with glibc, so it should be needed, yes.

@paulrutter
Copy link
Contributor

I will take a look, thanks

paulrutter added a commit that referenced this pull request Jun 10, 2024
- Add prebuilt binaries for Alpine, #16
@paulrutter
Copy link
Contributor

@orgads it has been released to npm, can you check if this works for you?

@orgads
Copy link
Contributor Author

orgads commented Jun 10, 2024

hmm... Actually it looks like the original build works on alpine as well. I wonder how it works... 🤔

I guess you can revert the alpine parts of this change then. Sorry for the noise.

@paulrutter
Copy link
Contributor

As long as you have the build tools in place, it will compile it from source indeed.
Still, having the prebuilt binaries is still good to have, i'll leave it in for now.

@orgads
Copy link
Contributor Author

orgads commented Jun 10, 2024

I tested with 3.2.3 without installed build tools. I verified that the binary was not linked with musl, and that it was not built locally (the modification timestamp of the file was from a month ago).

It doesn't harm though, so it's your choice :)

@paulrutter
Copy link
Contributor

Strange, how is it able to call into native code then? Or do you have oom monitoring disabled? Then it would indeed work without compilation.

@orgads
Copy link
Contributor Author

orgads commented Jun 10, 2024

No, it does use the native bindings.

Here:

docker run -w /app --rm -it node:18.19.0-alpine3.19 ash                                                                                              ✔  1h 8m 53s  orgads@siplenovo  20:27:58
/app # npm init -y
Wrote to /app/package.json:
[...]
/app # npm i node-oom-heapdump@3.2.3
[...]
/app # ls -l node_modules/node-oom-heapdump/build/Release/
total 28
-rwxr-xr-x    2 1001     127          23608 May 16 07:42 node_oom_heapdump_native.node
drwxr-xr-x    3 root     root          4096 Jun 10 17:28 obj.target
/app # strings node_modules/node-oom-heapdump/build/Release/node_oom_heapdump_native.node | grep musl
/app # strings node_modules/node-oom-heapdump/build/Release/node_oom_heapdump_native.node | grep libc
libc.so.6
/app # cd node_modules/node-oom-heapdump/
/app/node_modules/node-oom-heapdump # ldd build/Release/node_oom_heapdump_native.node
        /lib/ld-musl-x86_64.so.1 (0x7d307af48000)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x7d307aca9000)
        libc.so.6 => /lib/ld-musl-x86_64.so.1 (0x7d307af48000) <--- This line is interesting :)
        libgcc_s.so.1 => /usr/lib/libgcc_s.so.1 (0x7d307ac85000)
Error relocating build/Release/node_oom_heapdump_native.node: _ZN2v86String11NewFromUtf8EPNS_7IsolateEPKcNS_13NewStringTypeEi: symbol not found
Error relocating build/Release/node_oom_heapdump_native.node: _ZN2v812api_internal12ToLocalEmptyEv: symbol not found
[...]
/app/node_modules/node-oom-heapdump # node
Welcome to Node.js v18.19.0.
Type ".help" for more information.
> require('bindings')('node_oom_heapdump_native.node')
{
  call: [Function: call],
  path: '/app/node_modules/node-oom-heapdump/build/Release/node_oom_heapdump_native.node'
}

This is what ChatGPT had to say about it: https://chatgpt.com/share/40b7f4f3-7bbf-406d-aef6-7375063f7107 :)

The bottom line is that it might be ok for simple API calls, but it is recommended to build separately for musl.

@paulrutter
Copy link
Contributor

Thanks for the details, didn’t know how that worked n alpine. Let's keep it in for now :)

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.

None yet

2 participants