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

Making native builds bulletproof #152

Closed
egasimus opened this issue Apr 3, 2020 · 6 comments
Closed

Making native builds bulletproof #152

egasimus opened this issue Apr 3, 2020 · 6 comments

Comments

@egasimus
Copy link

egasimus commented Apr 3, 2020

Yep got kicked from Discord because I'm on a VPN. Figures. Hope you at least received my first message. Anyway, I wanted to ask about how this line is supposed to work:

load_napi(std::ptr::null());

cargo test segfaults on it and npm run example panics with panicked at 'load lib "libglfw.so.3"' even though I patched node-glfw-dylib to provide it with the path to libglfw.so.3 on my system (which is admittedly a little unorthodox, e.g. my distro works without a ldconfig cache)

I haven't done any macros or interop in Rust yet so I don't really know how to start debugging this but it does seem like a fun challenge. Could you give me a quick rundown, or better yet point me to some documentation on how this part is supposed to work - and maybe I can help make it more portable?

@egasimus egasimus mentioned this issue Apr 3, 2020
@cztomsik
Copy link
Owner

cztomsik commented Apr 3, 2020

What it does is that it loads (using dlopen) symbols/functions from current binary (what node executable provides to others) - I'm loading napi functions this way. You can probably comment that line for cargo test but I think it will not pass anyway, tests are not something I have time for now, sorry.

I think the problem is finding glfw, I don't know how exactly it works in your distro, so maybe just try to hard-code the path here:

load_glfw(c_str!(dylib_file("glfw", "3")));
just to check it's the problem.

And you can also npm run build -- --features=silly and it will log some extra info (in expense of performance). Also, npm run build is faster than npm run build -- --release.

Debugging js part is simple - just run node with --inspect in your IDE and it should work. About rust part, I'm not sure, lldb should work I think but I'm usually fine with compile-time errors, debug prints and RUST_BACKTRACE=1.

BTW: Don't get too much attached to the rust code, I'd like to do few design changes soon (and there are some changes already in the prealpha branch)

BTW2: If you know how to fix glfw-dylib, submit PR or at least issue pls.

@egasimus
Copy link
Author

egasimus commented Apr 3, 2020

from current binary

Bingo. That was what I was missing. Now I can try to figure out how it fits together.

If you know how to fix glfw-dylib, submit PR or at least issue pls.

I just added an option to specify the location via env var. I'm not happy with my solution since it expects th var to be present at both install time and run time - but it's something. I'll put in the PR soon - maybe ponder it some more first

@cztomsik
Copy link
Owner

cztomsik commented Apr 3, 2020

I just added an option to specify the location via env var.

node-glfw-dylib is meant to be put in optionalDependencies but there's no such thing as optionalDevDependencies so that's why it failed the build :-/

I don't know about any good solution but it's probably the best to just log big bold warning and not fail the whole install. The idea was that graffiti depends on glfw and it's your business to have it (or you can use node-glfw-dylib which will try to download it for you and put it in the PATH)

The fail is here, feel free to fix it if you want.

In the future, I'd like to avoid glfw entirely but supporting so many platforms with various configurations is not a simple task and it would definitely cause many hard-to-replicate bugs so it's probably still worth the pain.

@egasimus egasimus closed this as completed Apr 4, 2020
@egasimus egasimus reopened this Apr 4, 2020
@egasimus
Copy link
Author

egasimus commented Apr 4, 2020

Oops, didn't mean to close this 😅 But it's running, and it's awesome!

I think there were two separate issues at play here, both stemming from my ignorance of how dlopen actually works, and both looking similar enough to actually confuse them with each other.

  1. Passing a null pointer to dlopen means "load from current binary". When running with cargo test (which you said is unsupported anyway), that ain't Node. Whoops.
  2. More importantly, dlopen("libglfw.so.3") looks for the library on the LD_LIBRARY_PATH. Not sure why it worked for you here with just chdir. This is probably what glfw-dylib should be helping with, somehow - as of now, its only feature is breaking the build if it finds itself in an unfamiliar environment :-)

As an aside, this is one benefit I get from running a wacky distro: after catching the low-level issues like this - which may seem arcane but are usually based on a simple conceptual model and a simple implementation, and hence, fast and actually engaging to work on (imagine if I had to rebuild a JAR or some shit to see if it worked! I'd just give up) - it's usually smooth sailing.

(Of course, the drawback from running a wacky distro is that I have to compose sentences like the above with multiple levels of nested clauses if I want to explain myself, which is very un-rapperlike.)

In the future, I'd like to avoid glfw entirely but supporting so many platforms with various configurations is not a simple task and it would definitely cause many hard-to-replicate bugs so it's probably still worth the pain.

I think it's not such a bad idea to delegate to a tried-and-true library like glfw. I wonder whether static linking could be beneficial here though. A single statically linked graffiti binary containing Node, Graffiti and platform deps sounds cool. I never got as far as packaging stuff when playing around with Electron but I assume electron-builder does something similar?

OTOH bypassing electron-builder entirely and distributing a GUI app as a text file starting with #!/usr/bin/env graffiti sounds rad as fuck! (To me anyway - I guess you can already tell I'm most sympathetic to a Linux-first approach, but this sort of thing seems like it could be a major differentiator.)

@cztomsik
Copy link
Owner

cztomsik commented Apr 4, 2020

  1. ... When running with cargo test (which you said is unsupported anyway), that ain't Node. Whoops.

Yeah, sorry for that. I will eventually hide nodejs interop behind feature flag and it will fix cargo test and also make it possible to use it as rust library but I want to finish other things first.

  1. ... Not sure why it worked for you here with just chdir.

At least on my machines (mac, raspbian, ubuntu, win vm), libs are searched in cwd first. I'm not that experienced in linux development but I think it'd rather be better if glfw-dylib just set GLFW_LIB env variable and graffiti would try to search there first. I'm not sure yet but patching LD_LIBRARY_PATH is tricky and I'd need to patch DYLD_LIBRARY_PATH on mac too and call SetDllDirectory win32 api call (that itself would need another native module). So I'd rather stay with hacky chdir() for now and tried to fix it.

I wonder whether static linking could be beneficial here though. A single statically linked graffiti binary containing Node, Graffiti and platform deps sounds cool.

It was already statically linked before - I was just not happy with that - it took longer to compile, I might need to recompile (and push prebuilt binaries) for supporting new osx versions (or security fixes) and I think it's just better to use system-provided (and patched) version if possible. TBH I like glfw but if I'd want to support mobile too, it would be a problem. It's likely it will be a mixture of glfw and something else in the end.

I never got as far as packaging stuff when playing around with Electron but I assume electron-builder does something similar?

electron-builder worked fine but it's some time I've tried lastly https://github.com/cztomsik/slack-app/blob/master/package.json#L33

OTOH bypassing electron-builder entirely and distributing a GUI app as a text file starting with #!/usr/bin/env graffiti sounds rad as fuck! (To me anyway - I guess you can already tell I'm most sympathetic to a Linux-first approach, but this sort of thing seems like it could be a major differentiator.)

I think you can do this with nexe - maybe first precompile with ncc. Or if you don't want to bundle npm, it should be as simple as #!/usr/bin/env npx graffiti - but graffiti would need to provide cli command (not done yet)

@cztomsik
Copy link
Owner

small update here :)

  • glfw is bundled/statically-compiled now
  • mobile/wasm will not be an issue after all
  • build should pass fine, you only need rust and c++

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

No branches or pull requests

2 participants