fix leon & binstalk-downloader bug relating to features (#1153)

- ci: Check feat powerset of leon & binstalk-downloader in `ci.yml`
 - fix leon feature `cli`: Enable dep `miette` in feature `cli`
 - fix binstalk-downloader when default feature is disabled and no other
   tls related feature is enabled (breaking change due to replace of
   `tls::Version` with newtype `TLSVersion`).

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

.github/workflows/ci.yml

@@ -53,66 +53,46 @@ jobs:
       env:
         GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
 
-  linux-cross-check:
+  cross-check:
     strategy:
       fail-fast: false
       matrix:
-        target:
-        - armv7-unknown-linux-musleabihf
-        - armv7-unknown-linux-gnueabihf
-        - aarch64-unknown-linux-musl
-        - aarch64-unknown-linux-gnu
-        - x86_64-unknown-linux-musl
-
-    runs-on: ubuntu-latest
+        include:
+          - target: armv7-unknown-linux-musleabihf
+            os: ubuntu-latest
+          - target: armv7-unknown-linux-gnueabihf
+            os: ubuntu-latest
+          - target: aarch64-unknown-linux-musl
+            os: ubuntu-latest
+          - target: aarch64-unknown-linux-gnu
+            os: ubuntu-latest
+          - target: x86_64-unknown-linux-musl
+            os: ubuntu-latest
+          - target: aarch64-apple-darwin
+            os: macos-latest
+          - target: aarch64-pc-windows-msvc
+            os: windows-latest
+    runs-on: ${{ matrix.os }}
     env:
       CARGO_BUILD_TARGET: ${{ matrix.target }}
-      JUST_USE_CARGO_ZIGBUILD: true
-
     steps:
     - uses: actions/checkout@v3
     - uses: ./.github/actions/just-setup
+      with:
+        tools: cargo-hack
       env:
         # just-setup use binstall to install sccache,
         # which works better when we provide it with GITHUB_TOKEN.
         GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
 
+    - name: Enable cargo-zigbuild
+      if: matrix.os == 'ubuntu-latest'
+      run: echo JUST_USE_CARGO_ZIGBUILD=true >> "$GITHUB_ENV"
+
     - run: just ci-install-deps
     - run: just avoid-dev-deps
     - run: just check
 
-  apple-m1-check:
-    runs-on: macos-latest
-    env:
-      CARGO_BUILD_TARGET: aarch64-apple-darwin
-
-    steps:
-    - uses: actions/checkout@v3
-    - uses: ./.github/actions/just-setup
-      env:
-        # just-setup use binstall to install sccache,
-        # which works better when we provide it with GITHUB_TOKEN.
-        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-
-    - run: just avoid-dev-deps
-    - run: just check
-
-  windows-aarch64-check:
-    runs-on: windows-latest
-    env:
-      CARGO_BUILD_TARGET: aarch64-pc-windows-msvc
-
-    steps:
-    - uses: actions/checkout@v3
-    - uses: ./.github/actions/just-setup
-      env:
-        # just-setup use binstall to install sccache,
-        # which works better when we provide it with GITHUB_TOKEN.
-        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-
-    - run: just avoid-dev-deps
-    - run: just check
-
   lint:
     strategy:
       fail-fast: false
@@ -146,10 +126,8 @@ jobs:
     name: Tests pass
     needs:
     - test
-    - linux-cross-check
-    - apple-m1-check
+    - cross-check
     - lint
-    - windows-aarch64-check
     - release-builds
     if: always() # always run even if dependencies fail
     runs-on: ubuntu-latest

crates/bin/src/args.rs

@@ -8,7 +8,7 @@ use std::{
 };
 
 use binstalk::{
-    helpers::remote::tls::Version,
+    helpers::remote,
     manifests::cargo_toml_binstall::PkgFmt,
     ops::resolve::{CrateName, VersionReqExt},
 };
@@ -293,11 +293,11 @@ pub enum TLSVersion {
     Tls1_3,
 }
 
-impl From<TLSVersion> for Version {
+impl From<TLSVersion> for remote::TLSVersion {
     fn from(ver: TLSVersion) -> Self {
         match ver {
-            TLSVersion::Tls1_2 => Version::TLS_1_2,
-            TLSVersion::Tls1_3 => Version::TLS_1_3,
+            TLSVersion::Tls1_2 => remote::TLSVersion::TLS_1_2,
+            TLSVersion::Tls1_3 => remote::TLSVersion::TLS_1_3,
         }
     }
 }

crates/binstalk-downloader/Cargo.toml

@@ -60,7 +60,13 @@ pkg-config = ["zstd/pkg-config"]
 
 zlib-ng = ["flate2/zlib-ng"]
 
+# Dummy feature, enabled if rustls or native-tls is enabled.
+# Used to avoid compilation error when no feature is enabled.
+__tls = []
+
 rustls = [
+    "__tls",
+
     "reqwest/rustls-tls",
 
     # Enable the following features only if trust-dns-resolver is enabled.
@@ -69,7 +75,7 @@ rustls = [
     "trust-dns-resolver?/dns-over-https-rustls",
     "trust-dns-resolver?/dns-over-quic",
 ]
-native-tls = ["reqwest/native-tls", "trust-dns-resolver?/dns-over-native-tls"]
+native-tls = ["__tls", "reqwest/native-tls", "trust-dns-resolver?/dns-over-native-tls"]
 
 # Enable trust-dns-resolver so that features on it will also be enabled.
 trust-dns = ["trust-dns-resolver", "reqwest/trust-dns"]

crates/binstalk-downloader/src/remote.rs

@@ -16,7 +16,7 @@ use thiserror::Error as ThisError;
 use tower::{limit::rate::RateLimit, Service, ServiceBuilder, ServiceExt};
 use tracing::{debug, info};
 
-pub use reqwest::{header, tls, Error as ReqwestError, Method, StatusCode};
+pub use reqwest::{header, Error as ReqwestError, Method, StatusCode};
 pub use url::Url;
 
 mod delay_request;
@@ -28,6 +28,9 @@ pub use certificate::Certificate;
 mod request_builder;
 pub use request_builder::{Body, RequestBuilder, Response};
 
+mod tls_version;
+pub use tls_version::TLSVersion;
+
 #[cfg(feature = "json")]
 pub use request_builder::JsonError;
 
@@ -35,7 +38,7 @@ const MAX_RETRY_DURATION: Duration = Duration::from_secs(120);
 const MAX_RETRY_COUNT: u8 = 3;
 const DEFAULT_RETRY_DURATION_FOR_RATE_LIMIT: Duration = Duration::from_millis(200);
 const RETRY_DURATION_FOR_TIMEOUT: Duration = Duration::from_millis(200);
-const DEFAULT_MIN_TLS: tls::Version = tls::Version::TLS_1_2;
+const DEFAULT_MIN_TLS: TLSVersion = TLSVersion::TLS_1_2;
 
 #[derive(Debug, ThisError)]
 #[non_exhaustive]
@@ -69,6 +72,7 @@ struct Inner {
 #[derive(Clone, Debug)]
 pub struct Client(Arc<Inner>);
 
+#[cfg_attr(not(feature = "__tls"), allow(unused_variables, unused_mut))]
 impl Client {
     /// * `per` - must not be 0.
     /// * `num_request` - maximum number of requests to be processed for
@@ -77,30 +81,34 @@ impl Client {
     /// The Client created would use at least tls 1.2
     pub fn new(
         user_agent: impl AsRef<str>,
-        min_tls: Option<tls::Version>,
+        min_tls: Option<TLSVersion>,
         per: Duration,
         num_request: NonZeroU64,
         certificates: impl IntoIterator<Item = Certificate>,
     ) -> Result<Self, Error> {
         fn inner(
             user_agent: &str,
-            min_tls: Option<tls::Version>,
+            min_tls: Option<TLSVersion>,
             per: Duration,
             num_request: NonZeroU64,
             certificates: &mut dyn Iterator<Item = Certificate>,
         ) -> Result<Client, Error> {
-            let tls_ver = min_tls
-                .map(|tls| tls.max(DEFAULT_MIN_TLS))
-                .unwrap_or(DEFAULT_MIN_TLS);
-
             let mut builder = reqwest::ClientBuilder::new()
                 .user_agent(user_agent)
                 .https_only(true)
-                .min_tls_version(tls_ver)
                 .tcp_nodelay(false);
 
-            for certificate in certificates {
-                builder = builder.add_root_certificate(certificate.0);
+            #[cfg(feature = "__tls")]
+            {
+                let tls_ver = min_tls
+                    .map(|tls| tls.max(DEFAULT_MIN_TLS))
+                    .unwrap_or(DEFAULT_MIN_TLS);
+
+                builder = builder.min_tls_version(tls_ver.into());
+
+                for certificate in certificates {
+                    builder = builder.add_root_certificate(certificate.0);
+                }
             }
 
             let client = builder.build()?;

crates/binstalk-downloader/src/remote/certificate.rs

@@ -1,13 +1,19 @@
+#[cfg(feature = "__tls")]
 use reqwest::tls;
 
 use super::Error;
 
 #[derive(Clone, Debug)]
-pub struct Certificate(pub(super) tls::Certificate);
+pub struct Certificate(#[cfg(feature = "__tls")] pub(super) tls::Certificate);
 
+#[cfg_attr(not(feature = "__tls"), allow(unused_variables))]
 impl Certificate {
     /// Create a Certificate from a binary DER encoded certificate
     pub fn from_der(der: impl AsRef<[u8]>) -> Result<Self, Error> {
+        #[cfg(not(feature = "__tls"))]
+        return Ok(Self());
+
+        #[cfg(feature = "__tls")]
         tls::Certificate::from_der(der.as_ref())
             .map(Self)
             .map_err(Error::from)
@@ -15,6 +21,10 @@ impl Certificate {
 
     /// Create a Certificate from a PEM encoded certificate
     pub fn from_pem(pem: impl AsRef<[u8]>) -> Result<Self, Error> {
+        #[cfg(not(feature = "__tls"))]
+        return Ok(Self());
+
+        #[cfg(feature = "__tls")]
         tls::Certificate::from_pem(pem.as_ref())
             .map(Self)
             .map_err(Error::from)

crates/binstalk-downloader/src/remote/tls_version.rs

@@ -0,0 +1,37 @@
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+enum Inner {
+    Tls1_2 = 0,
+    Tls1_3 = 1,
+}
+
+/// TLS version for [`crate::remote::Client`].
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+pub struct TLSVersion(Inner);
+
+impl TLSVersion {
+    pub const TLS_1_2: TLSVersion = TLSVersion(Inner::Tls1_2);
+    pub const TLS_1_3: TLSVersion = TLSVersion(Inner::Tls1_3);
+}
+
+#[cfg(feature = "__tls")]
+impl From<TLSVersion> for reqwest::tls::Version {
+    fn from(ver: TLSVersion) -> reqwest::tls::Version {
+        use reqwest::tls::Version;
+        use Inner::*;
+
+        match ver.0 {
+            Tls1_2 => Version::TLS_1_2,
+            Tls1_3 => Version::TLS_1_3,
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_tls_version_order() {
+        assert!(TLSVersion::TLS_1_2 < TLSVersion::TLS_1_3);
+    }
+}

crates/leon/Cargo.toml

@@ -17,5 +17,5 @@ thiserror = "1.0.38"
 
 [features]
 default = ["miette"]
-cli = ["dep:clap", "miette?/fancy-no-backtrace"]
+cli = ["dep:clap", "miette", "miette?/fancy-no-backtrace"]
 miette = ["dep:miette"]

justfile

@@ -148,7 +148,8 @@ target-glibc-ver-postfix := if glibc-version != "" {
     ""
 }
 
-cargo-build-args := (if for-release != "" { " --release" } else { "" }) + (" --target ") + (target) + (target-glibc-ver-postfix) + (cargo-buildstd) + (if extra-build-args != "" { " " + extra-build-args } else { "" }) + (cargo-no-default-features) + (cargo-split-debuginfo) + (if cargo-features != "" { " --features " + cargo-features } else { "" }) + (win-arm64-ring16) + (if timings != "" { " --timings" } else { "" })
+cargo-check-args := (" --target ") + (target) + (target-glibc-ver-postfix) + (cargo-buildstd) + (if extra-build-args != "" { " " + extra-build-args } else { "" }) + (cargo-split-debuginfo) + (win-arm64-ring16)
+cargo-build-args := (if for-release != "" { " --release" } else { "" }) + (cargo-check-args) + (cargo-no-default-features) + (if cargo-features != "" { " --features " + cargo-features } else { "" }) + (if timings != "" { " --timings" } else { "" })
 export RUSTFLAGS := (linker-plugin-lto) + (rustc-gcclibs) + (rustc-miropt) + (rust-lld) + (rustc-icf)
 
 
@@ -181,6 +182,12 @@ build: print-env
 
 check: print-env
     {{cargo-bin}} check {{cargo-build-args}}
+    cargo-hack hack check --feature-powerset -p leon {{cargo-check-args}}
+    {{cargo-bin}} check -p binstalk-downloader --no-default-features
+    cargo-hack hack check -p binstalk-downloader \
+        --feature-powerset \
+        --include-features default,json,gh-api-client \
+        {{cargo-check-args}}
 
 get-output file outdir=".":
     test -d "{{outdir}}" || mkdir -p {{outdir}}