Add more robust debug_assertions verification to CI. (#751)

When a workspace uses `debug_assertions` for conditional compilation (*like Xilem does*), especially of the `#[cfg(debug_assertions)]` kind, it is not sufficient to just test with it either set to `true` or `false`. Both need testing as we don't know in advance which path contains faulty code. An initial response to this issue was done in #431 which added some `--release --all-targets` steps to the CI. This PR here improves that verification in some key ways: * In addition to the stable toolchain, also verify MSRV. * Only turn off the `debug_assertions` configuration option as that is the differentiator for our testing. The full `--release` option spends time on optimizations. It may be worth doing occasional `--release` tests to detect bugs in code optimization, but those checks aren't worth it as part of the `cargo clippy` / `cargo check` bundle we run most frequently. * No longer use `--all-targets` which has known issues as explained in the rationale section of the CI script. Instead we duplicate the regular steps with `cargo-hack`. * External dependencies are always built with `debug_assertions` set to `true` which allows for cache reuse. This is a trade-off. I think the cache reuse performance gains are worth it for regular CI runs. However some external dependency might e.g. sneak in a higher MSRV requirement into a `#[cfg(not(debug_assertions))]` block. Unlikely but possible. This is again where a less frequent `--release` check would help. Maybe scheduled, maybe just before publishing a new version. In any case, future work. Additionally there is a new env variable `USING_DEBUG_ASSERTIONS` and a presence verification script `debug_assertions.sh`. These in combination make the CI script generic across workspaces regardless of `debug_assertions` usage. The env variable dictates whether we will spend the time doing the extra compiling. The bash script will detect if the variable goes out of sync with actual usage in code.
linebender · Nov 22, 2024 · 91b2343 · 91b2343
1 parent d8d916c
commit 91b2343
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 14 deletions.
diff --git a/.github/debug_assertions.sh b/.github/debug_assertions.sh
@@ -0,0 +1,29 @@
+#!/bin/bash
+
+# Check all the standard Rust source files
+output=$(rg "debug_assertions" -g "*.rs" .)
+
+if [ -z "$output" ]; then
+	if [ "$USING_DEBUG_ASSERTIONS" = "true" ]; then
+		echo "Could not find any debug_assertions usage in Rust code."
+		echo "The CI script must be modified to not expect usage."
+		echo "Set USING_DEBUG_ASSERTIONS to false in .github/workflows/ci.yml."
+		exit 1
+	else
+		echo "Expected no debug_assertions usage in Rust code and found none."
+		exit 0
+	fi
+else
+	if [ "$USING_DEBUG_ASSERTIONS" = "true" ]; then
+		echo "Expected debug_assertions to be used in Rust code and found it."
+		exit 0
+	else
+		echo "Found debug_assertions usage in Rust code."
+		echo ""
+		echo $output
+		echo ""
+		echo "The CI script must be modified to expect this usage."
+		echo "Set USING_DEBUG_ASSERTIONS to true in .github/workflows/ci.yml."
+		exit 1
+	fi
+fi
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -16,6 +16,8 @@ env:
   NO_WASM_PKGS: "--exclude masonry --exclude xilem"
   # Only some of our examples support Android (primarily due to extra required boilerplate).
   ANDROID_TARGETS: "-p xilem --example mason_android --example calc_android --example stopwatch_android --example variable_clock_android --example http_cats_android --example to_do_mvc_android"
+  # Whether the workspace contains Rust code using the debug_assertions configuration option.
+  USING_DEBUG_ASSERTIONS: "true"
 
 
 # Rationale
@@ -43,6 +45,9 @@ env:
 # The MSRV jobs run only cargo check because different clippy versions can disagree on goals and
 # running tests introduces dev dependencies which may require a higher MSRV than the bare package.
 #
+# If the workspace uses debug_assertions then we verify code twice, with it set to true or false.
+# We always keep it true for external dependencies so that we can reuse the cache for faster builds.
+#
 # We don't save caches in the merge-group cases, because those caches will never be re-used (apart
 # from the very rare cases where there are multiple PRs in the merge queue).
 # This is because GitHub doesn't share caches between merge queues and the main branch.
@@ -83,6 +88,9 @@ jobs:
       - name: check copyright headers
         run: bash .github/copyright.sh
 
+      - name: check debug_assertions presence
+        run: bash .github/debug_assertions.sh
+
       - name: install cargo-rdme
         uses: taiki-e/install-action@v2
         with:
@@ -122,15 +130,22 @@ jobs:
           save-if: ${{ github.event_name != 'merge_group' }}
 
       - name: cargo clippy
-        run: cargo hack clippy --workspace --locked --optional-deps --each-feature -- -D warnings
+        run: cargo hack clippy --workspace --locked --profile ci --optional-deps --each-feature -- -D warnings
 
       - name: cargo clippy (auxiliary)
-        run: cargo hack clippy --workspace --locked --optional-deps --each-feature --tests --benches --examples -- -D warnings
+        run: cargo hack clippy --workspace --locked --profile ci --optional-deps --each-feature --tests --benches --examples -- -D warnings
+
+      - name: cargo clippy (no debug_assertions)
+        if: env.USING_DEBUG_ASSERTIONS == 'true'
+        run: cargo hack clippy --workspace --locked --profile ci --optional-deps --each-feature -- -D warnings
+        env:
+          CARGO_PROFILE_CI_DEBUG_ASSERTIONS: "false"
 
-      # Verify that we can build in release mode
-      # TODO: Find a way for this to share artifacts with the above job?
-      - name: cargo clippy (release)
-        run: cargo clippy --release --workspace --locked --all-targets -- -D warnings
+      - name: cargo clippy (auxiliary) (no debug_assertions)
+        if: env.USING_DEBUG_ASSERTIONS == 'true'
+        run: cargo hack clippy --workspace --locked --profile ci --optional-deps --each-feature --tests --benches --examples -- -D warnings
+        env:
+          CARGO_PROFILE_CI_DEBUG_ASSERTIONS: "false"
 
   clippy-stable-wasm:
     name: cargo clippy (wasm32)
@@ -156,15 +171,22 @@ jobs:
           save-if: ${{ github.event_name != 'merge_group' }}
 
       - name: cargo clippy
-        run: cargo hack clippy --workspace ${{ env.NO_WASM_PKGS }} --locked --target wasm32-unknown-unknown --optional-deps --each-feature -- -D warnings
+        run: cargo hack clippy --workspace ${{ env.NO_WASM_PKGS }} --locked --profile ci --target wasm32-unknown-unknown --optional-deps --each-feature -- -D warnings
 
       - name: cargo clippy (auxiliary)
-        run: cargo hack clippy --workspace ${{ env.NO_WASM_PKGS }} --locked --target wasm32-unknown-unknown --optional-deps --each-feature --tests --benches --examples -- -D warnings
+        run: cargo hack clippy --workspace ${{ env.NO_WASM_PKGS }} --locked --profile ci --target wasm32-unknown-unknown --optional-deps --each-feature --tests --benches --examples -- -D warnings
+
+      - name: cargo clippy (no debug_assertions)
+        if: env.USING_DEBUG_ASSERTIONS == 'true'
+        run: cargo hack clippy --workspace ${{ env.NO_WASM_PKGS }} --locked --profile ci --target wasm32-unknown-unknown --optional-deps --each-feature -- -D warnings
+        env:
+          CARGO_PROFILE_CI_DEBUG_ASSERTIONS: "false"
 
-      # Verify that we can build in release mode
-      # TODO: Find a way for this to share artifacts with the above job?
-      - name: cargo clippy (release)
-        run: cargo clippy --release --workspace ${{ env.NO_WASM_PKGS }} --locked --target wasm32-unknown-unknown --all-targets -- -D warnings
+      - name: cargo clippy (auxiliary) (no debug_assertions)
+        if: env.USING_DEBUG_ASSERTIONS == 'true'
+        run: cargo hack clippy --workspace ${{ env.NO_WASM_PKGS }} --locked --profile ci --target wasm32-unknown-unknown --optional-deps --each-feature --tests --benches --examples -- -D warnings
+        env:
+          CARGO_PROFILE_CI_DEBUG_ASSERTIONS: "false"
 
   prime-lfs-cache:
     name: Prime LFS Cache
@@ -344,7 +366,13 @@ jobs:
           save-if: ${{ github.event_name != 'merge_group' }}
 
       - name: cargo check
-        run: cargo hack check ${{ env.RUST_MIN_VER_PKGS }} --locked --optional-deps --each-feature
+        run: cargo hack check ${{ env.RUST_MIN_VER_PKGS }} --locked --profile ci --optional-deps --each-feature
+
+      - name: cargo check (no debug_assertions)
+        if: env.USING_DEBUG_ASSERTIONS == 'true'
+        run: cargo hack check ${{ env.RUST_MIN_VER_PKGS }} --locked --profile ci --optional-deps --each-feature
+        env:
+          CARGO_PROFILE_CI_DEBUG_ASSERTIONS: "false"
 
   check-msrv-wasm:
     name: cargo check (msrv) (wasm32)
@@ -369,7 +397,13 @@ jobs:
           save-if: ${{ github.event_name != 'merge_group' }}
 
       - name: cargo check
-        run: cargo hack check ${{ env.RUST_MIN_VER_PKGS }} ${{ env.NO_WASM_PKGS }} --locked --target wasm32-unknown-unknown --optional-deps --each-feature
+        run: cargo hack check ${{ env.RUST_MIN_VER_PKGS }} ${{ env.NO_WASM_PKGS }} --locked --profile ci --target wasm32-unknown-unknown --optional-deps --each-feature
+
+      - name: cargo check (no debug_assertions)
+        if: env.USING_DEBUG_ASSERTIONS == 'true'
+        run: cargo hack check ${{ env.RUST_MIN_VER_PKGS }} ${{ env.NO_WASM_PKGS }} --locked --profile ci --target wasm32-unknown-unknown --optional-deps --each-feature
+        env:
+          CARGO_PROFILE_CI_DEBUG_ASSERTIONS: "false"
 
   doc:
     name: cargo doc

diff --git a/Cargo.toml b/Cargo.toml
@@ -122,3 +122,8 @@ accesskit = "0.17.0"
 accesskit_winit = "0.23.0"
 nv-flip = "0.1.2"
 time = "0.3.36"
+
+[profile.ci]
+inherits = "dev"
+[profile.ci.package."*"]
+debug-assertions = true # Keep always on for dependencies for cache reuse.