briansmith / ring

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

prefixed_export warnings on some architectures #1896

Closed DavidHorton5339 closed 4 months ago

DavidHorton5339 commented 8 months ago

Recent changes have marked prefixed_export! macro as deprecated, and simultaneously tried to ignore the deprecated macro option. Unfortunately this still throws up a warning which causes builds to fail. The macro is only defined/used for architectures other than aarch64, arm, x86 and x86_64, so only those architectures are affected. Example targets exhibiting the problem are powerpc-unknown-linux-gnu, mipsel-unknown-linux-gnu warning: use of deprecated macro prefixed_export: #[export_name] creates problems and we will stop doing it.

   --> src/arithmetic/montgomery.rs:136:1
    |
136 | prefixed_export! {
    | ^^^^^^^^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

warning: unused attribute `allow`
   --> src/arithmetic/montgomery.rs:135:1
    |
135 | #[allow(deprecated)]
    | ^^^^^^^^^^^^^^^^^^^^
    |
note: the built-in attribute `allow` will be ignored, since it's applied to the macro invocation `prefixed_export`
   --> src/arithmetic/montgomery.rs:136:1
    |
136 | prefixed_export! {
    | ^^^^^^^^^^^^^^^
    = note: `#[warn(unused_attributes)]` on by default
briansmith commented 8 months ago

Hmm...why would our CI jobs pass if this happens?

DavidHorton5339 commented 8 months ago

They are warnings. Does your CI treats warnings as errors (mk/cargo.sh test run from command line does not)?

DavidHorton5339 commented 8 months ago

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

briansmith commented 8 months ago

I do see the warnings but they do not cause the build to break.

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

Is there a way to get the #[allow(deprecated]) to work? If not, I will accept a PR that replaces the #[deprecated] with a comment and that removes the useless #[allow(deprecated]).

DavidHorton5339 commented 8 months ago

I think the code as it is is trying to apply #[allow(deprecated)] to the function after macro expansion, rather than to the macro invocation itself. I don't know of a way of applying an attribute to a macro invocation - I doubt this is possible. The only way I could get it to work is by having #![allow(deprecated)] at the file level in montgomery.rs (and removing the useless #[allow(deprecated] on the macro invocation). Would that work for you?

briansmith commented 8 months ago

I think we're better off just replacing the #[deprecated(...)] on the definition with a comment, and then removing the #[allow(deprecated)]. I still want to get other deprecation warnings, if any, in montgomery.rs.

DavidHorton5339 commented 7 months ago

Yes, that would be better than using the deprecation as a TODO marker, then masking it.

briansmith commented 4 months ago

I'm going to close this. It is annoying that we have these warnings but we don't need to have an issue to track the annoyance.