erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
887 stars 190 forks source link

Feature/weak link optional symbols #247

Closed skrap closed 5 years ago

skrap commented 5 years ago

Hello!

This change moves rust-zmq away from compile-time interrogation and cementing of libzmq features, to runtime detection of features. This seems useful for two cases: 1) If the build host separate from execution environment, then the list of features available in the build host's libzmq could differ from the libzmq where the binary is actually run. This could result in features being inappropriately disabled (best failure) or a dynamic link error at execution (worst). 2) If cross-compiling, today's build.rs will fail to link, as it'll be pointed at the target libzmq, rather than the host libzmq. This change removes build.rs from the outer zmq crate entirely, and it cross-compiles cleanly.

The approach here was inspired by https://blog.8-p.info/en/2018/08/13/weak-linkage/, and taken from the mio code (also apache licensed). The copied dlsym.rs defines a dlsym!macro, which uses the libc dynamic linker to ask if particular symbols are present or not (once) and then exposes those symbols as Option<FunctionType>, which can be used at runtime to decide what to do.

This error handling approach generally mapped very easily into the code.

It's possible you may want the zmq_has feature removed as well, as it's not used now, but I left it in because I wasn't sure. Please let me know.

It's also possible that you may be able to remove the part of the sockopt macro which deals with the cfg!(ZMQ_HAS_X) cases, as they're gone now, but those macros are beyond my abilities to edit. Sorry!

Comments and changes definitely welcome!

skrap commented 5 years ago

Oh. Windows. Drat. I'll see about fixing that, but if there are other comments in the meantime, please feel free.

skrap commented 5 years ago

std does this for windows here: https://github.com/rust-lang/rust/blob/190feb65290d39d7ab6d44e994bd99188d339f16/src/libstd/sys/windows/dynamic_lib.rs

I should be able to copy that approach, if I can stand up a windows VM.

skrap commented 5 years ago

Ok, I am hitting some obstacles getting further with the windows issue. I only have a really old vs2012 license (that was the last time I did any windows development) and I am struggling to get libsodium and libzmq to build as you do in the appveyor batch script. I'll keep plugging away at it, but assistance from someone who has a working windows dev setup would be welcome.

skrap commented 5 years ago

Yeah, I think my windows env is broken somehow. Even running the v.0.9.0 appveyor script fails:

image

I think I will need assistance to identify the windows issues, but it's probably just that the windows tests need to add a dynamic check for curve and dssapi support somewhere.

skrap commented 5 years ago

@erickt I know you must be super busy. Would it be possible for you to let me know if you have objections to the general direction of this change? That way I can know whether to put effort towards fixing up the Windows code. Many thanks.

I also see that you've bumped the minimum ZMQ version to 4.1, which would at least make weak-linking of zmq_has unnecessary. If you're interested in the PR, I can make that change as well.

rotty commented 5 years ago

Hi! I've skimmed over your pull request, and it seems there has been quite a bit of work gone into, so thanks for that! I've not yet really come to a conclusion on whether or not using runtime linking is the way to go, but here are my thoughts so far:

rotty commented 5 years ago

Regarding making it easier to use an up-to-date libzmq, see #257, which, when implemented, should make that process relatively hassle-free.

skrap commented 5 years ago

@rotty : Thanks for checking it out! The work was not a big deal — if was a proof of concept which seemed useful enough to consider, but if you guys don't want it, I won't mind!

My concern with build-time feature probing is mainly around the cross-compiling case. Is this case, you are building on a host which can't link against the target binaries. More concretely, imagine you're building a rust executable to run on ARM, but your build machine is x86. The build script (as it stands today in master) would like to link and run against libzmq to check its features, but as it stands today, a build will use the build machine's x86 libzmq, rather than the target ARM machine's libzmq. So, the features it enables will only be due to chance, and won't necessarily match the environment where the final executable will actually be run.

I definitely understand the limitations around runtime linking, and I agree it's not ideal for many reasons. Perhaps I could propose a different direction altogether? One could enable or disable zmq features via cargo feature flags. I could see a few ways of going about this:

My feeling is that the first option is probably better, but I don't use any of the optional zmq features, so I'm definitely biased :)

rotty commented 5 years ago

I'm familiar with the basics of cross-compiling and the issue you presented, I'm just not yet sure how to best solve it. My feeling is that feature-probing for native builds is nice, as it mostly "just works". On the other hand, it does not provide a way for depending crates to express that they require specific libzmq features (besides depending on new-enough version of libzmq) -- they would just fail to build (which is still better than runtime errors). Maybe coupling the zmq crate to libzmq minimum versions is a possibility, but that also has downsides, as it makes it harder to use the zmq crate with distro-shipped libzmq installations, which is a usecase I care about.

For the cross-compile case, we might be able to auto-detect the cross-compilation attempt and disable feature-probing, and use some other mechanism.

Anyway, I need to do research, looking into other crates that have a similar problem and take a look at how they solve it.

rotty commented 5 years ago

I'll close this, as I think using runtime linking is not the right approach; regarding cross-compilation, I'll try to find a solution to disable feature-probing and thus avoid linking against the zmq-sys crate for the zmq crate's build script, probably by putting the feature-probing mechanism behind a cargo feature flag.