Skip to content

Incorporate feedback on pipelines#254

Open
zimmy87 wants to merge 7 commits intomainfrom
user/v-davidz/pipeline_feedback
Open

Incorporate feedback on pipelines#254
zimmy87 wants to merge 7 commits intomainfrom
user/v-davidz/pipeline_feedback

Conversation

@zimmy87
Copy link
Contributor

@zimmy87 zimmy87 commented Mar 10, 2026

Some of the logic in this repository's CI pipelines recently received a copilot code review in a context outside of this repository, and a portion of the feedback is applicable to this repository. This change applies that feedback.

Changes include:

  • xtask/src/build.rs
    • remove unsafe std::env::set_var() call and replace with safe xshell::Shell::set_var() call
  • xtask/src/clang_format.rs
    • remove explicit specification of "std::io::*" namespace due to already being imported
    • normalize path separators in is_excluded()
    • propagate clang-format exit status when --in-place is used
  • xtask/src/clippy.rs
    • always run clippy on workspace
  • xtask/src/coverage_report.rs
    • fix incorrect comment
  • xtask/src/nextest_report.rs
    • use append logic when writing to GITHUB_OUTPUT
  • xtask/src/precheck.rs
    • simplify subtask calls by adding more targeted imports
    • add extra clarification in comments
  • xtask/src/rustup_component_add.rs
    • fix incorrect comment

Copilot AI review requested due to automatic review settings March 10, 2026 19:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the xtask-based CI/pipeline helpers to incorporate review feedback, improving safety, portability, and consistency of various repo automation steps.

Changes:

  • Replace process-global env mutation in build with xshell::Shell::set_var.
  • Improve formatting/CI helpers: path normalization and exit-status handling in clang_format, always run clippy on the workspace, and append outputs for GitHub Actions report tasks.
  • Refactor precheck subtask invocation for simpler, more direct calls and clarify comments.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
xtask/src/build.rs Use Shell::set_var for RUSTFLAGS instead of std::env::set_var.
xtask/src/clang_format.rs Use imported stdout(), normalize separators in excludes, and surface clang-format failures in --in-place mode.
xtask/src/clippy.rs Ensure clippy runs against the full workspace consistently.
xtask/src/coverage_report.rs Correct module-level doc comment to match JSON-based coverage input.
xtask/src/nextest_report.rs Append to GITHUB_OUTPUT rather than overwriting it.
xtask/src/precheck.rs Use targeted imports and inline task construction + execution; add clarifying comments.
xtask/src/rustup_component_add.rs Fix struct field doc comment wording.

You can also share your feedback on Copilot code review. Take the survey.

@zimmy87 zimmy87 marked this pull request as ready for review March 10, 2026 21:08
Copilot AI review requested due to automatic review settings March 10, 2026 21:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.


You can also share your feedback on Copilot code review. Take the survey.

@zimmy87 zimmy87 requested a review from jaygmsft March 10, 2026 22:06
Copilot AI review requested due to automatic review settings March 12, 2026 17:14
@jaygmsft jaygmsft enabled auto-merge (squash) March 12, 2026 17:14
@jaygmsft jaygmsft disabled auto-merge March 12, 2026 17:15
@jaygmsft jaygmsft enabled auto-merge (squash) March 12, 2026 17:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.


You can also share your feedback on Copilot code review. Take the survey.


- **precheck**: Combines copyright, fmt, and clippy checks for comprehensive validation
- **precheck**: Combines setup, copyright, audit, fmt, clippy, and nextest stages for comprehensive validation
- **clippy**: Runs `cargo clippy --all-targets` with warnings treated as errors
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clippy implementation now always runs with --workspace, but this README line still documents the command as cargo clippy --all-targets. Please update the documentation to reflect the actual invocation (e.g., mention --workspace, or describe it in a way that remains accurate if flags change again).

Suggested change
- **clippy**: Runs `cargo clippy --all-targets` with warnings treated as errors
- **clippy**: Runs Clippy across the entire workspace (all targets) with warnings treated as errors

Copilot uses AI. Check for mistakes.

It auto fixes copyright issues. This ensures all source code has correct copyright headers.

## Precheckin Steps
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section header has a typo: "Precheckin" should be "Prechecking".

Suggested change
## Precheckin Steps
## Prechecking Steps

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants