Merge rust-bitcoin/rust-bitcoin#3003: ci: semver-checks for non-additive cargo features

f76f68217b ci: pin cargo-semver-checks version and cron job (Jose Storopoli) 8a91015769 ci: harden zip command with -n (Jose Storopoli) 4edd504cb6 ci: pin stable in semver-checks and updates weekly (Jose Storopoli) 1948995443 ci: semver-check for non-additive cargo features (Jose Storopoli) b5180732e6 io: add not_unwind_safe as PhantomData (Jose Storopoli) Pull request description: Closes #3001. Closes #3023. ## Summary Adds new CI Job that checks for semver breaks in `no-default-features` against `all-features`. A fail would represent that we are inserting non-additive cargo features. Credits to @Kixunil for the amazing idea. This PR does: 1. fix a non-additive feature in `bitcoin-io`; 1. move the current `semver-checks` to `semver-checks-pr` since it checks if the PR is introducing classical semver-checks; 1. adds a new `semver-checks-feature`; 1. Add `-n` to `unzip` in `semver-checks`; 1. pins `rustc` stable and updates in a cron job weekly on Fri; and 1. pins `cargo-semver-checks` to `0.33.0` and updates in a cron job weekly on Sat. ## Implementation notes We don't need nightly (and somehow it fails) but if we incorporate the ENV [`RUSTC_BOOTSTRAP=1` as hardcoded in the cargo-semver-checks codebase](50b93599df/src/rustdoc_cmd.rs (L110C17-L110C39)) and run `cargo` with the stable toolchain it works. To get the `cargo-semver-checks` version we use the [`crates.io` API](https://crates.io/api/v1/crates/cargo-semver-checks) along with some quite intelligible `jg` magic (NOTE: `jq` is available in GH's `ubuntu-latest` image). ACKs for top commit: Kixunil: ACK f76f68217b apoelstra: ACK f76f68217b At some point we should pull the stable-version to the root with the nightly-version and use it everywhere; but can be in a separate PR Tree-SHA512: 9949350fb7d14853ed9b24c43c1b8c28d1178cc623fd4bb1a71b40f0e32a56052596f40ab55b1c26d17f4f4538a7a321b34cae2a65c110ed1939cc5cf0fc55ce
2024-07-28 17:46:39 +00:00 · 2024-07-28 17:46:39 +00:00 · 0c5b1a6804
parent 9868d9aaec f76f68217b
commit 0c5b1a6804
9 changed files with 260 additions and 9 deletions
--- a/.github/workflows/cargo-semver-checks-version
+++ b/.github/workflows/cargo-semver-checks-version
@ -0,0 +1 @@
+0.33.0
--- a/.github/workflows/cron-weekly-update-cargo-semver-checks.yml
+++ b/.github/workflows/cron-weekly-update-cargo-semver-checks.yml
@ -0,0 +1,40 @@
+name: Update cargo-semver-checks
+on:
+  schedule:
+    - cron: "0 0 * * 6" # runs every Saturday at 00:00
+  workflow_dispatch: # allows manual triggering
+jobs:
+  format:
+    name: Update cargo-semver-checks
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Update semver-checks to use latest crates.io published version
+        run: |
+          set -x
+          # Grab the latest version of cargo semver-checks from crates.io
+          # that is not yanked.
+          LATEST_VERSION=$(curl --proto '=https' --tlsv1.3 -sSf https://crates.io/api/v1/crates/cargo-semver-checks/versions | jq -r 'first( .versions[] | select(.yanked =- false) ) | .num')
+          # Update the latest version in the reference file.
+          echo "${LATEST_VERSION}" > ./.github/workflows/cargo-semver-checks-version
+          echo "cargo_semver_checks_version=${LATEST_VERSION}" >> $GITHUB_ENV
+          # If somehow the latest version has not changed. In this case don't make an empty PR.
+          if ! git diff --exit-code > /dev/null; then
+              echo "Updated cargo-semver-checks. Opening PR."
+              echo "changes_made=true" >> $GITHUB_ENV
+          else
+              echo "Attempted to update cargo-semver-checks but the crates.io version did not change. Not opening any PR."
+              echo "changes_made=false" >> $GITHUB_ENV
+          fi
+      - name: Create Pull Request
+        if: env.changes_made == 'true'
+        uses: peter-evans/create-pull-request@v6
+        with:
+          token: ${{ secrets.APOELSTRA_CREATE_PR_TOKEN }}
+          author: Update cargo-semver-checks Bot <bot@example.com>
+          committer: Update cargo-semver-checks Bot <bot@example.com>
+          title: Automated weekly update to cargo-semver-checks (to ${{ env.cargo_semver_checks_version }})
+          body: |
+           Automated update to Github CI workflow `semver-checks.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
+          commit-message: Automated update to Github CI to cargo-semver-checks version-${{ env.cargo_semver_checks_version }}
+          branch: create-pull-request/weekly-cargo-semver-checks-update
--- a/.github/workflows/cron-weekly-update-stable.yml
+++ b/.github/workflows/cron-weekly-update-stable.yml
@ -0,0 +1,40 @@
+name: Update Stable rustc
+on:
+  schedule:
+    - cron: "0 0 * * 5" # runs every Friday at 00:00 (generally rust releases on Thursday)
+  workflow_dispatch: # allows manual triggering
+jobs:
+  format:
+    name: Update stable rustc
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: dtolnay/rust-toolchain@stable
+      - name: Update semver-checks.yml to use latest stable
+        run: |
+          set -x
+          # Extract the version of the compiler dtolnay/rust-toolchain gives us.
+          STABLE_VERSION=$(rustc --version | sed -ne 's/^rustc //p' | cut -d ' ' -f1)
+          # Update the stable version in the reference file.
+          echo "${STABLE_VERSION}" > ./.github/workflows/stable-version
+          echo "stable_version=${STABLE_VERSION}" >> $GITHUB_ENV
+          # If somehow the stable version has not changed. In this case don't make an empty PR.
+          if ! git diff --exit-code > /dev/null; then
+              echo "Updated stable. Opening PR."
+              echo "changes_made=true" >> $GITHUB_ENV
+          else
+              echo "Attempted to update stable but the latest-stable date did not change. Not opening any PR."
+              echo "changes_made=false" >> $GITHUB_ENV
+          fi
+      - name: Create Pull Request
+        if: env.changes_made == 'true'
+        uses: peter-evans/create-pull-request@v6
+        with:
+          token: ${{ secrets.APOELSTRA_CREATE_PR_TOKEN }}
+          author: Update Stable Rustc Bot <bot@example.com>
+          committer: Update Stable Rustc Bot <bot@example.com>
+          title: Automated weekly update to rustc stable (to ${{ env.stable_version }})
+          body: |
+           Automated update to Github CI workflow `semver-checks.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
+          commit-message: Automated update to Github CI to rustc stable-${{ env.stable_version }}
+          branch: create-pull-request/weekly-stable-update
--- a/.github/workflows/semver-checks-pr-label.yml
+++ b/.github/workflows/semver-checks-pr-label.yml
@ -50,7 +50,7 @@ jobs:
            fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/semver-break.zip`, Buffer.from(download.data));
      - name: "Unzip artifact"
        if: ${{ hashFiles('semver-break.zip') != '' }}
-        run: unzip semver-break.zip
+        run: unzip -n semver-break.zip
      - name: "Comment and add label on PR - Semver break"
        uses: actions/github-script@v7
        if: ${{ hashFiles('semver-break') != '' }}
--- a/.github/workflows/semver-checks.yml
+++ b/.github/workflows/semver-checks.yml
@ -4,8 +4,8 @@ on:   # yamllint disable-line rule:truthy
 name: Check semver breaks

 jobs:
-  API:
-    name: API - stable toolchain
+  PR:
+    name: PR Semver - stable toolchain
    runs-on: ubuntu-latest
    strategy:
      fail-fast: false
@ -16,12 +16,15 @@ jobs:
          fetch-depth: 0 # we need full history for cargo semver-checks
      - name: "Install Rustup"
        uses: dtolnay/rust-toolchain@stable
+      - name: "Select stable-version"
+        run: |
+          rustup default $(cat ./.github/workflows/stable-version)
      - name: "Install cargo-binstall"
        uses: cargo-bins/cargo-binstall@main
-      - name: "Binstall cargo-semver-checks"
-        run: cargo binstall cargo-semver-checks --no-confirm
+      - name: "Binstall pinned cargo-semver-checks"
+        run: cargo binstall cargo-semver-checks@$(cat ./.github/workflows/cargo-semver-checks-version) --no-confirm
      - name: "Run semver checker script"
-        run: ./contrib/check-semver.sh
+        run: ./contrib/check-semver-pr.sh
      - name: Save PR number
        if: ${{ hashFiles('semver-break') != '' }}
        env:
@ -39,3 +42,24 @@ jobs:
        with:
          name: semver-break
          path: semver-break
+
+  Feature:
+    name: Non additive cargo features - stable toolchain
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+    steps:
+      - name: "Checkout repo"
+        uses: actions/checkout@v4
+      - name: "Install Rustup"
+        uses: dtolnay/rust-toolchain@stable
+      - name: "Select stable-version"
+        run: |
+          rustup default $(cat ./.github/workflows/stable-version)
+      - name: "Install cargo-binstall"
+        uses: cargo-bins/cargo-binstall@main
+      - name: "Binstall pinned cargo-semver-checks"
+        run: cargo binstall cargo-semver-checks@$(cat ./.github/workflows/cargo-semver-checks-version) --no-confirm
+      - name: "Run semver checker script"
+        run: ./contrib/check-semver-feature.sh
+
--- a/.github/workflows/stable-version
+++ b/.github/workflows/stable-version
@ -0,0 +1 @@
+1.79.0
--- a/contrib/check-semver-feature.sh
+++ b/contrib/check-semver-feature.sh
@ -0,0 +1,136 @@
+#!/usr/bin/env bash
+#
+# Checks semver compatibility between the `--no-features` and `all-features`.
+# This is important since it tests for the presence non-additive cargo features.
+#
+# Under the hood uses cargo semver-checks to check for breaking changes.
+# We cannot use it directly since it only supports checking against published
+# crates.
+# That's the intended use case for cargo semver-checks:
+# you run before publishing a new version of a crate to check semver breaks.
+# Here we are hacking it by first generating JSON files from cargo doc
+# and then using those files to check for breaking changes with
+# cargo semver-checks.
+
+set -euo pipefail
+
+# These are the hardcoded flags that cargo semver-checks uses
+# under the hood to invoke rustdoc.
+RUSTDOCFLAGS="-Z unstable-options --document-private-items --document-hidden-items --output-format=json --cap-lints=allow"
+
+main() {
+    # Generate JSON files for no-features and all-features
+    # 1. bitcoin
+    generate_json_files_all_features "bitcoin"
+    generate_json_files_no_default_features "bitcoin"
+
+    # 2. base58ck
+    generate_json_files_all_features "base58ck"
+    generate_json_files_no_default_features "base58ck"
+
+    # 3. bitcoin_hashes
+    generate_json_files_all_features "bitcoin_hashes"
+    generate_json_files_no_default_features "bitcoin_hashes"
+
+    # 4. bitcoin-units
+    generate_json_files_all_features "bitcoin-units"
+    generate_json_files_no_default_features "bitcoin-units"
+
+    # 5. bitcoin-io
+    generate_json_files_all_features "bitcoin-io"
+    generate_json_files_no_default_features "bitcoin-io"
+
+    # Check for API semver non-addivite cargo features on all the generated JSON files above.
+    run_cargo_semver_check "bitcoin"
+    run_cargo_semver_check "base58ck"
+    run_cargo_semver_check "bitcoin_hashes"
+    run_cargo_semver_check "bitcoin-units"
+    run_cargo_semver_check "bitcoin-io"
+
+    # Invoke cargo semver-checks to check for non-additive cargo features
+    # in all generated files.
+    check_for_non_additive_cargo_features
+}
+
+# Run cargo doc with the cargo semver-checks rustdoc flags.
+# We don't care about dependencies.
+run_cargo_doc() {
+    RUSTDOCFLAGS="$RUSTDOCFLAGS" RUSTC_BOOTSTRAP=1 cargo doc --no-deps "$@"
+}
+
+# Run cargo semver-check
+run_cargo_semver_check() {
+    local crate="$1"
+
+    echo "Running cargo semver-checks for $crate"
+    # Hack to not fail on errors.
+    # This is necessary since cargo semver-checks will fail if the
+    # semver check fails.
+    # We check that manually later.
+    set +e
+    cargo semver-checks -v --baseline-rustdoc "$crate-no-default-features.json" --current-rustdoc "$crate-all-features.json" > "$crate--additive-features.txt" 2>&1
+    set -e
+}
+
+# The following function uses cargo doc to generate JSON files that
+# cargo semver-checks can use.
+# - no-default-features: generate JSON doc files with no default features.
+generate_json_files_no_default_features() {
+    local crate="$1"
+
+    echo "Running cargo doc no-default-features for $crate"
+    run_cargo_doc --no-default-features -p "$crate"
+
+    # replace _ for - in crate name.
+    # This is necessary since some crates have - in their name
+    # which will be converted to _ in the output file by cargo doc.
+    mv "target/doc/${crate//-/_}.json" "$crate-no-default-features.json"
+}
+# - all-features: generate JSON doc files with all features.
+generate_json_files_all_features() {
+    local crate="$1"
+
+    echo "Running cargo doc all-features for $crate"
+    run_cargo_doc --all-features -p "$crate"
+
+    # replace _ for - in crate name.
+    # This is necessary since some crates have - in their name
+    # which will be converted to _ in the output file by cargo doc.
+    mv -v "target/doc/${crate//-/_}.json" "$crate-all-features.json"
+}
+
+# Check if there are non-additive cargo features.
+# We loop through all the generated files and check if there is a FAIL
+# in the cargo semver-checks output.
+# If we detect a fail, we create an empty file non-additive-cargo.
+# If the following CI step finds this file, it will add:
+# 1. a comment on the PR.
+# 2. a label to the PR.
+check_for_non_additive_cargo_features() {
+    for file in *additive-features.txt; do
+        echo "Checking $file"
+        if grep -q "FAIL" "$file"; then
+            echo "You have introduced non-additive cargo features"
+            echo "FAIL found in $file"
+            # flag it as a breaking change
+            # Handle the case where FAIL is found
+            touch non-additive-cargo
+        fi
+    done
+    if ! [ -f non-additive-cargo ]; then
+       echo "No non-additive cargo features found"
+    else
+        err "Non-additive cargo features found"
+    fi
+}
+
+err() {
+    echo "$1" >&2
+    exit 1
+}
+
+#
+# Main script
+#
+main "$@"
+exit 0
--- a/contrib/check-semver-pr.sh
+++ b/contrib/check-semver-pr.sh
--- a/io/src/error.rs
+++ b/io/src/error.rs
@ -6,6 +6,10 @@ use core::fmt;
 #[derive(Debug)]
 pub struct Error {
    kind: ErrorKind,
+    /// Indicates that the `struct` can pretend to own a mutable static reference
+    /// and an [`UnsafeCell`](core::cell::UnsafeCell), which are not unwind safe.
+    /// This is so that it does not introduce non-additive cargo features.
+    _not_unwind_safe: core::marker::PhantomData<(&'static mut (), core::cell::UnsafeCell<()>)>,

    #[cfg(feature = "std")]
    error: Option<Box<dyn std::error::Error + Send + Sync + 'static>>,
@ -20,13 +24,13 @@ impl Error {
    where
        E: Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
    {
-        Self { kind, error: Some(error.into()) }
+        Self { kind, _not_unwind_safe: core::marker::PhantomData, error: Some(error.into()) }
    }

    /// Creates a new I/O error.
    #[cfg(all(feature = "alloc", not(feature = "std")))]
    pub fn new<E: sealed::IntoBoxDynDebug>(kind: ErrorKind, error: E) -> Error {
-        Self { kind, error: Some(error.into()) }
+        Self { kind, _not_unwind_safe: core::marker::PhantomData, error: Some(error.into()) }
    }

    /// Returns the error kind for this error.
@ -49,6 +53,7 @@ impl From<ErrorKind> for Error {
    fn from(kind: ErrorKind) -> Error {
        Self {
            kind,
+            _not_unwind_safe: core::marker::PhantomData,
            #[cfg(any(feature = "std", feature = "alloc"))]
            error: None,
        }
@ -89,7 +94,11 @@ impl std::error::Error for Error {
 #[cfg(feature = "std")]
 impl From<std::io::Error> for Error {
    fn from(o: std::io::Error) -> Error {
-        Self { kind: ErrorKind::from_std(o.kind()), error: o.into_inner() }
+        Self {
+            kind: ErrorKind::from_std(o.kind()),
+            _not_unwind_safe: core::marker::PhantomData,
+            error: o.into_inner(),
+        }
    }
 }