Remove cmake

[Jasper-Bekkers]

Why?

Currently because of the way this crate is set up (calling cmake) it requires that any user of this crate has cmake on their system installed. We think this badly impacts developer experience when using zmq (or our own downstream crates). As an example of where this shows up (not just our Traverse Research crates), is for example in Google's evcxr setup instructions for all platforms.

What?

It's become pretty standard in the Rust ecosystem to avoid cmake and instead to mirror it's buildscripts directly in Rust, usually it's relatively straight forward (just move over some defines and point it to some cpp files).

This switches over to the cc crate so that people using this crate don't need cmake to be installed on their system, but instead can just use cargo build without any additional requirements. The setup for this PR mirrors for example the setup used in breakpad-sys by Embark Studios.

As a result of switching to cc directly - we can enable parallel builds, and speed up the build times for zmq quite a bit when using the vendored feature - potentially making the non-vendored use-case of this crate obsolete.

Platform support

• Windows
• Linux
• macOS

[jean-airoldie]

Hi, removing cmake dep is of course a good idea. From what i recall, I only used the cmake build because building zmq without it was god awfull. If you can make this to work with only cc as a dependency, I'll happily merge this.

However the are a couple things i would like to see before I can merge:

• Add a linux CI build (looks travis doesn't provide free OSS credit anymore, so anything else will do, for instance appveyor seems to work find).
• Provide the same functionality, unless you show that said functionality is useless or deprecated.
• If you add support for other platforms, please add CI builds for these.

[Jasper-Bekkers]

I'll switch the entire CI step to run on GitHub actions then if you don't mind, and work on getting more or less feature parity with what was there before.

How attached are you to breaking/non-breaking changes to this crate? Right now I've kept the API mostly the same but there's quite a few things that aren't being used upstream, or not needed anymore (for example the profile always seems to get set to the current building profile - however cc also does that so it'd be pretty easy to override though Cargo.toml instead).

[Jasper-Bekkers]

The CI run on the fork has turned green now https://github.com/Traverse-Research/zeromq-src-rs/runs/5069154901?check_suite_focus=true

[jean-airoldie]

Like mentioned previously, I'm fine with going the full static linking route everywhere as long we also vendor libsodium behind a curve feature and we remove the static feature.

[jean-airoldie]

Also update the .gitmodule file to the proper libzmq branch. I'm guessing it should be v4.3.4.

[jean-airoldie]

Also checkout the vendor repo to the v4.3.4 branch.

[Jasper-Bekkers]

Like mentioned previously, I'm fine with going the full static linking route everywhere as long we also vendor libsodium behind a curve feature and we remove the static feature.

Right now it's set up such that if libsodium is enabled, we automatically enable the curve feature. Would you want it the other way around? Or just to have one feature for both?

[jean-airoldie]

Right now it's set up such that if libsodium is enabled, we automatically enable the curve feature. Would you want it the other way around? Or just to have one feature for both?

It's better to have one feature. I think its better to use the CURVE feature because most users don't really care about implementation details of what exact encryption library is used.

[Jasper-Bekkers]

It's better to have one feature. I think its better to use the CURVE feature because most users don't really care about implementation details of what exact encryption library is used.

I see what you mean - the reason it is the way it is, is because the libsodium feature requires some additional info to know where to locate it and it's headers. Want me to switch it - we'll have to move the libsodium params to the curve feature which also doesn't quite sound ideal?

[jean-airoldie]

I see what you mean - the reason it is the way it is, is because the libsodium feature requires some additional info to know where to locate it and it's headers. Want me to switch it - we'll have to move the libsodium params to the curve feature which also doesn't quite sound ideal?

What I was thinking was to depend on libsodium-sys-stable and enable it via the curve feature. This way we also vendor libsodium. I can do that part on another PR before the release if you want me to.

[Jasper-Bekkers]

@jean-airoldie I /think/ I've addressed most of your feedback - thanks, and I've made sure the build is green. I'm calling it quits for tonight, please let me know if there is any more feedback and I'll address it during the week.

[Jasper-Bekkers]

@jean-airoldie We've been running with this for a few days now - do you think it's mergable and if so could you also spin a new release?

[jean-airoldie]

Sorry, I totally forgot about this, i'll do this today.

[jean-airoldie]

I'm thinking I'm gonna merge, then do some of the changes I wanted to do myself, so its faster. In this case I was thinking of removing the artifact structure entirely and link against libsodium automatically when curve is enabled.

[jean-airoldie]

Thanks for your work! I'm sure making this build work was painful.