-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fix nightly #169
Fix nightly #169
Conversation
I accidentally made my fix branch with the regex-lite branch of my previous PR rather than main. |
Thank you for spotting this and the fix! My gut instinct would not let CI builds fail due to issues from nightly - however it would be nice to still detect them. Could you please factor out adding additional CI targets into a separate PR? This would allow to get the first one merged quickly and we could create optional build targets in the latter one. |
I would rather remove this from the lib.rs and add it as a flag only to the CI ( |
054e00f
to
fde0100
Compare
This is definitely worth a warning but should not make builds fail for users. In CI builds warnings are denied by default and so this will be sorted out there before integrating changes. See issue serialport#169 for more details.
This is definitely worth a warning but should not make builds fail for users. We deny warnings in CI and sort this out before integrating. See issue serialport#169 for more details.
CI recipes often feel like Russian roulette with all chambers loaded ... Finally got this right this with 97f8fcc. |
Thank you for suggesting this! This feels like the right thing to do here for me too. Changed this with 8d94b1a:
This can bee see in seen in this CI run and I will make failing nightly jobs not fail the entire build in a next step. Are you okay with that @eldruin? |
Marked nightly builds as optional with I've seen this in GitLab but this seems not to be available on GitHub as of now (see https://github.com/orgs/community/discussions/11592). At least we've marked the intent in the build recipes and I would let it rest in this state for now. |
Current nightly considers use nix::self as an unused import as nix is already through the extern prelude (since edition 2018) and generates a warning. Remove the explicit import to silence it. See: * Output of 'cargo --verbose check' * https://doc.rust-lang.org/stable/reference/names/preludes.html#extern-prelude
Cleaned up the remaining issue with nightly from CI run 357 with 7fec3df. |
Looks good to me. Yeah making it deny in CI I think is a good idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you!
Currently this crate does not build on nightly due to lint changes. Specially there are two lines that cause compilation failure:
A
deny(unused)
lint:serialport-rs/src/lib.rs
Line 23 in 84f6066
And importing a trait included in the prelude:
serialport-rs/src/lib.rs
Line 35 in 84f6066
If you attempt to build on nightly you get this error:
This fixes the lint, and add nightly to CI. I'm not that great at GitHub actions wrangling so it may be incorrectly formed. We'll see.
rust-lang/rust#121315