-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Skip cross-target cargo check unless the OS surface changed #8717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
35ed187
266c0ba
225ac5c
e95d40d
01d2f62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,9 @@ jobs: | |
| outputs: | ||
| # Flag that is raised when any rust code is changed. | ||
| rust_code: ${{ steps.check_rust_code.outputs.changed }} | ||
| # Cross-target cargo check is skipped unless the diff can change | ||
| # platform-specific compilation. Force with the run:cross-check label. | ||
| cross_target: ${{ steps.check_cross_target.outputs.changed }} | ||
| steps: | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
|
|
@@ -75,6 +78,90 @@ jobs: | |
| env: | ||
| MERGE_BASE: ${{ steps.merge_base.outputs.sha }} | ||
|
|
||
| - name: Check if cross-target compilation can change | ||
| id: check_cross_target | ||
| run: | | ||
| if [ "${FORCE_CROSS_TARGET}" = "true" ]; then | ||
| echo "changed=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
|
|
||
| if ! git diff --quiet "${MERGE_BASE}...HEAD" -- \ | ||
| ':Cargo.lock' \ | ||
| ':rust-toolchain.toml' \ | ||
| ':.cargo/**' \ | ||
| ':.github/workflows/ci.yaml' \ | ||
|
Comment on lines
+89
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For pull-request runs, this allowlist omits both the root and crate-level Useful? React with 👍 / 👎. |
||
| ':crates/host_env/**' \ | ||
| ':crates/stdlib/src/openssl/**' \ | ||
| ':crates/stdlib/src/ssl/**' \ | ||
| ':**/_signal.rs' \ | ||
| ':**/_testconsole.rs' \ | ||
| ':**/_thread.rs' \ | ||
| ':**/_winapi.rs' \ | ||
| ':**/_wmi.rs' \ | ||
| ':**/cert_store.rs' \ | ||
| ':**/crt_fd*.rs' \ | ||
| ':**/errno.rs' \ | ||
| ':**/faulthandler.rs' \ | ||
| ':**/fcntl.rs' \ | ||
| ':**/fileutils.rs' \ | ||
| ':**/grp.rs' \ | ||
| ':**/locale.rs' \ | ||
| ':**/mmap.rs' \ | ||
| ':**/msvcrt.rs' \ | ||
| ':**/multiprocessing.rs' \ | ||
| ':**/native_certs.rs' \ | ||
| ':**/nt.rs' \ | ||
| ':**/openssl.rs' \ | ||
| ':**/os.rs' \ | ||
| ':**/overlapped.rs' \ | ||
| ':**/posix*.rs' \ | ||
| ':**/pwd.rs' \ | ||
| ':**/resource.rs' \ | ||
| ':**/scproxy.rs' \ | ||
| ':**/select.rs' \ | ||
| ':**/shm.rs' \ | ||
| ':**/signal.rs' \ | ||
| ':**/socket.rs' \ | ||
| ':**/ssl.rs' \ | ||
| ':**/syslog.rs' \ | ||
| ':**/thread.rs' \ | ||
| ':**/time.rs' \ | ||
| ':**/termios.rs' \ | ||
| ':**/testconsole.rs' \ | ||
| ':**/tkinter.rs' \ | ||
| ':**/uuid.rs' \ | ||
| ':**/winapi.rs' \ | ||
| ':**/windows.rs' \ | ||
| ':**/winreg.rs' \ | ||
| ':**/winsound.rs' \ | ||
| ':**/wmi.rs' | ||
| then | ||
| echo "changed=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Platform cfg, or FFI crates that only exist on some targets. | ||
| platform_re='target_os|target_arch|target_family|target_env|target_vendor|target_pointer_width|target_endian|cfg\(windows\)|cfg\(unix\)|cfg\(wasi|not\(windows|not\(unix|\[target\.|\blibc::|\bnix::|windows_sys|winapi::|rustix::|std::os::' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Detect nested and attributed platform configuration. The regular expression only detects direct forms such as Extend the detector for nested 🤖 Prompt for AI Agents |
||
|
|
||
| if ! git diff -G "${platform_re}" --quiet "${MERGE_BASE}...HEAD"; then | ||
| echo "changed=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
|
|
||
| while IFS= read -r file; do | ||
| [ -f "${file}" ] || continue | ||
| if grep -Eq "${platform_re}" "${file}"; then | ||
| echo "changed=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| done < <(git diff --name-only "${MERGE_BASE}...HEAD" -- '*.rs') | ||
|
|
||
| echo "changed=false" >> "$GITHUB_OUTPUT" | ||
| env: | ||
| MERGE_BASE: ${{ steps.merge_base.outputs.sha }} | ||
| FORCE_CROSS_TARGET: ${{ contains(github.event.pull_request.labels.*.name, 'run:cross-check') }} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On pull requests, adding the advertised Useful? React with 👍 / 👎. |
||
|
|
||
| rust_tests: | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }} | ||
| env: | ||
|
|
@@ -217,7 +304,7 @@ jobs: | |
| if: | | ||
| ( | ||
| !contains(github.event.pull_request.labels.*.name, 'skip:ci') && | ||
| needs.determine_changes.outputs.rust_code == 'true' | ||
| needs.determine_changes.outputs.cross_target == 'true' | ||
| ) || github.ref == 'refs/heads/main' | ||
| strategy: | ||
| matrix: | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include all Cargo manifests in cross-target detection.
A target-specific dependency or feature can change in
Cargo.tomlwithout changingCargo.lock. If the changed line does not contain the target table header, this detector reportschanged=falseand skips the cross-target matrix.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents