-
Notifications
You must be signed in to change notification settings - Fork 235
Conversation
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.
Works for me on Fedora Linux 27!
Getting this error on macOS:
I'm on macOS 10.12.6 so I dunno if this is a clang version issue, but I'll keep poking at it. |
This is the Clang version I am working with. As a quick though, try changing |
Nah, didn't make a difference, I get the same error. |
Try changing |
No change:
|
FWIW I get the same error without the changes you've made, so it's probably an issue with my clang version or the configuration of my machine. Here's my
|
napi/scripts/napi.js
Outdated
case 'darwin': | ||
libExt = '.dylib' | ||
break; | ||
case 'linux': |
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.
Maybe add linux
as last case and let it fall-through because value is same for linux and default?
I changed the default behavior to exit the build process and warn if the OS isn't recognized. I think this is more helpful than potentially using the wrong dynamic library type only to get cryptic "ELF header" errors later in the build process. I also fixed a hyphen typo I introduced in my previous commit. |
Hey @apcragg, could you omit the changes to the lock files? I need to ignore those for the subcomponents and figure out why it's changing on the electron app. If you let me push changes to your branch I could do it too. |
napi/scripts/napi.js
Outdated
switch (subcommand) { | ||
case 'build': | ||
const releaseFlag = argv.release ? '--release' : '' | ||
const targetDir = argv.release ? 'release' : 'debug' | ||
cp.execSync(`cargo rustc ${releaseFlag} -- -Clink-args=\"-undefined dynamic_lookup -export_dynamic\"`, {stdio: 'inherit'}) | ||
cp.execSync(`cp target/${targetDir}/{lib${moduleName}.dylib,${moduleName}.node}`, {stdio: 'inherit'}) | ||
cp.execSync(`cargo rustc ${releaseFlag} -- -Clink-args=\"-undefined=dynamic_lookup -export_dynamic\"`, {stdio: 'inherit'}) |
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.
For me, the =
sign in -undefined=dynamic_lookup
is causing an error. It only works when we go back to replacing =
with a space.
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.
That must be a mac/linux thing because I can only compile using Clang 3.9 on Ubuntu 16.04 LTS with the '=' sign. I will remove the package lock changes and add the '=' sign to the os case statement.
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.
Does it work for you with -undefined,dynamic_lookup
as that also works with Linux.
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.
The problem seems to be that on MacOS, linker -flag <arg>
is valid but on Linux it is being seen as linker -flag [linker-file-target]
. Since dynamic_lookup
isn't a valid file or directory for the clang linker, it throws an error. If `linker -flag,' (no space) works for MacOS then this seems like a cross-platform fix. If anyone has more insight on the discrepancy please let me know.
@daviwil Could you try to tweak the script to see the stderr output associated with the |
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.
We need to get the following issues ironed out before we can merge this:
- Confirm that we can build on Linux
- Continue to build correctly on macOS (the
=
in-undefined=dynamic_lookup
is currently breaking for me) - Omit changes to lock files
- Rebase into 1 or 2 commits to keep the history clean
Apparently I had removed Xcode from my machine a while back and clang wasn't getting updated anymore. Reinstalled Xcode and now my clang is up to date:
Building against master works fine now but building against this PR fails due to the argument changes to clang, as Nathan noted. |
@daviwil Could you take a look at my earlier comment and try replacing the |
I cleaned up the pull request to meet the requested changes. I still need confirmation that these changes work on MacOS.
|
Sorry for the delay, just tried your suggestion and it didn't work, but this does:
However, when I use these arguments with clang |
I added per-platform args. It should maintain the previous working behavior on MacOS and allow building on Linux systems |
Works perfectly for me now on both Linux and macOS! |
Thanks for figuring this out @apcragg and thanks @daviwil for test driving it. I'm going to apply some small style tweaks on master, but otherwise this looks great. Thanks! Now we just have to understand these mysterious build failures some people are getting on macOS and hopefully our build story will be golden. Thanks again! ⚡️ |
I changed the
napi
build script to work on Linux platforms in addition to MacOS. I am using the built-in Node.jsos
module to detect the user's system type in order to copy the correct dynamic library file. I also included<string.h>
as I was having issues withmemcpy
not working on Ubuntu 16.04 LTS.This address comments in #15