briansmith / ring

Safe, fast, small crypto using Rust
Other
3.69k stars 697 forks source link

invalid_reference_casting warning for Rust toolset < 1.73 #1895

Closed DavidHorton5339 closed 6 months ago

DavidHorton5339 commented 8 months ago

MIPS targets (including mipsel-unknown-linux-musl and mips-unknown-linux-musl) have had rust compiler support removed. This means MIPS builds are stuck on v1.71 (1.72, 1.73 and 1.74 were broken). and cannot use a later Rust toolset. invalid_reference_casting lint has been added to the code, but this is unsupported in Rust toolsets before 1.73. This generates a warning which makes builds fail.

   Compiling ring v0.17.7 (/mnt/c/code/ring)
warning: unknown lint: `invalid_reference_casting`
  --> src/lib.rs:64:5
   |
64 |     invalid_reference_casting,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unknown_lints)]` on by default
briansmith commented 8 months ago

In CI we build with 1.61.1 as the MSRV and that build passes. Which version of Rust are you building with? Are our 1.61.1 jobs in CI doing something wrong?

DavidHorton5339 commented 8 months ago

mk/cargo.sh test also generates warnings, but warnings are not treated as errors here, which they are in the main build. So maybe the CI run has overlooked the problem? Specifically, I've seen this warning on 1.71, and having discovered that invalid_reference_casting is newly introduced (they renamed it, what were they thinking?!), wouldn't expect it to work on anything < 1.73. I tried mk/cargo.sh test with 1.61 to confirm this warning was present in that version (it also generated some additional warnings I haven't seen before).

DavidHorton5339 commented 8 months ago

Warnings confirmed in example https://github.com/briansmith/ring/actions/runs/7492345579/job/20400988301

briansmith commented 8 months ago

mk/cargo.sh test also generates warnings, but warnings are not treated as errors here, which they are in the main build.

When I do cargo +1.71.0 check I do indeed get a warning, but it doesn't cause the build to fail.

DavidHorton5339 commented 8 months ago

Sorry, should have made it clear - it's -D warnings in our application build that's causing builds to fail. We don't really expect warnings from 3rd party crates (or our own code), and ring did not formerly issue any.

briansmith commented 8 months ago

If there is a way to detect which rustc version will be used, that doesn't require shelling out to run rustc and which doesn't involve an external build dependency, then I am open to having build.rs add a old_rustc cfg flag, and then having a #![cfg_attr(old_rustc, allow(unknown_lints))] or similar. Unfortunately, I don't see a way to do that.

DavidHorton5339 commented 7 months ago

I liked your proposal of removing explicit reference to the lint as it is deny by default. That would resolve this issue.

DavidHorton5339 commented 6 months ago

1.78 resolves this issue - so can be closed.