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

Fix nightly #169

Merged
merged 4 commits into from Apr 4, 2024
Merged

Fix nightly #169

merged 4 commits into from Apr 4, 2024

Conversation

RossSmyth
Copy link
Contributor

@RossSmyth RossSmyth commented Mar 27, 2024

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:

unused

And importing a trait included in the prelude:

use std::convert::From;

If you attempt to build on nightly you get this error:

error: the item `From` is imported redundantly
   --> src\lib.rs:35:5
    |
35  | use std::convert::From;
    |     ^^^^^^^^^^^^^^^^^^
    |
   ::: C:\Users\rsmyth\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\std\src\prelude\mod.rs:129:13
    |
129 |     pub use core::prelude::rust_2021::*;
    |             ------------------------ the item `From` is already defined here
    |
note: the lint level is defined here
   --> src\lib.rs:23:5
    |
23  |     unused
    |     ^^^^^^
    = note: `#[deny(unused_imports)]` implied by `#[deny(unused)]`

error: could not compile `serialport` (lib) due to 1 previous 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

@RossSmyth
Copy link
Contributor Author

I accidentally made my fix branch with the regex-lite branch of my previous PR rather than main.

@sirhcel
Copy link
Contributor

sirhcel commented Mar 28, 2024

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.

@eldruin
Copy link
Contributor

eldruin commented Mar 28, 2024

I would rather remove this from the lib.rs and add it as a flag only to the CI (RUSTFLAGS="-D unused") precisely to avoid this. We should notice lint changes in the compiler but this should not make the crate impossible to build for users.

@sirhcel sirhcel force-pushed the FixNightly branch 3 times, most recently from 054e00f to fde0100 Compare March 28, 2024 22:14
sirhcel added a commit to RossSmyth/serialport-rs that referenced this pull request Mar 28, 2024
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.
@sirhcel
Copy link
Contributor

sirhcel commented Mar 28, 2024

I'm not that great at GitHub actions wrangling so it may be incorrectly formed. We'll see.

CI recipes often feel like Russian roulette with all chambers loaded ... Finally got this right this with 97f8fcc.

@sirhcel
Copy link
Contributor

sirhcel commented Mar 28, 2024

I would rather remove this from the lib.rs and add it as a flag only to the CI (RUSTFLAGS="-D unused") precisely to avoid this. We should notice lint changes in the compiler but this should not make the crate impossible to build for users.

Thank you for suggesting this! This feels like the right thing to do here for me too. Changed this with 8d94b1a:

  • Unused imports are no longer denied and generate a warning.
  • Warnings will be denied in CI. We are getting stricter here, but I see no point in integrating code generating warnings.

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?

@sirhcel
Copy link
Contributor

sirhcel commented Mar 28, 2024

Marked nightly builds as optional with continue-on-error (6b84960) but sadly this does not give the expected visual indication. The CI run is marked green in the actions, this PR is marked as failed with a red x and I would have loved to get a different indicator in both cases for "there might be trouble ahead".

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
@sirhcel
Copy link
Contributor

sirhcel commented Mar 29, 2024

Cleaned up the remaining issue with nightly from CI run 357 with 7fec3df.

@RossSmyth
Copy link
Contributor Author

Looks good to me. Yeah making it deny in CI I think is a good idea

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.

Looks good to me, thank you!

@eldruin eldruin merged commit 09bfb98 into serialport:main Apr 4, 2024
30 checks passed
@RossSmyth RossSmyth deleted the FixNightly branch April 11, 2024 13:08
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.

None yet

3 participants