search

rust-embedded / rust-i2cdev

Rust library for interfacing with i2c devices under Linux
Apache License 2.0
205 stars 53 forks source link

Update nix to 0.25, hide nix from public API. #74

Closed Erk- closed 1 year ago

Erk- commented 1 year ago

Hiding nix from the public API will allow to update it in the future without causing a breaking change.

rust-highfive commented 1 year ago

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nastevens (or someone else) soon.

Please see the contribution instructions for more information.

Erk- commented 1 year ago

The nix error is a simple i32, so it could be resolved by making it possible to do that conversion.

eldruin commented 1 year ago

Its representation is just an i32 but the type is still an enum. If we reduce it to just the value, downstream crates would still need to reimplement the platform-specific value matching already in place in nix/libc. Worse yet, without versioning information so I think that is not a solution.

Erk- commented 1 year ago

In the example you gave that is already how std::io::Error is handled, so I am not sure it would cause issues as you describe.

The main issue I see with the current system is that is just breaks semver as it have for the past few releases which leads to other issues downstream. The other solution would of course be to change nix update releases to be breaking.

eldruin commented 1 year ago

True, I see nix can reconstruct the error type from the raw OS error so the matching downstream can be delegated to a self-defined nix version. We can probably do the same for nix errors as well. We would only need to assess when do the values actually change, but I am guessing that is never because they are all just reexported from libc and we only need to think about potential additions/removals to the enum. In this case I think downstream code would still continue to work since the error enum is non_exahustive even with different nix versions. If we do this, I think we can remove the Errno struct and just provide the i32 value from nix, though, since downstream crates won't be able to do much with that variant other than calling cause(). I would also make it clear that the value comes from nix and that nix can be used to interpret it.

Regarding breaking changes, nix updates should only be done in breaking releases as it is currently part of the public API. This was simply overseen in release 0.5.1.

Erk- commented 1 year ago

I have made the changes you requested with a note on how to reconstruct it as a nix error.

Regarding breaking changes, nix updates should only be done in breaking releases as it is currently part of the public API. This was simply overseen in release 0.5.1.

Makes sense, stuff like it happens sometimes.

eldruin commented 1 year ago

Just a nitpick left. Other than that this is good to go.

bors[bot] commented 1 year ago

Build succeeded:

bors[bot] commented 1 year ago

Build succeeded: