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

Fix: Remove libsodium dependency (for now). #554

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

Bartel-C8
Copy link
Contributor

Use TweetNacl, which doesn't need a (user having) libsodium installed.

As discussed here: #529 (comment) , libzmq doesn't handle statically linking libsodium.

I think the best, and most versatile option (keeping in mind all build platforms: Unix, Alpine, macOS x64, macOS ARM, Windows x64, Windows ARM, ...), as a next step, to build libsodium ourselves in the build.ts, before libzmq itself. Preferably with libsodium-cmake (https://github.com/robinlinden/libsodium-cmake) .

Use TweetNacl, which doesn't need a (user having) libsodium installed.
@Bartel-C8
Copy link
Contributor Author

@aminya : What do you think about it? The current released beta really has issues/problems with installing. So I would prefer having a new beta release with tweetnacl. So we can fix this decently but at ease...

Copy link
Member

@aminya aminya left a comment

Choose a reason for hiding this comment

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

A better solution is to include the dylib/so files in the package. I don't think removing it is a good idea.

@Bartel-C8
Copy link
Contributor Author

I do agree that it's not ideal to remove it. But how were the beta-6 and earlier shipped? Without libsodium I guess?

Also, dynamic linking in combination with .node files will be cumbersome for the many platforms I think: nodejs/node-gyp#89 (I am an electron user, so I have a big focus on that)

So a static build of libsodium will be the most easy/stable/versatile I guess...

But this PR was to quickly address the failing install of the current released beta. And even the case I had on macOS, where the shipped .node in my electron build has a hard dependency on libsodium installed on the client. Without being bundled currently...

So I had runtime crashes at clients, without noticing at first, as my own environment is the build environment l, which had libsodium installed...

@Bartel-C8
Copy link
Contributor Author

Bartel-C8 commented Jan 5, 2023

Btw, currently, the windows build is built without libsodium, as it is not installed on the CI. So it's not uniform anyway now. That is why I thought this PR is a good first step.

Another "problem" I see is that the whole build system should have a working fallback if libsodium wasn't installed on the build system: e.g. for rebuilds on installs where the prebuilts are incompatible. That is why I think the libsodium-cmake is a good idea, so it's always available correctly. E.g. for windows, installation of libsodium is not trivial, as you should know in advance which libsodium variant to link with (msvc/mingw).

I do completely agree, that for the official 6.0 release, this should be resolved. But like said, the current beta release is not really usable currently, both for macOS as for Linux... So the full correct/reproducible solution will take quite some effort imo.

@Bartel-C8
Copy link
Contributor Author

@aminya : As the upstream issue in libzmq was not accepted (initially hopefully) this will take time. And so the latest published build is not usable for all OSses. So I am in favour of merging this PR for a temporary working solution at least...

@mitchobrian
Copy link

Do we have a timescale for this?

@pshenmic
Copy link
Contributor

I'm having different issues on Node 20 with zeromq 6.0.0-beta.17 on the CI and in the Docker images:

 /opt/actions-runner/_work/_tool/node/20.9.0/arm64/bin/node: symbol lookup error: /opt/actions-runner/_work/platform/platform/.yarn/unplugged/zeromq-npm-6.0.0-beta.17-8f82236e6c/node_modules/zeromq/build/Release/zeromq.node: undefined symbol: sodium_init
Error: Error relocating /platform/.yarn/unplugged/zeromq-npm-6.0.0-beta.17-8f82236e6c/node_modules/zeromq/build/Release/zeromq.node: crypto_box_easy_afternm: symbol not found

We really need get this PR in

@aminya aminya merged commit 82b838d into zeromq:master Nov 15, 2023
@Bartel-C8
Copy link
Contributor Author

Thanks @aminya , in name of the zmq.js community ;-).

Should we mention that CURVE is disabled then in the README?
A new release would befit many then from now on! 🙏

@mitchobrian
Copy link

mitchobrian commented Nov 15, 2023 via email

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

4 participants