OpenMS icon indicating copy to clipboard operation
OpenMS copied to clipboard
verified publisher icon OpenMS

Use external packages for extern when building deb

Open poshul opened this issue 1 week ago • 4 comments

address https://github.com/OpenMS/OpenMS/issues/8482

Summary by CodeRabbit

  • Chores
    • CI now conditionally runs Debian-specific dependency installation only when building .deb packages.
    • Debian builds install additional development packages to support optional system libraries.
    • Debian package dependencies updated to include an additional runtime library.
    • Build defaults adjusted so Debian packages prefer available system libraries when present.

✏️ Tip: You can customize this high-level summary in your review settings.

poshul avatar Dec 14 '25 13:12 poshul

Walkthrough

Adds a Debian-specific CI branch and script, appends libsqlite3-0 to Debian package dependencies, and makes three CMake external-library options default ON for Debian package builds and OFF otherwise.

Changes

Cohort / File(s) Summary
CI workflow conditional execution
\.github/workflows/openms_ci_matrix_full.yml``		 Invoke tools/ci/deps-ubuntu-deb.sh only when pkg_type equals "deb" instead of always calling the generic Ubuntu deps script.
Debian packaging dependencies
\cmake/package_deb.cmake``		 Append libsqlite3-0 to CPACK_DEBIAN_PACKAGE_DEPENDS.
CMake external library defaults
\src/openms/extern/CMakeLists.txt``		 Change defaults for USE_EXTERNAL_JSON, USE_EXTERNAL_SQLITECPP, and USE_EXTERNAL_SIMDE to be ON when PACKAGE_TYPE STREQUAL "deb", OFF otherwise (use computed defaults in option(...)).
New Debian-specific CI deps script
\tools/ci/deps-ubuntu-deb.sh``		 Add script that enables strict error handling and installs libsqlitecpp-dev, nlohmann-json3-dev, and libsimde-dev via apt-get for Debian-package CI runs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to:
    • The workflow conditional to ensure the Debian branch runs only for pkg_type == "deb".
    • Consistency between package names in cmake/package_deb.cmake and those installed by tools/ci/deps-ubuntu-deb.sh (noting libsqlite3-0 vs libsqlitecpp-dev).
    • PACKAGE_TYPE propagation so the computed CMake defaults are applied in CI and local builds.

Possibly related PRs

  • OpenMS/OpenMS#8037 — edits the same CPACK_DEBIAN_PACKAGE_DEPENDS declaration.
  • OpenMS/OpenMS#8042 — related CI dependency-install flow and complementary deps scripts.
  • OpenMS/OpenMS#8091 — related changes to USE_EXTERNAL_JSON CMake behavior and CI dependency handling.

Suggested reviewers

  • timosachsenberg

Poem

🐰 I hopped through CI with a careful paw,
A Debian branch to fix what I saw,
A tiny lib added, defaults flipped on cue,
Scripts install goodies so builds can run true,
hop hop 🥕

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 accurately summarizes the main change: using external packages for extern dependencies when building Debian packages, which aligns with the conditional defaults and new CI setup.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch fix_sql_lite_spw
📜 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 83e465e8db64559955decca2a7f23a86601e0613 and e679f28aa6170ee37031e293cccbdd2fe805d124.

