search

jeikabu / runng

MIT License
25 stars 3 forks source link

Agree on a crate for linking to libnng #29

Closed neachdainn closed 5 years ago

neachdainn commented 5 years ago

Runng and Nng-rs currently use different crates for linking to the NNG library. If my understanding is correct, that means that anyone who has both Nng-rs and Runng in their dependency tree will have linker issues1. I think it would be wise for us to pick a single "sys" crate that we both use. I don't particularly care if we use your code or my code but we should probably utilize the nng-sys crate name to match the community style.

Nng-rs repository

1: I was going to run a quick test of this but I ran into issues compiling Runng. It's probably something wrong with my environment - I didn't investigate because I don't think it impacts the point I'm trying to make.

jeikabu commented 5 years ago

I was vaguely aware nng-sys was back in use. It was mostly empty and hadn't been touched in 6 months when I started. I thought we were going to have changes to suit our needs, but in practice it's almost never touched.

I'm fine with moving to nng-sys. Couple of thoughts:

neachdainn commented 5 years ago

I did look into bindgen when I took the name over, I just wasn't a fan of the amount of clutter that it generates or how things were named (e.g., things like this and this). That being said, I'm definitely willing to use it now that I'm not the only person using the crate, and perhaps it is more configurable that I originally thought. I am a bit wary about the platform thing, since I just had an issue with that.

I think it would be a very good idea to talk to Garrett and put the nng-sys crate under the Nanomsg org. That's a crate that all wrappers will need to use and, if bindgen is used, won't have any opinions or need to be touched very often. It makes sense to put it somewhere official.

I'm not sure I follow the big about version numbers. Am I doing something that isn't 1.1.1-rc.x? If I'm not, I would definitely like to correct that.

EDIT: I just figured out why I couldn't get Runng to build and I remembered another reason why I decided to not use Bindgen: it creates a dependency on libclang. Right now, the only requirements for the crate are Rust, C99, and CMake which makes it a fairly light project. I suppose a compromise might be to manually run Bindgen on every NNG release? That won't work well with the various platforms. I'm not sure what the best solution in here.

jeikabu commented 5 years ago

Yeah, that's the compromise of bindgen. You'll get everything wrapped for free, but the naming conventions may not be ideal. You can always add a thin adapter layer if you think anyone would want to use the C functions.

Apparently the portable rust type is std::os::raw::c_char. Need to use that instead of i8/u8.

Admittedly the clang dependency is a bummer. Most "regular systems" probably have clang (even docs.rs does). I have run into problems in VM (like travis-ci) and docker scenarios. Suppose you could change the output folder per-platform and check it in with the source.

To me the most important things are:

  1. Keep it sync'd with libnng
  2. Keep it sync'd with Rust (it should work with past/future versions by using the corresponding version of bindgen)
neachdainn commented 5 years ago

I know that there are people using my version of nng-sys on Android, so I wouldn't be very happy just dropping the Clang dependency on them. I also have plans in the future to use nng-rs on odd platforms that might not have Clang available and are slow enough that I wouldn't want to try and build it.

I don't think there's a major rush to get the unification done, so I'll keep thinking this issue over, but I'm starting to like the compromise I mentioned in my previous post. Manually running Bindgen every release should keep things in sync with both Rust and NNG without having adding the dependency on Clang. We might encounter troubles with odd platforms but we could probably manually patch those without too much effort. It would also allow us to do some cool things with features, i.e., have a set of features that would allow you to select the particular NNG version you want to use.

jeikabu commented 5 years ago

Sure, sounds reasonable.

gdamore commented 5 years ago

If you guys want a repo on the nanomsg org, let me know. I'm amenable.

neachdainn commented 5 years ago

I think that would be very good. I'll have time this weekend to play around with Bindgen and see if I can get a repository that works as the backing for both Runng and Nng-rs.

jeikabu commented 5 years ago

I went ahead and split off a new repo that should do the trick:

Sent you an invite and it should be a much better starting point for you. With a couple of tweaks should be good to transfer to nanomsg org.

neachdainn commented 5 years ago

I've only had a few minutes to look over it but it looks like a solid start. Looking through the docs you linked, it doesn't seem like there is really anything that will end up being platform specific, so I'll play around with removing the Bindgen dependency (or perhaps put it behind a feature?) this weekend. I'll submit a merge request once I have one ready.

What exactly is the legacy-111-rc4 feature?

jeikabu commented 5 years ago

legacy-111-rc4 is temporary so I can still get the old bindings. It's safe to remove since this is definitely looking like the way forward. I have a branch with the needed changes to runng.

Removing that junk got rid of 20KB of binary (seriously) so you're right you may be able to make bindgen optional. I'm rooting for you.

Feel free to change readme or anything else, I'm not attached to any of it.

neachdainn commented 5 years ago

My first set of changes is ready for you to look at over at the pull request.

jeikabu commented 5 years ago

How are things looking in nng-rs? Have had a chance to look into integrating the common lib? Run into any problems?

neachdainn commented 5 years ago

I will try to do this tonight and will make it a priority over the weekend if I don't find the time today.

jake-ruyi commented 5 years ago

No rush, was just curious if it looked like it could work for you. My plan is to wait for what will probably be nng 1.2 since it includes a number of API changes. Release that as the final version of runng-sys, and then two weeks after that just do a package swap for the shared nng-sys.

neachdainn commented 5 years ago

That sounds like a good plan. Do you have any idea when the nng 1.2 release is going to be? I didn't look too long, but I couldn't find anything about it in the repo.

gdamore commented 5 years ago

I am working on bug fixes and performance stuff. Probably soon as I don't expect any new features. Sent from my Verizon, Samsung Galaxy smartphone -------- Original message --------From: Nate Kent notifications@github.com Date: 2/22/19 5:45 PM (GMT-08:00) To: jeikabu/runng runng@noreply.github.com Cc: gdamore garrett@damore.org, Comment comment@noreply.github.com Subject: Re: [jeikabu/runng] Agree on a crate for linking to libnng (#29) That sounds like a good plan. Do you have any idea when the nng 1.2 release is going to be? I didn't look too long, but I couldn't find anything about it in the repo.

—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/jeikabu/runng","title":"jeikabu/runng","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/jeikabu/runng"}},"updates":{"snippets":[{"icon":"PERSON","message":"@neachdainn in #29: That sounds like a good plan. Do you have any idea when the nng 1.2 release is going to be? I didn't look too long, but I couldn't find anything about it in the repo."}],"action":{"name":"View Issue","url":"https://github.com/jeikabu/runng/issues/29#issuecomment-466603950"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/jeikabu/runng/issues/29#issuecomment-466603950", "url": "https://github.com/jeikabu/runng/issues/29#issuecomment-466603950", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

jeikabu commented 5 years ago

Everybody is using nng-rust so closing this.