-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 cross compile error #2758
Fix cross compile error #2758
Conversation
/ @nodejs/build |
Thanks for the PR, I'll verify. |
Thank you! |
I'm not really sure of what implications this might have, but couldn't resist to try it. @walkerlee it's missing a closing I'm setting up a cross compiler for the RPis (still not complete), it works without this, but was screaming for a test run: https://ci.nodejs.org/job/joaocgreis-node-test-commit-----only-fanned/18/ |
@joaocgreis Thansks for your remind. I am sorry for my mistake. This is my script for cross compile nodejs. (use raspberry-pi toolchain) If configure with --shared-openssl --shared-zlib options and do not apply patch, it's cause link error. |
@joaocgreis btw I wouldn't mind trying your cross compile work on the armv8 machines I have here, they are right next to the Pi's and if they can do it quick enough it might be quicker than having to get the Pi's to download from an external location. Point me to a script when you have something that's close. |
Aren't those armv8 machines like, really powerful? Haha |
I've taken a better look at this, LGTM. I found 4 dependencies use this function: zlib, openssl, libuv and http_parser. This fixes zlib, that passes the library path in pkg_config. Openssl does not seem to have a .pc file, but works if the libs are in the same place as zlib (as happens with the script provided by @walkerlee, thanks for it, helped testing!). The .pc file of libuv seems ok for this, has the libraries in libdir. I could not find the .pc for http_parser. @jbergstroem do you think that openssl or http_parser could have a .pc somewhere that passes the library path in cflags? |
Sorry, fell out of radar. @joaocgreis openssl will likely have a @bnoordhuis and I have been working on this -- perhaps he has any additional input. LGTM from me -- tested with and without libpath. |
@@ -728,7 +728,7 @@ def configure_library(lib, output): | |||
# libpath needs to be provided ahead libraries | |||
if pkg_libpath: | |||
output['libraries'] += ( | |||
filter(None, map(str.strip, pkg_cflags.split('-L')))) | |||
map('-L{0}'.format, filter(None, map(str.strip, pkg_libpath.split('-L'))))) |
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.
@walkerlee isn't this code just removing '-L'
from each library and then adding it back at the end?
Would the appropriate course of action not be just to split pkg_libpath
on whitespace?
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.
Whitespace isn't safe -- /foo/my files/
. Output from pkg_config should avoid being parsed if possible.
Closing due to lack of activity or responses. |
Can't get pkg_libpath properly. It'll cause cross compile failed. (cannot find -lz -lssl -lcrypto, etc...)