📒 Files selected for processing (1)
  • cmake/package_deb.cmake (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings 
Learnt from: CR
Repo: OpenMS/OpenMS PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Be aware of OpenMS dependencies and external libraries
⏰ 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-and-test (ubuntu-24.04, g++, 13)
  • GitHub Check: build-and-test (windows-2025, cl.exe, 14.44)
🔇 Additional comments (1)
cmake/package_deb.cmake (1)

48-49: Good syntax fix. The libsqlite3-0 dependency is correct.

The syntax issue from the previous review has been properly addressed—all dependencies are now within a single quoted string, and you're using a runtime package instead of a dev package.

Regarding the SQLiteCpp dependency: since Debian's sqlitecpp packaging provides only a static library (libSQLiteCpp.a) via libsqlitecpp-dev with no separate runtime package, programs built against it require libsqlite3-0 (the SQLite shared library) as a direct runtime dependency. The current dependency on libsqlite3-0 is correct.

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.

coderabbitai[bot] avatar Dec 14 '25 13:12 coderabbitai[bot]

Coderabbit is probably right here. The package type is only set in the packaging step. Cmake will reconfigure. And probably rebuild the whole project since it's one of the earliest dependencies.

Actionable comments posted: 3

[!CAUTION] Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/openms/extern/CMakeLists.txt (1)

41-77: Ensure that PACKAGE_TYPE is set in the Build step's environment variables.

The conditional defaults for external library options in src/openms/extern/CMakeLists.txt depend on the PACKAGE_TYPE variable. However, PACKAGE_TYPE is only defined in the Package step's environment (workflow line 541), not in the Build step (lines 437-475). During the initial CMake configuration in cibuild.cmake, PACKAGE_TYPE defaults to "none" (from CMakeLists.txt), causing all three options to default to OFF:

  • USE_EXTERNAL_JSON_DEFAULT = OFF (mitigated by workflow env variable at line 475)
  • USE_EXTERNAL_SQLITECPP_DEFAULT = OFF (no env variable override)
  • USE_EXTERNAL_SIMDE_DEFAULT = OFF (no env variable override)

Add PACKAGE_TYPE: ${{ steps.set-vars.outputs.pkg_type }} to the Build step's environment variables (around line 475) so that the conditional defaults work as intended during the Build step's CMake configuration.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b5481c847d3fdd3c486b96d63e61203953ca41f6 and 95c91c70e51ca38eb2564c09220491cfd70272cb.

📒 Files selected for processing (4)
  • .github/workflows/openms_ci_matrix_full.yml (1 hunks)
  • cmake/package_deb.cmake (1 hunks)
  • src/openms/extern/CMakeLists.txt (3 hunks)
  • tools/ci/deps-ubuntu-deb.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings 
Learnt from: CR
Repo: OpenMS/OpenMS PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Be aware of OpenMS dependencies and external libraries
📚 Learning: 2025-08-05T12:43:11.681Z 
Learnt from: CR
Repo: OpenMS/OpenMS PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-05T12:43:11.681Z
Learning: Be aware of OpenMS dependencies and external libraries

Applied to files:

  • src/openms/extern/CMakeLists.txt
⏰ 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). (9)
  • GitHub Check: build-and-test (windows-2025, cl.exe, 14.44)
  • GitHub Check: build-and-test (ubuntu-24.04-arm, g++, 13)
  • GitHub Check: build-and-test (macos-14, xcode, 15.4)
  • GitHub Check: build-and-test (ubuntu-24.04, g++, 13)
  • GitHub Check: build-and-test (ubuntu-24.04, clang++, 17)
  • GitHub Check: build-win
  • GitHub Check: build-lnx
  • GitHub Check: build-macos-arm
  • GitHub Check: cppcheck-test

jpfeuffer avatar Dec 17 '25 01:12 jpfeuffer

PACKAGE_TYPE: ${{ steps.set-vars.outputs.pkg_type }}

There's a reason we removed that from the ENV for the build step, and IIRC it's because otherwise the tests don't get built.

poshul avatar Dec 17 '25 08:12 poshul

Hmm but building twice won't be the solution. It will build once with the internal SQLite (run the tests) and then again with the external.

If we don't/can't fix that we specify the package type at build time we probably have to separate the defaults from the package type and specify each "USE_EXTERNAL_.." manually during build time.

jpfeuffer avatar Dec 17 '25 12:12 jpfeuffer

closing in favor of https://github.com/OpenMS/OpenMS/pull/8496

poshul avatar Dec 18 '25 12:12 poshul