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

node-gyp links to node version of hdr_histogram, not local version #1755

Closed
bmacnaughton opened this issue May 17, 2019 · 3 comments · Fixed by nodejs/node#27779
Closed

node-gyp links to node version of hdr_histogram, not local version #1755

bmacnaughton opened this issue May 17, 2019 · 3 comments · Fixed by nodejs/node#27779

Comments

@bmacnaughton
Copy link

bmacnaughton commented May 17, 2019

node version 11.10.0+
Linux 4.15.0-48-generic #51-Ubuntu SMP x86_64 x86_64 x86_64 GNU/Linux
gcc version 7.4.0 (Ubuntu 7.4.0-1ubuntu1~18.04)

My node addon uses the C++ node-addon-api to wrap a (primarily C) library supplied as a .so file. The library contains a number of dependencies including hdr_histogram. The problem I am having is that the library's hdr_* symbols are being resolved by the version built into node (.../node/deps/histogram/). Histograms were added in node 11.10.0 and their availability as global symbols causes my program to fail because the two implementations diverge in how the counts are allocated.

My first thought was that there must be a way to partially resolve external symbols by linking my modules together into a combined module. I've tried ld -r modules*.o -o combined.a, and gcc -shared modules*.o -o combined.so as well as various permutations on those, While i understand linking and loading I don't have in depth knowledge of the gnu tool chain nor node-gyp, so I may be missing something obvious.

If there is a way to tell node-gyp to search my libraries before using symbols provided by node that would be a nice quick fix.

Any ideas on how I can address this problem would be very much appreciated.

@bnoordhuis
Copy link
Member

This is related to nodejs/node#27664 because I'm fairly sure those symbols aren't supposed to be exported by the node binary (cc @jasnell)

Is it an option for you to compile your C library from source to a static library and link against that?

If not, can you check if env LD_LIBRARY_PATH=/path/to/dir/containing/your/lib node app.js works? If yes, adding -Wl,--rpath,... to your 'ldflags': [...] stanza should do the trick.

@bmacnaughton
Copy link
Author

i've tried both these approaches and have not had success.

we currently produce a static library as well (10x bigger than the shared version) but using it the node version was still loaded. it's possible that the library build system (which i only started looking at yesterday) is not being built in a way that causes the symbols to resolved because it seems like that ought to work. i'll continue digging into what's going on there.

the second approach works in a small test app where i explicitly load the hdr_histogram library as a dependency. but it doesn't work when i'm trying to load our library.so that contains the hdr_histogram library. one alternative would be to distribute the hdr_histogram library separately but as my code is not the only package using our library that will take further evaluation.

@bmacnaughton
Copy link
Author

ok, i know a lot more about ld, nm, readelf, ldd, and the like. i believe i can work around this; i think that the reason we're ending up with the problem is that our shared library does not do partial linking at times when it can.

i appreciate your suggestions - they started me down the right path.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue May 23, 2019
Fixes: nodejs/node-gyp#1755
Fixes: nodejs#27778
PR-URL: nodejs#27779
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue May 23, 2019
Fixes: nodejs/node-gyp#1755
Fixes: #27778
PR-URL: #27779
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
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 a pull request may close this issue.

2 participants