Skip to content

Conversation

@lupyuen
Copy link
Member

@lupyuen lupyuen commented Oct 6, 2024

Summary

This PR continues to enhance the CI Workflow, to skip the unnecessary NuttX Builds. The changes in this PR will not take effect until the next PR, which will switch build.yml to use the rules in this PR.

In this PR, we refactor the CI Build Rules into a separate Reusable Workflow arch.yml. The original rules were migrated to arch.yml:

  • We target only the Simple PRs: One Arch Label + One Size Label (e.g. "Arch: risc-v, Size: XS")
  • For "Arch: risc-v": Build risc-v-01, risc-v-02
  • For "Arch: xtensa": Build xtensa-01, xtensa-02
  • The above rules apply when the PR is Created or Modified
  • When the PR is Merged: All targets shall be built

New and Updated Rules:

  • For "Arch: arm": Build arm-01, arm-02, ...
  • For "Arch: arm64": Build other
  • For "Arch: simulator": Build sim-01, sim-02
  • For "Arch: x86_64": Build other
  • For Simple PRs (One Arch Label + One Size Label): Skip the macOS and Windows builds (macos, macos/sim-*, msys2) since these builds are costly and slow
  • Except for "Arch: Simulator", which will enable the macOS Builds for sim-01 and sim-02
  • If GitHub CLI Fails: Build all targets

The code is explained here: #13775

Impact

The changes in this PR will not take effect until the next PR, which will switch build.yml to use the rules in this PR.

Testing

We tested with the upcoming version of build.yml. When we create a PR, the irrelevant builds are totally omitted. (Merge PR will compile all targets, same for Others)

Previously the irrelevant builds were skipped in seconds. Now the irrelevant builds (e.g. arm-01) are totally omitted for Simple PRs (e.g. "Arch: risc-v")

Screenshot 2024-10-06 at 11 39 43 AM

@github-actions github-actions bot added Area: CI Size: M The size of the change in this PR is medium labels Oct 6, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 7, 2024

[Experimental Bot, please feedback here]

The PR summary is well-written and informative, but there are a few missing pieces to fully meet the NuttX requirements. Here's a breakdown:

Strengths:

  • Clear explanation of the change: The summary clearly states the purpose of the PR, which is to enhance CI workflow efficiency by skipping unnecessary NuttX builds.
  • Technical details provided: It explains how the refactor works, including the new rules and their impact on different architectures.
  • Links to related resources: It links to the relevant issue for further context and the upcoming version of build.yml for testing validation.
  • Comprehensive testing: The testing section lists various scenarios and provides links to the actual test runs. This demonstrates thoroughness in validating the changes.

Areas for Improvement:

  • Impact on User: While the summary mentions "The changes in this PR will not take effect until the next PR," it's important to explicitly state "NO" under the Impact section for "Impact on user" to confirm there will be no user-facing changes.
  • Impact on Build: Similarly, even though the changes are not yet active, the impact on the build process is significant. The PR description should clearly state "YES" under "Impact on build" and elaborate on how the build process changes with this refactor.
  • Testing Logs: The template requests testing logs before and after the change. While links to test runs are provided, including snippets of relevant log output showcasing the change in behavior would be beneficial.

Concise Feedback:

This PR is well on its way to meeting the NuttX requirements. The summary is clear and informative, and the testing is comprehensive. However, it needs to explicitly address the impact on the build process and include snippets of relevant testing logs to be fully compliant.

@xiaoxiang781216 xiaoxiang781216 linked an issue Oct 7, 2024 that may be closed by this pull request
1 task
This PR continues to enhance the CI Workflow, to skip the unnecessary NuttX Builds. The changes in this PR will not take effect until the next PR, which will switch `build.yml` to use the rules in this PR.

In this PR, we refactor the CI Build Rules into a separate Reusable Workflow `arch.yml`. The original rules were migrated to `arch.yml`:
- We target only the Simple PRs: One Arch Label + One Size Label (e.g. "Arch: risc-v, Size: XS")
- For "Arch: risc-v": Build `risc-v-01`, `risc-v-02`
- For "Arch: xtensa": Build `xtensa-01`, `xtensa-02`
- The above rules apply when the PR is Created or Modified
- When the PR is Merged: All targets shall be built

New and Updated Rules:
- For "Arch: arm": Build `arm-01`, `arm-02`, ...
- For "Arch: arm64": Build `other`
- For "Arch: simulator": Build `sim-01`, `sim-02`
- For "Arch: x86_64": Build `other`
- For Simple PRs (One Arch Label + One Size Label): Skip the macOS and Windows builds (`macos`, `macos/sim-*`, `msys2`) since these builds are costly and slow
- Except for "Arch: Simulator", which will enable the macOS Builds for `sim-01` and `sim-02`
- If GitHub CLI Fails: Build all targets

The code is explained here: apache#13775
@xiaoxiang781216 xiaoxiang781216 merged commit 3173746 into apache:master Oct 7, 2024
lupyuen added a commit to lupyuen2/wip-nuttx-apps that referenced this pull request Oct 8, 2024
This PR syncs the Build Rules from `nuttx` to `nuttx-apps` repo. The Build Rules were added to `nuttx` repo here: apache/nuttx#13858
xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request Oct 8, 2024
This PR syncs the Build Rules from `nuttx` to `nuttx-apps` repo. The Build Rules were added to `nuttx` repo here: apache/nuttx#13858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CI Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Enhance the CI Workflow to skip the Unmodified Architectures

3 participants