Agree on a crate for linking to libnng #29

neachdainn · 2019-01-27T23:52:01Z

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 issues¹. 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.

The text was updated successfully, but these errors were encountered:

jeikabu · 2019-01-28T01:29:56Z

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:

Use bindgen
- NNG being "plain C" it could be a poster-child for bindgen and is painless to keep it up to date
- I've been using it on macOS/win32/win64/ubuntu and now arm64
- The only problems I've run into is that sometimes different platforms use different types (e.g. os::c_char is i8 vs u8, enum ends up i32 vs u32)
I wish it existed outside a workspace/virtual manifest
- Rust/cargo has issues here that makes using crate "features" clumsy
- We could talk to gdamore about putting it under nanomsg org
Use caution with the version numbers. 1.1.1-rc.X was the only thing that crates.io, docs.rs, and cargo all agreed with....

neachdainn · 2019-01-28T01:47:06Z

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 · 2019-01-28T02:05:08Z

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:

Keep it sync'd with libnng
Keep it sync'd with Rust (it should work with past/future versions by using the corresponding version of bindgen)

neachdainn · 2019-01-28T02:40:40Z

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 · 2019-01-28T02:47:27Z

Sure, sounds reasonable.

gdamore · 2019-01-28T16:23:50Z

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

neachdainn · 2019-01-28T18:44:14Z

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 · 2019-01-30T15:51:26Z

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

Stand-alone crate containing just the NNG library bindings
Cleaned up the generated code (docs.rs)
Setup initial linux/macOS and win64 builds
Updated crate meta-data and docs
Added your "build-nng" feature
Made no_std compatible

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 · 2019-01-31T17:58:48Z

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 · 2019-02-01T01:30:59Z

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 · 2019-02-04T22:23:10Z

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

jeikabu · 2019-02-22T01:32:19Z

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

neachdainn · 2019-02-22T02:02:38Z

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 · 2019-02-22T04:08:56Z

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 · 2019-02-23T01:45:21Z

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 · 2019-02-23T02:17:44Z

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":"#29 (comment)"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "#29 (comment)", "url": "#29 (comment)", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

jeikabu · 2019-04-28T08:02:52Z

Everybody is using nng-rust so closing this.

jeikabu mentioned this issue Jan 28, 2019

Nng-rust repo nanomsg/nng#866

Closed

neachdainn mentioned this issue Feb 4, 2019

WIP: Remove Bindgen Dependency jeikabu/nng-rust#1

Closed

jeikabu closed this as completed Apr 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Agree on a crate for linking to libnng #29

Agree on a crate for linking to libnng #29

neachdainn commented Jan 27, 2019 •

edited

Loading

jeikabu commented Jan 28, 2019

neachdainn commented Jan 28, 2019 •

edited

Loading

jeikabu commented Jan 28, 2019 •

edited

Loading

neachdainn commented Jan 28, 2019 •

edited

Loading

jeikabu commented Jan 28, 2019

gdamore commented Jan 28, 2019

neachdainn commented Jan 28, 2019

jeikabu commented Jan 30, 2019 •

edited

Loading

neachdainn commented Jan 31, 2019

jeikabu commented Feb 1, 2019 •

edited

Loading

neachdainn commented Feb 4, 2019

jeikabu commented Feb 22, 2019

neachdainn commented Feb 22, 2019

jake-ruyi commented Feb 22, 2019

neachdainn commented Feb 23, 2019

gdamore commented Feb 23, 2019 via email

jeikabu commented Apr 28, 2019

Agree on a crate for linking to libnng #29

Agree on a crate for linking to libnng #29

Comments

neachdainn commented Jan 27, 2019 • edited Loading

jeikabu commented Jan 28, 2019

neachdainn commented Jan 28, 2019 • edited Loading

jeikabu commented Jan 28, 2019 • edited Loading

neachdainn commented Jan 28, 2019 • edited Loading

jeikabu commented Jan 28, 2019

gdamore commented Jan 28, 2019

neachdainn commented Jan 28, 2019

jeikabu commented Jan 30, 2019 • edited Loading

neachdainn commented Jan 31, 2019

jeikabu commented Feb 1, 2019 • edited Loading

neachdainn commented Feb 4, 2019

jeikabu commented Feb 22, 2019

neachdainn commented Feb 22, 2019

jake-ruyi commented Feb 22, 2019

neachdainn commented Feb 23, 2019

gdamore commented Feb 23, 2019 via email

jeikabu commented Apr 28, 2019

neachdainn commented Jan 27, 2019 •

edited

Loading

neachdainn commented Jan 28, 2019 •

edited

Loading

jeikabu commented Jan 28, 2019 •

edited

Loading

neachdainn commented Jan 28, 2019 •

edited

Loading

jeikabu commented Jan 30, 2019 •

edited

Loading

jeikabu commented Feb 1, 2019 •

edited

Loading