erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
899 stars 194 forks source link

Build zeromq from source via zeromq-src-rs #262

Closed jean-airoldie closed 5 years ago

jean-airoldie commented 5 years ago

Also fixed improperly formatted files.

This would fix #257, at least the building from source part. Also related to #241.

This uses zeromq-src for the source code and the tooling.

I do not know if this builds in windows, since it only tested on ubuntu xenial and fedora 29.

jean-airoldie commented 5 years ago

The windows build fails with LINK : fatal error LNK1181: cannot open input file 'stdc++.lib'. I have absolutely no clue what that means since I don't know anything about windows.

rotty commented 5 years ago

Nice, you beat me to it! I started yesterday working on a libzmq-src crate, including the Windows build logic figured out by @oblique and @zachlute in PR #241. I'll try to prepare a PR to your zeromq-src that adds MSVC support, but I'd work blind on that, as I also do not have a Windows installation at my disposal. Perhaps @oblique would be interested in preparing that PR instead?

jean-airoldie commented 5 years ago

I added the repo to appveyor. Feel free to add a appveyor.yml script in your PR. That should help if you do proceed blind.

jean-airoldie commented 5 years ago

Sure. I mostly wanted to see if the lib would properly compile and was too lazy to modify the CI script.

I agree with the feature flag. I'll make the changes, should take a few minutes.

And the rustfmt thing is because I setup I have a background script that formats code, I'll undo that.

jean-airoldie commented 5 years ago

I don't think a feature flag would work in this specific case since the user will most likely be using the zmq crate directly. This means he won't be able to specify a feature flag for zmq-sys. I'll add support for both the feature flag and a env variable. However this would mean that zeromq-src can't be an optional dependency like in openssl.

jean-airoldie commented 5 years ago

We should document the env variable ZMQ_SYS_VENDORED and ZMQ_SYS_STATIC behavior as well as configure the CI to test building both possible ways.

rotty commented 5 years ago

I don't think a feature flag would work in this specific case since the user will most likely be using the zmq crate directly. This means he won't be able to specify a feature flag for zmq-sys. I'll add support for both the feature flag and a env variable.

I think this is possible. IIUC, the vendored flag is something that only binary crates would actually use. I tried to find an example, and lo and behold, our beloved cargo does that:

cargo has a feature flag vendored-openssl, which gets passed on to openssl, which (as an implementation detail) passes the feature flag down to openssl-sys:

I think we could (or should) use the same approach, and then do not need the environment variable. The feature flag should be documented, however, and potential licensing implications made clear.

jean-airoldie commented 5 years ago

That's pretty sick. I'll try to figure out how it works.

edit: I got it its pretty simple.

jean-airoldie commented 5 years ago

One question remains. The vendored crate can be either built & linked statically or dynamically. We could add a static feature flag to zmq that would be reexported to zmq-sys (like we did for vendored).

The problem with that is that this static feature flag only takes effect when specified simultaneously with the vendored flag, which is a potential footgun.

We could panic the build with a message if static is not specified along with vendored but it feels like a sketchy idea.

We could also have the vendored-static and vendored flag. But once again there is not sane way to make features mutually exclusive.

We could also always build the vendored crate dynamically.

In any case, that's all the time I had for today.

jean-airoldie commented 5 years ago

I have an update regarding the zeromq licensing. Turns out that zeromq has a static linking exception. To quote from the zeromq website:

ZeroMQ is safe for use in close-source applications. The LGPL share-alike terms do not apply to applications built on top of ZeroMQ. You do not need a commercial license. The LGPL applies to ZeroMQ's own source code, not your applications. Many commercial applications use ZeroMQ.

rotty commented 5 years ago

@jean-airoldie: Nice find! I completely missed that. It means we do not specially need to document any licensing restrictions when building with vendored, as it appears there are none, IIUC :grin:.

jean-airoldie commented 5 years ago

Regarding my previous point, I think simply linking statically makes the most sense. Previously my rational was that dynamic linking should be allowed because of the LGPL license, but now I can't think of a compelling argument anymore.

jean-airoldie commented 5 years ago

I will close this for now. I will rework the zeromq-src API to only link statically, shouldn't take too long.

rotty commented 5 years ago

My point on code duplication and rolling security fixes on (e.g.) Linux distros still stands (see this comment):

Linux distributions tend to avoid source code duplication between packages, and also (generally) prefer dynamic linking, as it makes shipping security fixes a lot easier. This is at least the case for Debian (and probably derivatives).

jean-airoldie commented 5 years ago

But why would you use the vendored flag if you want to use your distros package? I'm talking specifically about removing the ability to build from source as a dynamic library.

jean-airoldie commented 5 years ago

I guess I could build both the static and shared lib and let rust decide which one to use. This would also avoid potential feature flag confusion.

rotty commented 5 years ago

Ah, ok, I was thinking you meant dropping dynamic linking support completely, which would indeed make life easier regarding feature detection and the likes.

Having vendored (as long as it stays optional and non-default) only support static linking is totally fine in my book, especially given the static linking exception. I wouldn't have insisted on dynamic linking support of vendored even if libzmq was vanilla LGPL, as it's an optional feature. Maybe someone feels inclined to add dynamic linking support for vendored, but it's in no way essential.