Skip to content
This repository was archived by the owner on Nov 9, 2017. It is now read-only.

Link static lib directly into .node, remove OSX hack.#6

Merged
dherman merged 1 commit intoneon-bindings:masterfrom
eddyb:patch-1
Dec 28, 2015
Merged

Link static lib directly into .node, remove OSX hack.#6
dherman merged 1 commit intoneon-bindings:masterfrom
eddyb:patch-1

Conversation

@eddyb
Copy link
Copy Markdown
Contributor

@eddyb eddyb commented Dec 24, 2015

Should allow building on all architectures supported by Node.js and Rust.
Tested that it builds and loads fine on Linux.

@diorahman
Copy link
Copy Markdown

Interesting, I got this from the compiled hello.node lib on linux 64:

> require('./build/Release/hello')
{ '\u0000\u0000\u0000\u0000\u0000': [Function] }
> for (var k in a) { a[k](); }
'hello node'

@eddyb
Copy link
Copy Markdown
Contributor Author

eddyb commented Dec 24, 2015

@diorahman Yes, that's fixed by dherman/neon#22.

@diorahman
Copy link
Copy Markdown

@eddyb thanks!

@eddyb eddyb changed the title Build shared lib directly, remove OSX hack. Link static lib directly into .node, remove OSX hack. Dec 24, 2015
@eddyb
Copy link
Copy Markdown
Contributor Author

eddyb commented Dec 24, 2015

Turns out shared libs are not worth creating because .node is already a shared lib and it's easier to link everything into it statically.

@No9
Copy link
Copy Markdown

No9 commented Dec 25, 2015

FYI when I follow the example on FreeBSD 11 CURRENT December node 4.2.3 i get

node -e 'require("./")'
/xxx/hello/lib/index.js:3
console.log(neon.hello());

TypeError: neon.hello is not a function
    at Object.<anonymous> (/xxx/hello/lib/index.js:3:18)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at [eval]:1:1
    at Object.exports.runInThisContext (vm.js:54:17)
    at Object.<anonymous> ([eval]-wrapper:6:22)

But when I follow the steps by @diorahman in #6 (comment) I get the same positive outcome with no error :)

BTW Loving this cool stuff <3

@eddyb
Copy link
Copy Markdown
Contributor Author

eddyb commented Dec 25, 2015

@No9 It seems that @dherman did not update the crates.io version of neon - so you're not getting the fix at dherman/neon#22.

If you replace the crates.io version dependency on neon with a github one (https://github.com/dherman/neon should work), and run cargo update followed by npm install, you should get a fully functional .node addon.

@No9
Copy link
Copy Markdown

No9 commented Dec 25, 2015

Thanks @eddyb for your help

After trying that update I seem to have walked into an unrelated issue

/xxx/neon-cli/lib/create.js:64
    { type: 'input', name: 'author',      message: "author",           default: guess.author   },
TypeError: Cannot read property 'author' of undefined
    at wizard (/usr/home/anton/builds/neon-cli/lib/create.js:64:86)
    at Object.<anonymous> (/usr/home/anton/builds/neon-cli/bin/cli.js:64:3)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:467:10)
    at startup (node.js:136:18)
    at node.js:963:3

FWIW I am running neon-cli linked not installed I'll try and come back to it at some stage and work out why

@diorahman
Copy link
Copy Markdown

Yeah, updating the deps, in the e.g. hello's Cargo.toml and the ../neon is dherman/neon seems working:

[dependencies]
neon = { version = "0.1", path = "../neon" }

@eddyb
Copy link
Copy Markdown
Contributor Author

eddyb commented Dec 25, 2015

@diorahman git = "https://github.com/dherman/neon" in Cargo.toml is what I meant (where you currently have path = "../neon").

@diorahman
Copy link
Copy Markdown

OK.

Got this one:

/home/diorahman/Experiments/misc/rs/neon/src/hello/build/Release/hello.node
module.js:460
  return process.dlopen(module, path._makeLong(filename));
                 ^

Error: ../target/release/libhello.so: cannot open shared object file: No such file or directory
    at Error (native)
    at Object.Module._extensions..node (module.js:460:18)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at Object.load (/home/diorahman/Experiments/misc/rs/neon/src/hello/node_modules/neon-bridge/lib/index.js:19:10)
    at Object.<anonymous> (/home/diorahman/Experiments/misc/rs/neon/src/hello/lib/index.js:1:97)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)

Do you have any ideas?

Previously, in order to check the error, I then copied the target directory to ... And it works. It seems there is a problem regarding to the process cwd.

@eddyb
Copy link
Copy Markdown
Contributor Author

eddyb commented Dec 25, 2015

@diorahman You're not using the latest version of this PR, maybe? I've switched from linking a dylib into the .node to linking a staticlib, which seems to work much better for me.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I guess we can use:

"libraries": ["<(PRODUCT_DIR)/../../<(static_lib)"],

@diorahman
Copy link
Copy Markdown

@eddyb thanks! Now everything works as expected!

@dherman
Copy link
Copy Markdown
Collaborator

dherman commented Dec 28, 2015

This is great. I'll follow up immediately with the one fix needed to make it work in OS X again.

dherman pushed a commit that referenced this pull request Dec 28, 2015
Link static lib directly into .node, remove OSX hack.
@dherman dherman merged commit e2b3e37 into neon-bindings:master Dec 28, 2015
@eddyb eddyb deleted the patch-1 branch December 28, 2015 22:07
@dherman dherman mentioned this pull request Dec 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants