WIP Feature/nng 1.2 #18

jeikabu · 2019-04-08T07:43:13Z

Fixes for nng master@HEAD what will become v1.2

…ture/nng_1.2

neachdainn · 2019-04-08T18:35:28Z

Glancing over it, it looks good to me. I'll double check it against Nng-rs later tonight.

neachdainn

This version uses nng_pipe_ev instead of c_int in several places, such as nng_pipe_notify, which I think is a very good change. However, the nng_pipe_ev::try_from function should be updated to take an nng_pipe_ev instead of an i32.

Other than that, it looks fine to me. If you want to merge without making the change, I won't complain.

neachdainn · 2019-04-16T17:09:30Z

Cargo.toml

@@ -45,6 +45,7 @@ source-update-bindings = ["build-bindgen"]
 cty = {version = "0.2", optional = true}

 [build-dependencies]
+version_check = "0.1"


I didn't know about this crate. It seems pretty cool - I'll have to look into it for my own projects.

…pdate nng

- Depend on default cmake behavior (see #23)

- Powershell treats writes to stderr as exceptions. Anything about crates.io goes to stderr... need to just ignore errors - https://help.appveyor.com/discussions/problems/5413-calling-external-executable-causes-nativecommanderror-despite-no-apparent-error

…into feature/nng_1.2

neachdainn · 2019-12-22T10:25:28Z

Let me know when this is ready to go live and I'll take a look / push it to Crates.io.

jeikabu · 2019-12-22T10:40:06Z

Let me know when this is ready to go live and I'll take a look / push it to Crates.io.

Should be pretty close. Go ahead and take a look.
This has gotten pretty messy, let me look at cleaning it up a bit.

neachdainn · 2019-12-22T10:43:48Z

Looks good to me. I'll push it when you say it's ready.

EDIT: And after it is merged into master.

neachdainn · 2019-12-22T12:27:30Z

src/lib.rs

        }
    }
 }

-impl nng_pipe_ev {


This enum doesn't match the others with try_convert_from and TryFrom implementation. It's small enough to not really matter, though.

If I remember correctly, those were just a temporary solution before TryFrom was stabilized, and nng_pipe_ev didn't exist then. Once we no longer care about Rust <1.34 we can just remove try_convert_from. Doesn't make sense to make them deprecated since there isn't a replacement for earlier rust.

neachdainn · 2019-12-22T12:45:59Z

src/bindings.rs

-        arg2: ::std::os::raw::c_int,
-        arg3: *mut ::std::os::raw::c_void,
-    ),
+    unsafe extern "C" fn(arg1: nng_pipe, arg2: nng_pipe_ev, arg3: *mut ::core::ffi::c_void),


This is a source of potential UB that the Rust wrappers can't guard against. C does not enforce that the nng_pipe_ev will be one of the allowed values and it is UB in Rust to not be a variant. It may be best to change the Bindgen settings to not use actual enums for the NNG enum types.

To be clear, I think having C enums as argument is good (Rust will enforce valid variants) but anywhere an enum is received is immediately potentially UB. See the Rust language reference. Using repr(u*) instead of repr(C) does not alleviate the issue.

Sure, let's switch them.

- Mixing of C and Rust enums is potentially undefined - rustfmt

jeikabu · 2020-01-03T09:12:55Z

@neachdainn Merge?

neachdainn · 2020-01-06T00:59:02Z

I'm having issues building this. I'll figure out why and then report back.

neachdainn · 2020-01-06T01:17:42Z

The issue seems to be with the nng-stats feature. Trying to build without that feature enabled leads to errors with undeclared types within the C code (stats_lock, stats_root). I'm not sure if this is a bug in the build script or if it's a bug with NNG's build.

Opened nanomsg/nng#1114 to address this. We could remove the feature and submit a new version but I would prefer to wait until this is fixed in NNG. That being said, I'm very eager to get a v1.2.* crate pushed and I'll be more open to removing the feature if NNG doesn't have a fixed release soon.

It appears to be fixed on the NNG master branch. So we can either remove the nng-stats feature or we can wait until v1.2.4

jeikabu · 2020-01-06T12:41:40Z

Yes, apparently it's broken in 1.2.3.

There's no sense in removing nng-stats, would just have to put it back in. The normal thing to do would make it a "known issue" in 1.2.3. Let it be fixed in 1.2.4- who knows when that will be and what else might be broken.

neachdainn · 2020-01-06T16:57:52Z

That sounds reasonable to me. You have my support to do the merge whenever you're ready.

jeikabu added 7 commits March 2, 2019 14:03

Pull nng HEAD

cd4c642

Legacy longer getopt/setopt functions behind feature

0e2180b

Update nng submodule

db6a7f1

Update nng

edf272f

Merge branch 'master' of https://github.com/jeikabu/nng-rust into fea…

b2e242c

…ture/nng_1.2

Update pre-generated bindings.rs

e1f059d

Remove unused feature

07b75e1

jeikabu requested a review from neachdainn April 8, 2019 07:43

neachdainn reviewed Apr 11, 2019

View reviewed changes

