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

Update to nix-0.28 #176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

neumann-paulmann
Copy link

While at it, use std::os::fd::owned::OwnedFd instead of custom implementation.

Fixes #175

While at it, use std::os::fd::owned::OwnedFd instead of custom implementation.
Copy link
Contributor

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work!
Could you look into the CI failures?
Also, could you add an entry to the changelog about this?

@neumann-paulmann
Copy link
Author

neumann-paulmann commented Apr 5, 2024

Hi. This looks like a MSRV issue.
The changelog state that it's currently at 1.36, which is ancient while I'm developing on 1.76 currently.
The latest nix supposedly has a MSRV of 1.69.
Would it be possible to raise the MSRV of serialport to 1.69 as well?

@eldruin
Copy link
Contributor

eldruin commented Apr 5, 2024

Hmm, this does not seem MSRV-related to me, though. Here is a run with 1.76.0:
https://github.com/serialport/serialport-rs/actions/runs/8535157043/job/23396028026?pr=176#step:7:42

The MSRV is set to 1.59.0, where have you found the 1.36.0 reference?
Changing the MSRV has implications related to the supported platforms.
What do you think @sirhcel ?

@neumann-paulmann
Copy link
Author

You're right. I was looking at the first issues like here: https://github.com/serialport/serialport-rs/actions/runs/8535157043/job/23396025571?pr=176#step:7:32.
I, however, cannot reproduce the other issues on my machine with above version. The library builds and all tests run fine:

