There was a TODO to for better library lookup logic, this is it (hope…

[yb66]

…fully).

• Use Pathnames because they're pathnames.
• Use the ENV vars instead of hardcoded paths.
• Switched the default paths around as /usr/local would probably be preferred.
• Still has the hardcoded paths as the default.

[yb66]

Before I forget, I've no idea what to do for Windows on line 600.

Regards,
iain

[larskanis]

Library lookup by using LD_LIBRARY_PATH and co. is redundant to what the dynamic loader of the operating system does. Why should we repeat the search through these directories? Is there a particular issue you're trying to fix?

[yb66]

Yes, in that it doesn't find libraries that aren't in the usual place but are set in the env vars by the user, because I installed exempi yesterday using pkgsrc which installs into /opt/pkg and FFI didn't find the library. With these changes it works.

[tduehr]

Rebate against master please.

[yb66]

@tduehr Okay, but because of this open pull request I'm not sure what to do about pushing it back up. Would you prefer/advise I use --force, --force-with-lease, or use a new branch?

Regards,
iain

[tduehr]

The difference is moot unless you have multiple people working in the same branch. When I’m working on a PR, I usually just use —force because I know I’m the only person working on that branch in my copy of the repo.

[yb66]

@tduehr Thanks, I wasn't sure. The specs and tests pass on my machine so we'll see what happens with the CI.

iain

[larskanis]

I still don't think that's the way to go. It's the responsibility of the dynamic loader of the operating system to resolve relative paths (by /etc/ld.conf, environment variables, etc.) not ffi's. I would rather argue to remove these hardcoded paths entirely, because they are simply wrong in a multiarch setup (and most dists are multiarch nowadays). I've seen a lot of issues by such hardcoded paths especially on Windows with MSYS2/MINGW. It often leads to linking to some library of a wrong arch.

If there's a particular loader issue with a library in /opt/pkg, it should be investigated why this happens before implementing new logic in a place where it doesn't belong to.

[yb66]

@larskanis Where's the call to the dynamic loader made?

[larskanis]

@yb66 dlopen is called here.

[yb66]

@larskanis Thanks, I'm happy give that a look when I get a moment. In the meantime there is some hardcoded stuff with logic that doesn't always work as it should and these changes fix that, so I would say the changes should be integrated until the problem with the dlopen code is fixed. The tests/specs should definitely be looked at too but as I'm not very familiar with the codebase I'd prefer someone else do it.

[pmahoney-raise]

I'd like to see something like this supported...

I'm on os x with ffi failing to load libsodium, even after setting LD_LIBRARY_PATH (worked on Linux) and DYLD_[FALLBACK_]LIBRARY_PATH. (seems same issue as #461)

It'd be great if there was an environment-variable-based way to configure how ffi does a lookup (rather than editing code or monkey-patching, etc.). Even if was something explicit and specific to ruby's ffi like RUBY_FFI_PATH_FOR_mylibrary=/the/absolute/path/lib/mylibrary.dylib

[tduehr]

Adding an ffi specific ENV variable is a good idea. It'd be useful to override existing paths too. RUBY_FFI_LIBRARY_PATH

Also, yes, that's OSX clearing the normal library path variables. Use a ruby in /usr/local/ instead and it'll work.

[pmahoney-raise]

My ruby is in /nix/store/... :(

[yb66]

@tduehr "Use a ruby in /usr/local/ instead and it'll work." Why is the suggestion to follow a not-always-helpful convention when there are better ways to handle this?

[yb66]

@pmahoney-raise You use Nix? Interesting. I keep mine in /opt/rubies because I'm using chruby.

It seems that we're outliers and hardcoding paths is the thing to do in 2018 (and beyond).

[owenthereal]

Any update on this?

[tduehr]

@larskanis this is a fallback when the dynamic loader can't find the library. Though, we should probably check dyld for all possible library names before doing a search like this.

[larskanis]

I think the issue we try to solve here and in the very first commit is purely MacOS specific. We already had a security incident on exactly this part of code, so that I think we should shrink the scope of this post-dyld processing to where it is really necessary.

The dynamic loader on both Linux and Windows respects the relevant environment variables properly, so that we should make the code MacOS-only. I don't have a Mac, so I can't reproduce the issues, but we should try to remove the absolute paths.

[tduehr]

Good point, that would make it macOS only, and only for Apple unsupported configurations. This should probably change to only a user specified env variable.