Skip to content
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

CI: Validate all defconfig files before running any builds #14299

Closed
wants to merge 4 commits into from

Conversation

lupyuen
Copy link
Member

@lupyuen lupyuen commented Oct 15, 2024

Summary

Currently, CI Build Jobs will validate the defconfig file just before compiling the NuttX Target (like rv-virt:nsh). This means that the Build Job might run for a while, before hitting a defconfig error and failing much later.

This PR updates the CI Workflow build.yml to validate all defconfig files before running any builds. This means that errors in the defconfig files will be flagged earlier. And the Build Job will terminate (with an error) before any build begins.

This behaviour is helpful for resolving CI Build Issues quickly. The code is derived from [tools/testbuild.sh](http://tools/testbuild.sh). The enhancement was suggested here: #14259

Impact

The CI Workflow build.yml will validate all defconfig files before running any builds.

If any errors are found in defconfig files: The Build Job will terminate (with an error) after all defconfig files have been validated, before any build begins.

The Updated CI Workflow shall be synced to nuttx-apps repo in the next PR.

Testing

We tested by creating an intentional error in a defconfig file:

If defconfig validation fails: The Build Job terminates (with an error) after all defconfig files have been validated, before any build begins
https://github.com/lupyuen5/label-nuttx/actions/runs/11344293823/job/31548821624

Validating targets in tools/ci/testlist/x86_64-01.dat
./tools/refresh.sh --silent qemu-intel64/nsh
  Normalize qemu-intel64/nsh
8d7
< CONFIG_AAA=y
Saving the new configuration file
Error: qemu-intel64/nsh:1:1: error: qemu-intel64/nsh is configured incorrectly. To fix it, run "tools/refresh.sh qemu-intel64/nsh"
...
Error: tools/ci/testlist/x86_64-01.dat:1:1: error: Quitting, defconfig validation failed for tools/ci/testlist/x86_64-01.dat

Errors are specially formatted to display correctly in the GitHub Actions Job Summary:

Screenshot 2024-10-15 at 6 43 47 PM

If defconfig validation is successful: The Build Job continues to build the targets
https://github.com/lupyuen5/label-nuttx/actions/runs/11343543363/job/31546414579

Validating targets in tools/ci/testlist/x86_64-01.dat
./tools/refresh.sh --silent qemu-intel64/nsh
  Normalize qemu-intel64/nsh
...
Configuration/Tool: qemu-intel64/nsh
  Building NuttX...

@lupyuen lupyuen linked an issue Oct 15, 2024 that may be closed by this pull request
1 task
@github-actions github-actions bot added Area: CI Size: S The size of the change in this PR is small labels Oct 15, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 15, 2024

[Experimental Bot, please feedback here]

The PR you described seems to meet the NuttX requirements and provides valuable improvements to the CI process. Here's a breakdown:

Summary:

  • Clear explanation of the change: You clearly describe the issue (late defconfig validation) and the proposed solution (early validation).
  • Links to relevant resources: You link to the relevant NuttX issue and the code inspiration source.

Impact:

  • Comprehensive impact assessment: You cover all the essential points, including:
    • Feature change (CI behavior)
    • User impact (None)
    • Build impact (Earlier termination on defconfig errors)
    • Hardware impact (None)
    • Documentation impact (Potentially requires updating CI documentation)
    • Security impact (None)
    • Compatibility impact (None)
  • Clear explanation of future steps: You mention syncing the change to the nuttx-apps repo.

Testing:

  • Verification on local setup: You state that the changes are verified locally.
  • Detailed testing logs: You provide links to CI runs demonstrating:
    • Build failure with an intentional defconfig error
    • Successful build when defconfig validation passes

Suggestions for Improvement:

  • Explicitly mention documentation updates: While you mention potential documentation impact, it's helpful to be explicit about whether the PR includes documentation updates or if a separate PR is planned.
  • Consider adding a "How to Test" section: For more complex changes, it's beneficial to provide steps on how reviewers can test the changes locally.

Overall:

This PR effectively addresses a real problem in the CI workflow, improving the developer experience by providing faster feedback on defconfig errors. The PR is well-written, provides comprehensive information, and includes clear testing evidence.

@lupyuen lupyuen force-pushed the label13 branch 2 times, most recently from bb78641 to a06a691 Compare October 15, 2024 09:39
Currently, CI Build Jobs will validate the `defconfig` file just before compiling the NuttX Target (like `rv-virt:nsh`). This means that the Build Job might run for a while, before hitting a `defconfig` error and failing much later.

This PR updates the CI Workflow `build.yml` to validate all `defconfig` files before running any builds. This means that errors in the `defconfig` files will be flagged earlier. And the Build Job will terminate (with an error) before any build begins.

This behaviour is helpful for resolving CI Build Issues quickly. The code is derived from `tools/testbuild.sh`. The enhancement was suggested here: apache#14259
Currently, CI Build Jobs will validate the `defconfig` file just before compiling the NuttX Target (like `rv-virt:nsh`). This means that the Build Job might run for a while, before hitting a `defconfig` error and failing much later.

This PR updates the CI Workflow `build.yml` to validate all `defconfig` files before running any builds. This means that errors in the `defconfig` files will be flagged earlier. And the Build Job will terminate (with an error) before any build begins.

This behaviour is helpful for resolving CI Build Issues quickly. The code is derived from `tools/testbuild.sh`. The enhancement was suggested here: apache#14259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants