Skip to content

Conversation

@t0mdavid-m
Copy link
Member

@t0mdavid-m t0mdavid-m commented Sep 19, 2025

Summary by CodeRabbit

  • Chores
    • Updated Windows CI environment to a newer, more stable runner to modernize the build platform.
    • Improved build pipeline robustness with clearer diagnostics and reporting for environment/tool issues.
    • Refined test/run handling so intermittent tool failures are surfaced clearly and non-critical failures are treated as soft errors.
    • No changes to application functionality or user experience.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

CI Windows workflow runner changed from windows-latest to windows-2022; no public or exported API/declaration changes.

Changes

Cohort / File(s) Summary
Windows CI workflow
.github/workflows/build-windows-executable-app.yaml
Update runner image: jobs build-openms and build-executable use windows-2022 instead of windows-latest. No changes to public/exported declarations.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

I hop across the CI logs at dawn,
swapped runners while the dew was on the lawn.
A tiny change, no APIs re-arranged,
The build keeps humming — just the host was changed.
🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "switch to powershell" is related to Windows/CI work but does not accurately describe the primary change shown in the diff; the changes update the Windows CI workflow to run jobs on windows-2022 instead of windows-latest and the provided summary shows no explicit PowerShell configuration change. The title therefore only partially matches the changeset rather than summarizing the main edit. Under the guideline that partially related titles can pass, this title is acceptable but not ideal for clarity.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_windows_build

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b935e15 and 6d3772e.

📒 Files selected for processing (1)
  • .github/workflows/build-windows-executable-app.yaml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/build-windows-executable-app.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-openms
  • GitHub Check: build-full-app

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
.github/workflows/build-windows-executable-app.yaml (3)

53-53: Pinning to windows-2022 is good; align other Windows jobs for consistency.

Mixing windows-2022 here with windows-latest in build-executable risks toolchain drift. Recommend pinning both to windows-2022 (or both to windows-latest) to avoid subtle MSVC/SDK differences.

Minimal change for build-executable:

-  build-executable:
-    runs-on: windows-latest
+  build-executable:
+    runs-on: windows-2022

62-62: Update checkout to v4 and consider shallow/sparse settings.

actions/checkout@v3 still works, but v4 brings perf and security fixes. Also consider fetch-depth: 1 to speed up and reduce CI bandwidth.

-    - name: Checkout
-      uses: actions/checkout@v3
+    - name: Checkout
+      uses: actions/checkout@v4
       with:
         repository: t0mdavid-m/OpenMS
         ref: FVdeploy
         path: 'OpenMS'
+        fetch-depth: 1

151-170: Harden the bash step and remove ambiguity around ccache.

  • Add set -Eeuo pipefail so failures are caught early.
  • Prefer command -v over which (builtin, more portable).
  • You export CMAKE_CCACHE_EXE to an absolute path but the step env later sets it to "ccache". Unify to the absolute path to avoid confusion and ensure CMake picks the intended binary.

Apply within this step:

       run: |
+          set -Eeuo pipefail
           # Ensure cmake/ctest and choco tools are visible in bash
           export PATH="/c/Program Files/CMake/bin:/c/ProgramData/chocolatey/bin:$PATH"

           mkdir -p "$GITHUB_WORKSPACE/OpenMS/bld/"
           # Print paths for debugging (somehting here frequently fails due to changes in runners)
-          which cmake || true
-          which ctest || true
-          which ninja || true
-          which ccache || true
+          command -v cmake || true
+          command -v ctest || true
+          command -v ninja || true
+          command -v ccache || true

           # Give CMake the absolute ccache path to avoid lookup glitches
           export CMAKE_CCACHE_EXE="C:/ProgramData/chocolatey/bin/ccache.exe"

           # Run the dashboard but don't fail the step on a spurious 127
           ctest --output-on-failure -V -S "$GITHUB_WORKSPACE/OpenMS/tools/ci/cibuild.cmake" || {
             status=$?
             echo "ctest exited with $status (treating as soft failure because dashboard already uploaded)."
             # Treat only 'command not found' (127) as ignorable; rethrow others
             if [ "$status" -ne 127 ]; then exit "$status"; fi
           }

And mirror the absolute path in the step env (below) so there’s no contradiction later in the step or in sub-processes:

# In the same step's env: block
CMAKE_CCACHE_EXE: "C:/ProgramData/chocolatey/bin/ccache.exe"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bb412b and b935e15.

📒 Files selected for processing (1)
  • .github/workflows/build-windows-executable-app.yaml (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-openms
  • GitHub Check: build-full-app

@t0mdavid-m t0mdavid-m merged commit e7cff7a into develop Sep 22, 2025
5 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 5, 2025
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.

2 participants