Linux Build Compatibility #25

apcragg · 2018-03-07T01:48:48Z

I changed the napi build script to work on Linux platforms in addition to MacOS. I am using the built-in Node.js os 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 with memcpy not working on Ubuntu 16.04 LTS.

This address comments in #15

daviwil

Works for me on Fedora Linux 27!

daviwil · 2018-03-07T02:35:53Z

Getting this error on macOS:

--- stderr
thread 'main' panicked at 'function not loaded: clang_Type_getNumTemplateArguments', /Users/daviwil/.cargo/registry/src/github.com-1ecc6299db9ec823/clang-sys-0.21.1/src/lib.rs:1456:1
note: Run with `RUST_BACKTRACE=1` for a backtrace.

child_process.js:644
    throw err;
    ^

Error: Command failed: cargo rustc  -Clink-args="-undefined -dynamic_lookup -export_dynamic"
    at checkExecSyncError (child_process.js:601:13)
    at Object.execSync (child_process.js:641:13)
    at Object.<anonymous> (/Users/daviwil/Projects/Work/xray/napi/scripts/napi.js:48:8)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! xray@0.0.1 build: `napi build`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the xray@0.0.1 build script.

I'm on macOS 10.12.6 so I dunno if this is a clang version issue, but I'll keep poking at it.

apcragg · 2018-03-07T02:40:44Z

clang --version
clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
Target: x86_64-pc-linux-gnu

This is the Clang version I am working with. As a quick though, try changing -dynamic_lookup to just dynamic_lookup which is what it was prior to my PR. I assumed the missing "-" was a typo as adding it did not cause issues for me but the "num template arguments" error makes me suspicious.

daviwil · 2018-03-07T02:44:01Z

Nah, didn't make a difference, I get the same error.

apcragg · 2018-03-07T02:47:13Z

Try changing -Clink-args to -- -Clink-args

daviwil · 2018-03-07T02:49:08Z

No change:

Error: Command failed: cargo rustc --release -- -Clink-args="-undefined -dynamic_lookup -export_dynamic"

daviwil · 2018-03-07T03:08:37Z

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 clang --version output:

Apple LLVM version 6.0 (clang-600.0.54) (based on LLVM 3.5svn)
Target: x86_64-apple-darwin16.7.0
Thread model: posix

steelbrain · 2018-03-07T04:15:14Z

napi/scripts/napi.js

+	case 'darwin':
+		libExt = '.dylib'
+		break;
+	case 'linux':


Maybe add linux as last case and let it fall-through because value is same for linux and default?

apcragg · 2018-03-07T14:44:13Z

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.

nathansobo · 2018-03-07T15:19:59Z

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.

nathansobo · 2018-03-07T15:21:05Z

napi/scripts/napi.js

 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'})


For me, the = sign in -undefined=dynamic_lookup is causing an error. It only works when we go back to replacing = with a space.

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.

Does it work for you with -undefined,dynamic_lookup as that also works with Linux.

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.

nathansobo · 2018-03-07T15:24:59Z

@daviwil Could you try to tweak the script to see the stderr output associated with the cargo invocation failure?

nathansobo

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

daviwil · 2018-03-07T15:52:06Z

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:

Apple LLVM version 9.0.0 (clang-900.0.39.2)
Target: x86_64-apple-darwin17.4.0
Thread model: posix

Building against master works fine now but building against this PR fails due to the argument changes to clang, as Nathan noted.

apcragg · 2018-03-07T16:26:30Z

@daviwil Could you take a look at my earlier comment and try replacing the = with ,?

apcragg · 2018-03-07T19:07:52Z

I cleaned up the pull request to meet the requested changes. I still need confirmation that these changes work on MacOS.

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

…ompatibility

daviwil · 2018-03-07T21:04:36Z

Sorry for the delay, just tried your suggestion and it didn't work, but this does:

cp.execSync(`cargo rustc ${releaseFlag} -- -Clink-args=\"-undefined dynamic_lookup -export_dynamic\"`, {stdio: 'inherit'})

However, when I use these arguments with clang 5.0.1 on Fedora, they don't work. Using -undefined=dynamic_lookup -export_dynamic is the only thing that works. Looks like we need to massage the args per platform?

apcragg · 2018-03-07T21:31:21Z

I added per-platform args. It should maintain the previous working behavior on MacOS and allow building on Linux systems

daviwil · 2018-03-07T21:38:06Z

Works perfectly for me now on both Linux and macOS!

nathansobo · 2018-03-07T23:28:25Z

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! ⚡️

daviwil approved these changes Mar 7, 2018

View reviewed changes

daviwil mentioned this pull request Mar 7, 2018

Contributing #15

Closed

steelbrain reviewed Mar 7, 2018

View reviewed changes

Ben3eeE mentioned this pull request Mar 7, 2018

Incomplete instructions and dependencies for build #27

Closed

nathansobo reviewed Mar 7, 2018

View reviewed changes

nathansobo suggested changes Mar 7, 2018

View reviewed changes

apcragg force-pushed the master branch from 80b1e0f to 6e1d960 Compare March 7, 2018 18:59

apcragg force-pushed the master branch from 6e1d960 to 65900a5 Compare March 7, 2018 19:10

Changed build script to work without {,} expansion to improve Linux c…

707b954

…ompatibility

apcragg force-pushed the master branch from 65900a5 to 707b954 Compare March 7, 2018 19:11

Massaging rustc parameters for platform specific operation

0155c06

nathansobo approved these changes Mar 7, 2018

View reviewed changes

nathansobo merged commit 8c67a8e into atom-archive:master Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux Build Compatibility #25

Linux Build Compatibility #25

apcragg commented Mar 7, 2018 •

edited

daviwil left a comment

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018 •

edited

daviwil commented Mar 7, 2018

steelbrain Mar 7, 2018

apcragg commented Mar 7, 2018 •

edited

nathansobo commented Mar 7, 2018

nathansobo Mar 7, 2018

apcragg Mar 7, 2018

apcragg Mar 7, 2018

apcragg Mar 7, 2018 •

edited

nathansobo commented Mar 7, 2018

nathansobo left a comment

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018

nathansobo commented Mar 7, 2018

Linux Build Compatibility #25

Linux Build Compatibility #25

Conversation

apcragg commented Mar 7, 2018 • edited

daviwil left a comment

Choose a reason for hiding this comment

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018 • edited

daviwil commented Mar 7, 2018

steelbrain Mar 7, 2018

Choose a reason for hiding this comment

apcragg commented Mar 7, 2018 • edited

nathansobo commented Mar 7, 2018

nathansobo Mar 7, 2018

Choose a reason for hiding this comment

apcragg Mar 7, 2018

Choose a reason for hiding this comment

apcragg Mar 7, 2018

Choose a reason for hiding this comment

apcragg Mar 7, 2018 • edited

Choose a reason for hiding this comment

nathansobo commented Mar 7, 2018

nathansobo left a comment

Choose a reason for hiding this comment

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018

apcragg commented Mar 7, 2018

daviwil commented Mar 7, 2018

nathansobo commented Mar 7, 2018

apcragg commented Mar 7, 2018 •

edited

daviwil commented Mar 7, 2018 •

edited

apcragg commented Mar 7, 2018 •

edited

apcragg Mar 7, 2018 •

edited