neachdainn mentioned this pull request Apr 14, 2019

Move to Nanomsg domain and publish on Crates.io #17

Closed

jake-ruyi force-pushed the feature/nng_1.2 branch from 71ba82e to 95be805 Compare April 16, 2019 12:12

neachdainn reviewed Apr 16, 2019

View reviewed changes

TryFrom stabilized in 1.34, nng_pipe_ev::try_from no longer needed, u…

20217cf

…pdate nng

jake-ruyi force-pushed the feature/nng_1.2 branch from 95be805 to 20217cf Compare April 23, 2019 05:12

jeikabu added 11 commits April 27, 2019 15:13

Update nng

b615c2c

Pull nng HEAD

013c98d

Legacy longer getopt/setopt functions behind feature

eb7469a

Update nng submodule

f43b221

Update nng

ac30bff

Update pre-generated bindings.rs

aa3c332

Remove unused feature

188dda4

TryFrom stabilized in 1.34, nng_pipe_ev::try_from no longer needed, u…

bdd72d1

…pdate nng

Update nng

961e6a7

Update nng@72dddb474e67cfd167012a8368b3bdbe9615497e

62a66b6

nng@32d1751

8f16b82

jeikabu force-pushed the feature/nng_1.2 branch from 44648d0 to 8f16b82 Compare October 8, 2019 08:40

jeikabu added 3 commits October 8, 2019 12:46

Update src/bindings.rs

14155be

Only set cmake generator if explicitly specified

4a980b0

- Depend on default cmake behavior (see #23)

jeikabu added 4 commits October 19, 2019 17:08

Merge branch 'feature/nng_1.2' of https://github.com/jeikabu/nng-rust …

0a082d2

…into feature/nng_1.2

Fix merge conflict

1c35978

Update nng@4bfd76e99eda7beed757506b2aec10b4a27458db

065fecc

Update nng to 1.2 c0e2927ffc3b8c6fa34a1ce66bd3a0d7a4d9a52d

33f565e

Update bindgen to 0.52 and regenerate bindings

42b8bb1

neachdainn reviewed Dec 22, 2019

View reviewed changes

jeikabu added 2 commits January 1, 2020 10:18

Update [email protected] 3c6ecbd617d8d187f6963b5a43dc1abe5efae12b

9da0032

nng_pipe_ev to const and update bindings

62b7a54

- Mixing of C and Rust enums is potentially undefined - rustfmt

jeikabu force-pushed the feature/nng_1.2 branch from b096d66 to 62b7a54 Compare January 1, 2020 09:57

jeikabu closed this Jan 6, 2020

jeikabu reopened this Jan 6, 2020

jeikabu merged commit c3c5a0e into master Jan 7, 2020

jeikabu deleted the feature/nng_1.2 branch January 7, 2020 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Feature/nng 1.2 #18

WIP Feature/nng 1.2 #18

jeikabu commented Apr 8, 2019

neachdainn commented Apr 8, 2019

neachdainn left a comment

neachdainn Apr 16, 2019

neachdainn commented Dec 22, 2019

jeikabu commented Dec 22, 2019 •

edited

Loading

neachdainn commented Dec 22, 2019 •

edited

Loading

neachdainn Dec 22, 2019

jeikabu Jan 3, 2020

neachdainn Dec 22, 2019

neachdainn Dec 22, 2019 •

edited

Loading

jeikabu Dec 22, 2019

jeikabu commented Jan 3, 2020

neachdainn commented Jan 6, 2020

neachdainn commented Jan 6, 2020 •

edited

Loading

jeikabu commented Jan 6, 2020 •

edited

Loading

neachdainn commented Jan 6, 2020

WIP Feature/nng 1.2 #18

WIP Feature/nng 1.2 #18

Conversation

jeikabu commented Apr 8, 2019

neachdainn commented Apr 8, 2019

neachdainn left a comment

Choose a reason for hiding this comment

neachdainn Apr 16, 2019

Choose a reason for hiding this comment

neachdainn commented Dec 22, 2019

jeikabu commented Dec 22, 2019 • edited Loading

neachdainn commented Dec 22, 2019 • edited Loading

neachdainn Dec 22, 2019

Choose a reason for hiding this comment

jeikabu Jan 3, 2020

Choose a reason for hiding this comment

neachdainn Dec 22, 2019

Choose a reason for hiding this comment

neachdainn Dec 22, 2019 • edited Loading

Choose a reason for hiding this comment

jeikabu Dec 22, 2019

Choose a reason for hiding this comment

jeikabu commented Jan 3, 2020

neachdainn commented Jan 6, 2020

neachdainn commented Jan 6, 2020 • edited Loading

jeikabu commented Jan 6, 2020 • edited Loading

neachdainn commented Jan 6, 2020

jeikabu commented Dec 22, 2019 •

edited

Loading

neachdainn commented Dec 22, 2019 •

edited

Loading

neachdainn Dec 22, 2019 •

edited

Loading

neachdainn commented Jan 6, 2020 •

edited

Loading

jeikabu commented Jan 6, 2020 •

edited

Loading