mozilla-services / common-rs

Common utilities for Mozilla server side apps
Mozilla Public License 2.0
3 stars 9 forks source link

fix: correct module import path for maxmind/actix-web-v4 #31

Closed ethowitz closed 2 years ago

ethowitz commented 2 years ago

There was a compile error that wasn't caught by CI. As of right now, I think CI only checks compilation with each feature individually but not combinations of features, and this particular error only occurs when compiling with both the maxmind and actix-web-v4 features.

ethowitz commented 2 years ago

We may want to use --feature-powerset instead of --each-feature with cargo hack, though I guess it would fail when compiling actix-web-v3 with actix-web-v4...

EDIT: apparently not...

vm common-rs • tracing-actix-web-mozlog-0.3.2 $ cargo build --features actix-web-v3,actix-web-v4
warning: use of deprecated function `warning::actix_web_location_must_specify_one_version_of_actix_web`: Only one of actix-web-v3 and actix-web-v4 can be used at once. This entire crate will be disabled.
  --> actix-web-location/src/lib.rs:54:9
   |
54 |         actix_web_location_must_specify_one_version_of_actix_web()
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: `actix-web-location` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s
vm common-rs • tracing-actix-web-mozlog-0.3.2 $
jrconlin commented 2 years ago

Wait, I'm confused by that warning. It's saying that one of the crates will be discarded, but not which one.

Ah, so it according to cargo tree it brings in only the latest actix-web (v4.0.0-beta.15), and actix-http (v3.0.0-beta.16)

We should probably make that explicit.

ethowitz commented 2 years ago

I think it's saying that the entire actix-web-location crate will be discarded if the crate is compiled with both actix-web-v3 and actix-web-v4, but I could be wrong. My point above was just that compilation does, in fact, succeed (albeit, with a warning) when including both features, which is sufficient for CI purposes

jrconlin commented 2 years ago

Yeah, I'm just concerned about whatever decides to include this crate might have "unexpected behaviors" (Honestly, I'd hold off updating this crate 'til January for kind of that reason).

Remind me to tell you my "Well, it compiled" story sometime. It's a delightfully itchy scar.

mythmon commented 2 years ago

The warning above was generated by this code:

/* If both v3 and v4 are enabled at the same time, generate a compiler warning. */
#[cfg(all(feature = "actix-web-v3", feature = "actix-web-v4"))]
mod warning {
    #![allow(dead_code)]

    #[deprecated(
        note = "Only one of actix-web-v3 and actix-web-v4 can be used at once. This entire crate will be disabled."
    )]
    fn actix_web_location_must_specify_one_version_of_actix_web() {}
    fn trigger_warning() {
        actix_web_location_must_specify_one_version_of_actix_web()
    }
}

Every export at the top level of the location crate has this attached:

#[cfg(any(
    all(feature = "actix-web-v3", not(feature = "actix-web-v4")),
    all(not(feature = "actix-web-v3"), feature = "actix-web-v4")
))]

If both features are enabled it always prints that deprecation warning (which is the only compile-time-warning I know how to print), and removes all the exports. Any code that actually tries to use this crate will fail because of that, but it lets us pass our CI.

@ethowitz For some reason I though that the feature powerset would fail, but maybe that was from before I implemented that feature interlock above? I'm not sure. Since it seems to works, like you demonstrated, I agree that it's a good idea to use.