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: Improved speed to build nuttx on MSYS2 job #16072

Merged
merged 1 commit into from
Mar 26, 2025

Conversation

simbit18
Copy link
Contributor

@simbit18 simbit18 commented Mar 25, 2025

Summary

After MSYS2 updated the package on GitHub
there was a slowdown in building NuttX in the MSYS2 job.

It went from an average of 6 minutes to 10 minutes !!!

The purpose of this PR is to speed up the build to decrease the usage of the Windows runner.

Modified Files
tools/ci/cibuild.sh

tools/sethost.sh

make olddefconfig -j 4 -> make olddefconfig
workaround for remove

  Cleaning...
  Configuring...
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule. 
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.

before the change
2025022211

https://github.com/NuttX/nuttx/actions/runs/13472281038/job/37647297902#logs

after the change
2025022219
https://github.com/NuttX/nuttx/actions/runs/13475897767/job/37655044076#logs

Impact

Impact on user: No changes to user-facing functionality
Impact on build: Build process remains the same

Testing

We tested on the local platform MSYS2 and GitHub

20250325

(https://github.com/simbit18/nuttx_test_pr/actions/runs/14058716583/job/39364107803#logs)

After MSYS2 updated the package on GitHub
there was a slowdown in building NuttX in the MSYS2 job.

It went from an average of 6 minutes to 10 minutes !!!

The purpose of this PR is to speed up the build to decrease the usage of the Windows runner.

Modified Files
tools/ci/cibuild.sh

tools/sethost.sh

make olddefconfig -j 4 -> make olddefconfig
workaround for remove
  Cleaning...
  Configuring...
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.

Signed-off-by: simbit18 <[email protected]>
@github-actions github-actions bot added Area: Tooling Area: CI Size: S The size of the change in this PR is small labels Mar 25, 2025
@nuttxpr
Copy link

nuttxpr commented Mar 25, 2025

[Experimental Bot, please feedback here]

This PR description does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details and clarity. Here's a breakdown of the issues and how to improve it:

Summary - Weaknesses:

  • Vague "Why": "Slowdown in building NuttX" is insufficient. Explain why the MSYS2 update caused the slowdown. Did a specific toolchain component change? Were dependencies altered? This helps reviewers understand the root cause and the rationale for your solution.
  • Unclear "How": Simply stating make olddefconfig -j 4 -> make olddefconfig doesn't explain how removing the -j flag addresses the underlying issue. The "workaround for remove" comment is confusing and doesn't explain what was removed or why.
  • Missing Issue References: If this relates to a reported issue in either the NuttX or NuttX-apps repositories, provide links. This connects the PR to the broader context.

Summary - How to Improve:

  • Be specific about the cause of the slowdown. Example: "MSYS2's recent update to make version X.Y.Z introduced a performance regression when using parallel builds (-j) with NuttX."
  • Explain the rationale behind removing the -j flag. Example: "Removing the -j flag avoids triggering the performance regression in the updated make utility. While this sacrifices parallelism, it provides a significant net performance improvement in the MSYS2 environment."
  • Explain the "workaround for remove" more clearly. What was removed and why? This cryptic comment adds confusion. If it's about suppressing warnings, explain why those warnings appeared and why suppressing them is acceptable.
  • Include links to any related NuttX or NuttX-apps issues.

Impact - Weaknesses:

  • Too Brief: While stating "No changes" is sometimes accurate, provide some context even if minimal. For the build impact, instead of "Build process remains the same," consider saying: "The build process is functionally the same, except that parallel builds are disabled on MSYS2." This acknowledges the change while still confirming no significant user impact.
  • Missing Information: The PR modifies tools/ci/cibuild.sh and tools/sethost.sh, suggesting a change in the CI build process. This should be documented in the "Impact on build" section. Explain which CI builds are affected (e.g., MSYS2 builds only).

Impact - How to Improve:

  • Be more descriptive, even if the impact is minimal. Acknowledge the specific change to the build process (disabling parallel builds).
  • Explicitly state which CI builds are affected by the change.

Testing - Weaknesses:

  • Insufficient Logs: Screenshots of CI runs are not helpful for reviewers. Provide relevant snippets of the build logs before and after the change, demonstrating the performance improvement. Focus on the timing information.
  • Unclear Local Testing: "We tested on the local platform MSYS2 and GitHub" is ambiguous. Specify the specific MSYS2 environment used (OS version, MSYS2 version, toolchain versions). "GitHub" is not a testing platform; you tested on GitHub Actions. Be precise.
  • Missing Before/After Log Comparison: The requirement asks for "Testing logs before change" and "Testing logs after change." Provide actual log snippets, not just links to CI runs.

Testing - How to Improve:

  • Replace screenshots with relevant log excerpts showing build times before and after the change.
  • Clearly specify the local MSYS2 environment used for testing (OS, MSYS2 version, toolchain).
  • Clearly specify the GitHub Actions workflows used for testing.
  • Format the log snippets using code blocks (```).

By addressing these points, you'll create a much clearer and more complete PR description that meets the NuttX requirements and facilitates a smoother review process. Remember, the goal is to make it easy for reviewers to understand the problem, your solution, and its impact.

@xiaoxiang781216 xiaoxiang781216 merged commit d7681cc into apache:master Mar 26, 2025
39 checks passed
@simbit18 simbit18 deleted the simbit18-jobs branch March 28, 2025 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Tooling 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.

5 participants