Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Edition from 2018 to 2015 #10

Closed
neachdainn opened this issue Feb 26, 2019 · 9 comments
Closed

Change Edition from 2018 to 2015 #10

neachdainn opened this issue Feb 26, 2019 · 9 comments

Comments

@neachdainn
Copy link
Collaborator

This crate should support as many versions of Rust as possible and I believe specifying the "edition" in the Cargo.toml locks it to 1.31 and above. To my knowledge, the Bindgen generated files and the hand-written files aren't doing anything that requires any relatively new Rust version.

We should also set up CI such that it makes sure we don't accidentally bump the minimum Rust version.

@neachdainn
Copy link
Collaborator Author

I bring this up because I will need to support Nng-rs on Ubuntu 14.04 which currently is at Rust v1.30 and may or may not ever upgrade. In this particular situation, Rustup is not an option.

@jeikabu
Copy link
Owner

jeikabu commented Feb 27, 2019

Ok with me. 2015 and 2018 libraries can supposedly be linked together without issue.
You able to take a look at getting this working?
Thought I remembered seeing a bindgen option related to editions, but maybe it was rust_target (which is probably still useful).

For CI I'm pretty sure we can just specify we want 1.30.

@neachdainn
Copy link
Collaborator Author

The libraries can be linked together but I do not know what a pre-1.31 Cargo will do with the "edition" key. Setting the rust_target would actually be a very good thing to do - I had no idea that option even existed. A good plan would probably be to use CI/Docker/Something to see the minimum version that the current setup supports, and then use that version in CI.

I'm not categorically against using newer Rust versions, I just know that there has been some discussion in the Rust community about supporting old versions (1 2), and I happen to know that I'll be targeting a platform that is stuck at 1.30, so I figured the discussion would be worth having.

@jeikabu
Copy link
Owner

jeikabu commented Feb 28, 2019

I definitely support the idea of the lowest-level/most-common dependencies having the least-restrictive requirements possible.

@neachdainn
Copy link
Collaborator Author

neachdainn commented Apr 3, 2019

I am currently working on a branch of Nng-rs that utilizes this crate as the bindings to NNG. Once that is complete, I will support publishing this to Crates.io as soon as this issue is resolved.

I am also open to waiting for the next NNG release, if we so choose.


EDIT: It is a little bit of a pain point that Bindgen has all of the function return an i32 but the error codes are u32. Nothing major, but I'm curious if you've noticed that as well.

EDIT: Ubuntu updated the Rustc version for 14.04, so I personally no longer need to target 1.30. While I do support seeing how old of a version we can support without additional work, I will personally be content with 1.31.

FINAL EDIT: I have a branch ready to use this version of nng-sys.

@jeikabu
Copy link
Owner

jeikabu commented Apr 3, 2019

I hadn't noticed the type of the returns because I immediately turn everything into an enum.

Sounds like this is pretty close. Nng development has slowed down the past month so I've been focused on other things.

@neachdainn
Copy link
Collaborator Author

neachdainn commented Apr 3, 2019

I hadn't noticed the type of the returns because I immediately turn everything into an enum.

It's a very minor pain point and having a unified nng-sys far outweighs this. But for me, it appears in a few small places. E.g.,

let flags = if nonblocking { nng_sys::NNG_FLAG_NONBLOCK } else { 0 }; // u32
let rv = nng_sys::nng_listen(handle,, addr, ptr::null_mut(), flags as i32); // i32
if rv != 0 {
    Error::from_code(rv as u32) // All error constants are u32
}
else {
    // ...
}

Sounds like this is pretty close.

Yeah, but I did just discover an issue when compiling with 1.31. I'm looking into it now to see what is happening. Note that this doesn't happen with the latest stable or nightly, but I do need to support 1.31.


EDIT: The issue appears to be related to this issue and this merge request. According to IRC, we need to change the line to use crate::bindings::*;. I'll give it a test and submit a merge request if it works.

@neachdainn
Copy link
Collaborator Author

I just realized that tracking the minimum version on Nng-rs also tracks the minimum version of this library, so I am good to begin the process of moving this formally into the NNG domain and then pushing it to Crates.io as soon as #16 is resolved.

@neachdainn
Copy link
Collaborator Author

I'm closing this since I now only need to support back to 1.31 and there are tests in place to make sure that the minimum version doesn't get bumped by accident.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants