Fix Builds for OpenMS 3.5 #299
Conversation
📝 WalkthroughWalkthroughRemoved a Windows PyInstaller build workflow and updated the remaining Windows CI workflow and Dockerfile: OpenMS version/branch bumped to 3.5.0, Qt install and contrib/THIRDPARTY path handling adjusted, capture-env added before build, WITH_PARQUET disabled, and pip pins loosened in the Dockerfile. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (7)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-windows-executable-app-with-pyinstaller.yaml (1)
21-30: Remove the obsolete temporary workaround.The temporary workaround fetching CMake scripts from the
developbranch should be removed, as indicated by the comment referencing OpenMS release 3.4. Since this PR targets OpenMS 3.5.0, the workaround is no longer necessary. Additionally, the related workflow file.github/workflows/build-windows-executable-app.yamlremoves this same workaround block, creating an inconsistency between the two Windows build workflows.Apply this diff to remove the workaround:
- # Temporary fix - until seqan is back online or new OpenMS release (3.4) - - name: Get latest cibuild.cmake - working-directory: OpenMS - run: | - git config user.name "GitHub Actions" - git config user.email "actions@github.com" - git fetch origin develop - git checkout origin/develop -- tools/ci/cibuild.cmake - git checkout origin/develop -- tools/ci/citest.cmake - git checkout origin/develop -- tools/ci/cipackage.cmake -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-windows-executable-app-with-pyinstaller.yaml(1 hunks).github/workflows/build-windows-executable-app.yaml(2 hunks)Dockerfile(1 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-full-app
- GitHub Check: build-openms
🔇 Additional comments (3)
Dockerfile (1)
12-12: LGTM!The OpenMS branch update to 3.5.0 aligns with the Windows workflow changes and ensures consistency across the build pipeline.
.github/workflows/build-windows-executable-app.yaml (2)
16-16: LGTM!The OpenMS version bump to 3.5.0 is consistent with other workflow files and the Dockerfile.
126-126: Good addition for environment debugging.The capture-env.sh step provides valuable build environment logging, which aids in debugging and reproducibility. The placement before the ctest build command is appropriate.
| workflow_dispatch: | ||
| env: | ||
| OPENMS_VERSION: 3.2.0 | ||
| OPENMS_VERSION: 3.5.0 |
There was a problem hiding this comment.
OpenMS 3.5.0 is not an official release; use 3.4.0 instead.
The latest official OpenMS release is 3.4.0 (May 9, 2025). Update the version reference to match an official release.
🤖 Prompt for AI Agents
.github/workflows/build-windows-executable-app-with-pyinstaller.yaml around line
5: the workflow pins OPENMS_VERSION to 3.5.0 which is not an official release;
change the value to the latest official release 3.4.0 by updating the
OPENMS_VERSION variable to "3.4.0" so the workflow uses the supported OpenMS
release.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-windows-executable-app.yaml(5 hunks)Dockerfile(2 hunks)
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile
[error] 61-61: Ensure the shebang uses an absolute path to the interpreter.
(SC2239)
⏰ 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). (5)
- GitHub Check: build-openms
- GitHub Check: build-full-app
- GitHub Check: build
- GitHub Check: build-simple-app
- GitHub Check: test (ubuntu-latest, 3.11)
🔇 Additional comments (6)
.github/workflows/build-windows-executable-app.yaml (5)
16-16: LGTM!The OpenMS version bump to 3.5.0 correctly aligns with the PR objective and matches the Docker configuration changes.
136-136: Clarify the reason for disabling Parquet support.Setting
WITH_PARQUET: "OFF"disables Parquet file format support. This functional change is not mentioned in the PR description.Please clarify:
- Is this a temporary workaround for a build issue in OpenMS 3.5.0?
- Is Parquet support intentionally removed?
- Are there any downstream impacts on users who depend on Parquet functionality?
If this is a known issue or intentional change in OpenMS 3.5.0, consider documenting it in the PR description or commit message.
37-43: No issues identified. The Qt 6.8.3 upgrade meets OpenMS compatibility requirements and thewin64_msvc2022_64architecture aligns with the MSVC 2022 toolchain used in the workflow. The restricted archives and disabled caching are suitable for CI.
124-124: Verify the capture-env.sh script exists in OpenMS 3.5.0.The addition of
capture-env.shbeforecibuild.cmakeappears to be a new requirement in the OpenMS 3.5.0 build process. Ensuretools/ci/capture-env.shexists in the release/3.5.0 branch and is properly executable, as the workflow will fail if this script is missing.
112-112: Verify that the THIRDPARTY directory pathWindows/x86_64exists in OpenMS 3.5.0.The path change from
Windows/64bittoWindows/x86_64appears to be nonstandard. Official OpenMS documentation consistently referencesWindows/64bitfor 64-bit Windows builds. If this path does not exist in the OpenMS 3.5.0 THIRDPARTY submodule, the build workflow will fail.Dockerfile (1)
12-12: OpenMS 3.5.0 branch does not exist as a release.The specified branch
release/3.5.0is not available in the OpenMS repository. The latest release branch is 3.4.1. Update to an existing release branch (e.g.,release/3.4.1) or verify the intended version number.Likely an incorrect or invalid review comment.
| # Install up-to-date cmake via mamba and packages for pyOpenMS build. | ||
| RUN mamba install cmake | ||
| RUN pip install --upgrade pip && python -m pip install -U setuptools nose 'cython<3.1' 'autowrap<0.23' pandas numpy pytest | ||
| RUN pip install --upgrade pip && python -m pip install -U setuptools nose cython autowrap pandas numpy pytest |
There was a problem hiding this comment.
Consider pinning Python package versions for reproducible builds.
Removing version constraints for cython and autowrap makes the build non-reproducible and vulnerable to breaking changes in future package releases. This could lead to unexpected build failures.
Apply this diff to restore version pinning:
-RUN pip install --upgrade pip && python -m pip install -U setuptools nose cython autowrap pandas numpy pytest
+RUN pip install --upgrade pip && python -m pip install -U setuptools nose cython==<version> autowrap==<version> pandas numpy pytestReplace <version> with specific versions that are known to work with OpenMS 3.5.0.
Alternatively, if you intentionally removed the pins to avoid compatibility issues, please verify that the latest versions of these packages work correctly with pyOpenMS 3.5.0.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Hadolint (2.14.0)
[error] 61-61: Ensure the shebang uses an absolute path to the interpreter.
(SC2239)
🤖 Prompt for AI Agents
In Dockerfile around line 61, the RUN pip install command installs cython and
autowrap without pinned versions, making builds non-reproducible; update that
line to pin cython and autowrap to specific versions known to work with OpenMS
3.5.0 (e.g., replace cython and autowrap with cython==<version> and
autowrap==<version>), or explicitly document and verify compatibility if you
intend to keep them unpinned.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build-windows-executable-app.yaml (1)
142-142: Consider documenting why Parquet support is disabled.
WITH_PARQUEThas been set toOFF. If this is a temporary workaround for OpenMS 3.5.0 build issues, consider adding a comment explaining the reason and whether it should be re-enabled in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-windows-executable-app.yaml(6 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). (4)
- GitHub Check: build-openms
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: test (ubuntu-latest, 3.11)
🔇 Additional comments (4)
.github/workflows/build-windows-executable-app.yaml (4)
16-16: LGTM!The OpenMS version bump to 3.5.0 aligns with the PR objectives to fix builds for OpenMS 3.5.
91-96: LGTM!Adding the contrib library path to PATH for runtime DLL resolution is appropriate and correctly placed after the contrib is downloaded.
37-43: Qt 6.8.3 meets the Qt version requirement for OpenMS. OpenMS 3.4 switched to Qt 6 (>= 6.5), and Qt 6.8.3 satisfies this minimum version constraint. The architecturewin64_msvc2022_64is appropriate for Windows builds.
130-130: Verify the capture-env.sh script exists in OpenMS 3.5.0.The workflow now invokes
capture-env.shbefore the build step. Ensure this script exists in OpenMS 3.5.0 attools/ci/capture-env.shand accepts the-vflag with a CMakeCache.txt path argument.
| # use flat THIRDPARTY structure | ||
| mkdir -p _thirdparty | ||
| cp -R OpenMS/THIRDPARTY/Windows/64bit/* _thirdparty/ | ||
| cp -R OpenMS/THIRDPARTY/Windows/x86_64/* _thirdparty/ |
There was a problem hiding this comment.
The path change from Windows/64bit to Windows/x86_64 is unsupported by the actual OpenMS THIRDPARTY directory structure.
Official OpenMS documentation prescribes copying subdirectories to ./Windows/64bit/, not Windows/x86_64. The live THIRDPARTY repository uses the Windows/64bit directory structure. This path change will cause the workflow to fail when attempting to copy from a non-existent directory. Revert to Windows/64bit as specified in OpenMS's official build instructions.
🤖 Prompt for AI Agents
.github/workflows/build-windows-executable-app.yaml around line 118: the
workflow copies third-party binaries from OpenMS/THIRDPARTY/Windows/x86_64 which
does not exist in the upstream THIRDPARTY layout; change the path back to
OpenMS/THIRDPARTY/Windows/64bit so it matches the official OpenMS directory
structure and will succeed when copying those subdirectories during the build.
Summary by CodeRabbit
Chores
Removed
✏️ Tip: You can customize this high-level summary in your review settings.