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

• [x] Windows
• [x] Linux
• [x] 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]

How attached are you to breaking/non-breaking changes to this crate?

I think it probably makes sense to do a breaking change since the CI is this crate is probably a lil lacking and I wouldn't want to run the risk of breaking someone's build because of the move to cc. Some platforms a pretty finicky to build zmq on so i think its safer this way.

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

Cool

[jean-airoldie]

As you mentionned i think it would make sense to make libsodium the default encryption lib otherwise it's kind of a footgun. I think the ideal solution would be to have a curve feature that enables both the ZMQ_CURVE flag and links to libsodium.

I'm not using ZMQ anymore to be clear, but in my archived repo I have an example of how to do the libsodium linking.

edit: fixed the link

[jean-airoldie]

Any platforms you care about that should be covered, I'm considering dropping BSD from this PR.

Only platforms I care about are windows and linux, so feel free to drop BSD.

[Jasper-Bekkers]

As you mentionned i think it would make sense to make libsodium the default encryption lib otherwise it's kind of a footgun. I think the ideal solution would be to have a curve feature that enables both the ZMQ_CURVE flag and links to libsodium.

I'm not using ZMQ anymore to be clear, but in my archived repo I have an example of how to do the libsodium linking.

edit: fixed the link

Looking at this now I see why you've set it up like that at the time - I think I'll have to come back on making libsodium a default feature, since it looks like the rust crate for it has been deprecated a while ago. I'll just add a test for it and leave the integration as-is.

[Jasper-Bekkers]

@jean-airoldie I've started using this PR in production now on our end, and it works on all the platforms we care about. I'll update the PR description and title a bit to reflect that, so for all intents and purposes this can be considered as "done" on my end.

There's quite a trail of nasty commits that I could clean up if you're intending not to squash this PR, but otherwise, unless you have no other feedback, I'd be happy to send this off.

[jean-airoldie]

Here's a sanity checking patch file for libsodium in the testcrate. 0001-Improve-libsodium-test.txt You can apply it via git am <filename>.

[jean-airoldie]

Looking at this now I see why you've set it up like that at the time - I think I'll have to come back on making libsodium a default feature, since it looks like the rust crate for it has been deprecated a while ago.

If you look at the patch I previously linked, you can see the libsodium-sys-stable crate that is maintained by the libsodium creator. It would be pretty simple to automatically link to it via a feature, in the same way I did in the testcrate.

[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.