~/RustroverProjects/serialport-rs> cargo build                                                                                                                                                                                  2024-04-05T10:06:49
   Compiling serialport v4.3.1-alpha.0 (/home/neumann/RustroverProjects/serialport-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 0.73s
~/RustroverProjects/serialport-rs> git status                                                                                                                                                                                   2024-04-05T10:06:56
Auf Branch nix-0.28-2
Ihr Branch ist auf demselben Stand wie 'github/nix-0.28-2'.

Unversionierte Dateien:
  (benutzen Sie "git add <Datei>...", um die Änderungen zum Commit vorzumerken)
        .idea/

nichts zum Commit vorgemerkt, aber es gibt unversionierte Dateien
(benutzen Sie "git add" zum Versionieren)
~/RustroverProjects/serialport-rs> cargo clean                                                                                                                                                                                  2024-04-05T10:07:11
     Removed 2445 files, 801.1MiB total
~/RustroverProjects/serialport-rs> cargo build --all-features                                                                                                                                                                   2024-04-05T10:08:08
   Compiling proc-macro2 v1.0.79
   Compiling unicode-ident v1.0.12
   Compiling libc v0.2.153
   Compiling pkg-config v0.3.30
   Compiling thiserror v1.0.58
   Compiling cfg_aliases v0.1.1
   Compiling serde v1.0.197
   Compiling bitflags v2.5.0
   Compiling cfg-if v1.0.0
   Compiling scopeguard v1.2.0
   Compiling nix v0.28.0
   Compiling libudev-sys v0.1.4
   Compiling quote v1.0.35
   Compiling syn v2.0.58
   Compiling libudev v0.3.0
   Compiling thiserror-impl v1.0.58
   Compiling serde_derive v1.0.197
   Compiling unescaper v0.1.4
   Compiling serialport v4.3.1-alpha.0 (/home/neumann/RustroverProjects/serialport-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 5.82s
~/RustroverProjects/serialport-rs> cargo test --all-features                                                                                                                                                                    2024-04-05T10:08:18
   Compiling version_check v0.9.4
   Compiling syn v1.0.109
   Compiling autocfg v1.2.0
   Compiling heck v0.4.1
   Compiling os_str_bytes v6.6.1
   Compiling hashbrown v0.12.3
   Compiling textwrap v0.16.1
   Compiling strsim v0.10.0
   Compiling termcolor v1.4.1
   Compiling bitflags v1.3.2
   Compiling once_cell v1.19.0
   Compiling assert_hex v0.4.1
   Compiling atty v0.2.14
   Compiling clap_lex v0.2.4
   Compiling proc-macro-error-attr v1.0.4
   Compiling proc-macro-error v1.0.4
   Compiling indexmap v1.9.3
   Compiling clap_derive v3.2.25
   Compiling clap v3.2.25
   Compiling serialport v4.3.1-alpha.0 (/home/neumann/RustroverProjects/serialport-rs)
    Finished test [unoptimized + debuginfo] target(s) in 6.66s
     Running unittests src/lib.rs (target/debug/deps/serialport-65059b8e45381439)

running 1 test
test posix::tty::test_ttyport_into_raw_fd ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/test_serialport.rs (target/debug/deps/test_serialport-ddc0aa2fcb68f854)

running 6 tests
test test_configuring_ports ... ok
test test_duplicating_port_config ... ok
test test_opening_port ... ok
test test_opening_native_port ... ok
test test_listing_ports ... ok
test test_opening_found_ports ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

     Running tests/test_try_clone.rs (target/debug/deps/test_try_clone-9d7e1a4c1730c099)

running 2 tests
test test_try_clone ... ok
test test_try_clone_move ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/test_tty.rs (target/debug/deps/test_tty-43b2dd8eb44f5847)

running 4 tests
test test_ttyport_pair ... ok
test test_ttyport_set_nonstandard_baud ... ok
test test_ttyport_set_standard_baud ... ok
test test_ttyport_timeout ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.00s

   Doc-tests serialport

running 3 tests
test src/lib.rs - new (line 806) - compile ... ok
test src/posix/tty.rs - posix::tty::TTYPort::pair (line 228) ... ok
test src/posix/tty.rs - posix::tty::TTYPort (line 46) ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.17s

~/RustroverProjects/serialport-rs> git branch                                                                                                                                                                                   2024-04-05T10:08:31
  main
  nix-0.28
  nix-0.28-
* nix-0.28-2
~/RustroverProjects/serialport-rs>                         

Both variants provide a direct conversion from Duration, so there is no
need for us to deal with converting the timeout and signedness.
@sirhcel
Copy link
Contributor

sirhcel commented Apr 9, 2024

Hmm, this does not seem MSRV-related to me, though. [...]

No, this looks like an issue with the signedness of milliseconds in the explicit conversion from Duration. Both target types for the poll timeout support directly converting from Duration and we can work around it by using these direct conversions.

Ended up with bf7050c while looking into this. So there is just raising the MSRV left.

Changing the MSRV has implications related to the supported platforms. What do you think @sirhcel?

At a first glance at least when it comes to the Rust support shipped with Yocto. Dependents using older Yocto releases could still pin their dependency on us to a minor version and we could attempt to scan all publicly visible dependencies. And on the other hand there is also meta-rust-bin which provides the brand spanking new releases of Rust. Let me think and sleep over it.

@eldruin
Copy link
Contributor

eldruin commented May 22, 2024

Did you have time to think about this @sirhcel ?

@sirhcel
Copy link
Contributor

sirhcel commented May 24, 2024

I finally managed to. Just to get my perspective on this right: Is bumping our nix dependency tied to switching to OwnedFd? Or is the latter a drive-by catch happening in the context of bumping nix?

I'm fine with switching to OwnedFd with a minor release as this does not look like a breaking change to me as far as I'm overseeing this as of now.

When it comes to bumping nix, I would postpone this to the next major release as this could be a breaking change for dependents still relying on our MSRV being 1.59.0. I know this is subject to general discussion but we've got our MSRV for dev builds broken recently with our dependencies serialport -> clap -> os_str_bytes (see #186). I would like to spare dependents with tighter MSRV requirements this experience.

@neumann-paulmann
Copy link
Author

neumann-paulmann commented May 24, 2024

I think that changes regarding the used file descriptor objects are necessary and that the custom implementation is no longer needed.
I also agree to not rush this, since it might break things as you and others correctly pointed out.
I currently do not have the time to look into this further. I might be able to do some work on this in two weeks or so when I'm done with my other stuff.

@eldruin eldruin added this to the 5.0.0 milestone May 24, 2024
@max-heller
Copy link

Given the adoption of OwnedFd, could a std::os:fd::AsFd implementation be added as well?

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

Successfully merging this pull request may close these issues.

Update dependency on nix to 0.28
4 participants