-
Notifications
You must be signed in to change notification settings - Fork 83
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(NODE-5143): linking with mongocrypt in node bindings #604
base: master
Are you sure you want to change the base?
fix(NODE-5143): linking with mongocrypt in node bindings #604
Conversation
198d597
to
f4be459
Compare
Thank you @YakoYakoYokuYoku for helping us here! The team is going to investigate and reply back with more info after the investigation is completed. We are tracking the issue: https://jira.mongodb.org/browse/NODE-5143 |
Hi @YakoYakoYokuYoku - is there a reason that you are unable to use the static libraries in the release build? One reason we currently don't allow dynamic linking to the system installed libmongocrypt is that due to our rapid development we have a lot of API changes happening which most common libraries do not have. This creates a high probability risk that the system installed libmongocrypt is not compatible with our Node bindings and will error. By not allowing it we can guarantee that it will always work. |
This happened while I trying to install the dependencies of libmongocrypt/bindings/node/binding.gyp Lines 40 to 47 in 677b5d8
Then I've noticed that it uses the So, when you want to build from source
This PR makes sure that the package prebuilt in the Evergreen CI uses |
Thanks @YakoYakoYokuYoku for the information. We've discussed this and think what we are ideally looking for is not to dynamically link to the system libmongocrypt, but we would like --build-from-source without any other options to work, so that the correct libmongocrypt is built. Is that something you could modify this PR to do instead? |
If I say so myself, I have an idea. If |
@YakoYakoYokuYoku we were actually talking about this as well, as using cmake-js would probably simplify this instead of using node-gyp. So that sounds like a great idea. :) |
2d6f27a
to
331c98f
Compare
|
@@ -0,0 +1,34 @@ | |||
cmake_minimum_required (VERSION 3.12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YakoYakoYokuYoku Starting to have a look at this. Are there specific CMake features that are being used here that require this version? Our Evergreen build images currently have Cmake 2.8.12.2 installed so they currently have errors on the version check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Root CMakeLists.txt
uses this version, so that's the reason why I've used this one.
Line 1 in 2f48f35
cmake_minimum_required (VERSION 3.12) |
Also isn't 2.8.12.2
a bit too old?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's old, but that's what our some of our CI images have. I'll need to open an internal ticket to get those updated - thanks so far for the work.
d17dce7
to
cc83e74
Compare
cc83e74
to
9aebd76
Compare
9aebd76
to
7e13ee3
Compare
I've now removed the option to use system |
This PR fixes an issue when installing
mongodb-client-encryption
withnpm install --build-from-source
.Now with this PR when the
shared_libmongocrypt
npm config is set to true it will link against a system suppliedlibmongocrypt
, otherwise it uses the static libraries from the release build.