refactor: replace QProcess/QObject with boost::process

[timosachsenberg]

Summary

• ExternalProcess no longer inherits QObject — no Q_OBJECT macro in core
• Uses boost::process for subprocess management with async_pipe for streaming output
• Adds idle_callback parameter so GUI can pump Qt event loop without core depending on Qt
• PythonInfo/JavaInfo/RWrapper rewritten with boost::process (sync, 30s timeout)
• TOPPBase runExternalProcess_ signatures changed from QString/QStringList to String/vector
• All ~11 TOPP adapter files updated
• DBSuitability and InternalCalibration updated
• ExternalProcessMBox passes idle_callback for QCoreApplication::processEvents()
• boost-process added to vcpkg.json (header-only, no cmake COMPONENTS needed)
• Part 4/7 of removing Qt6::Core from libOpenMS core library

Test plan

• ExternalProcess_test, PythonInfo_test, JavaInfo_test pass
• OpenMS, OpenMS_GUI, and TOPP targets all build
• 14 related tests pass

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor

  • External process execution rewritten: more reliable timeouts, clearer failure messages, and GUI remains responsive during long runs.
  • Command-line argument and string handling standardized across tools and GUI dialogs for more consistent behavior.

• Chores

  • Added Boost-based process support as part of the new execution infrastructure.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Replaces Qt-based process and string handling with Boost.Process/Boost.Asio and OpenMS String/std::vector across core system, TOPP, GUI, wrappers, and adapters; adds idle_callback for event-loop pumping and migrates async IO to Boost-based implementations.

Changes

Cohort / File(s)	Summary
Core process implementation src/openms/include/OpenMS/SYSTEM/ExternalProcess.h, src/openms/source/SYSTEM/ExternalProcess.cpp	Removed QObject/QProcess and Qt signal/slot machinery; migrated to Boost.Process/Boost.Asio; replaced QString/QStringList with String/std::vector<String>; added idle_callback parameter; removed QProcess private member.
TOPP public API src/openms/include/OpenMS/APPLICATIONS/TOPPBase.h, src/openms/source/APPLICATIONS/TOPPBase.cpp	Updated runExternalProcess_ overloads to use String, std::vector<String>, and std::map<String,String> instead of Qt types (signatures changed).
Language/runtime wrappers & checks src/openms/include/OpenMS/SYSTEM/RWrapper.h, src/openms/source/SYSTEM/RWrapper.cpp, src/openms/source/SYSTEM/JavaInfo.cpp, src/openms/source/SYSTEM/PythonInfo.cpp	Replaced QProcess with Boost.Process; changed RWrapper signatures to accept String/std::vector<String>; added timeouts, unified output capture and improved error handling.
GUI integration & helpers src/openms_gui/include/.../SwathTabWidget.h, .../WizardHelper.h, .../ExternalProcessMBox.h, src/openms_gui/source/VISUAL/MISC/ExternalProcessMBox.cpp	Switched QStringList usage/returns to std::vector<String>; ExternalProcessMBox updated to pass an idle_callback that pumps QCoreApplication::processEvents() and to show errors only when error_msg is non-empty.
GUI workflow callsites src/openms_gui/source/VISUAL/DIALOGS/SwathTabWidget.cpp, .../FLASHDeconvTabWidget.cpp, src/openms_gui/source/VISUAL/TVToolDiscovery.cpp	Converted external-tool invocations to String + std::vector<String> arguments; updated PyProphet/TRIC/OSW flows and tool callsites accordingly; changed SwathTabWidget output filenames return type to std::vector<String>.
TOPP adapters & tools src/topp/.../*.cpp (e.g., CometAdapter.cpp, FileConverter.cpp, LuciphorAdapter.cpp, MSFraggerAdapter.cpp, MSGFPlusAdapter.cpp, MaRaClusterAdapter.cpp, NovorAdapter.cpp, OpenNuXL.cpp, PercolatorAdapter.cpp, SageAdapter.cpp, SpectraSTSearchAdapter.cpp)	Replaced QString/QStringList with String/std::vector<String> for argument assembly and external calls; removed many .toQString() conversions; MSGFPlusAdapter replaced QLockFile with boost::interprocess::file_lock.
Internal processing callsites src/openms/source/PROCESSING/CALIBRATION/InternalCalibration.cpp, src/openms/source/QC/DBSuitability.cpp	Updated call sites to pass std::vector<String> and direct String values to RWrapper/ExternalProcess (removed Qt conversions).
Other system utilities src/openms/source/SYSTEM/RWrapper.cpp, src/openms/source/SYSTEM/JavaInfo.cpp, src/openms/source/SYSTEM/PythonInfo.cpp	Implemented Boost.Process-based executions with explicit timeouts and improved stderr/stdout capture; internal behavior changed from QProcess to Boost.
Build manifest & small portability vcpkg.json, src/openms/include/OpenMS/ANALYSIS/TOPDOWN/FLASHHelperClasses.h	Added boost-process dependency; added MSVC-specific uint typedef to aid compilation.

Sequence Diagram(s)

sequenceDiagram
    participant GUI as "GUI / ExternalProcessMBox"
    participant TOPP as "TOPPBase / Caller"
    participant EP as "ExternalProcess (Boost.Process)"
    participant Child as "Child process (R/Java/Python/Tool)"

    GUI->>TOPP: user triggers external run
    TOPP->>EP: run(exe: String, args: vector<String>, env, idle_callback)
    EP->>Child: launch via boost::process (start_dir, env)
    Child-->>EP: stdout / stderr (async pipes)
    EP->>EP: io_context poll loop (invoke idle_callback periodically)
    EP-->>TOPP: return EXIT / CRASH / NONZERO_EXIT + captured output
    TOPP-->>GUI: display results / error (only if error_msg non-empty)

Loading

Suggested reviewers

• jpfeuffer

Poem

🐇 I swapped Qt strings for vectors so fleet,
Boost pipes hum while processes speak,
I pump event loops with a twitch of my paw,
Std::vector hops in — no Qt to thaw,
A rabbit's small cheer for modern C++ technique!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor: replace QProcess/QObject with boost::process' accurately and concisely describes the primary architectural change throughout the changeset, which is the replacement of Qt-based process management with boost::process across the codebase.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch remove-qt/pr4-process

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/openms/source/SYSTEM/ExternalProcess.cpp (1)

171-197: ⚠️ Potential issue | 🟠 Major

Normalize POSIX exit classification across both branches.

native_exit_code() is the raw/uninterpreted status, while exit_code() is a separate value and is documented as meaningless once the child was terminated. The comparison at Line 179 therefore turns ordinary POSIX non-zero exits into CRASH, and the no-I/O path never consults the native status so signal terminations are only reported as NONZERO_EXIT. Please derive the return state from the native status once and reuse that logic in both branches. (codebrowser.dev)

Also applies to: 234-243

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openms/source/SYSTEM/ExternalProcess.cpp` around lines 171 - 197, The
POSIX branch incorrectly uses exit_code() instead of native_exit_code(), causing
normal non-zero exits to be classified as CRASH; update ExternalProcess.cpp to
compute a single status from child.native_exit_code() (e.g., store native =
child.native_exit_code()), then decide between RETURNSTATE::CRASH and
RETURNSTATE::NONZERO_EXIT based on that native value and reuse that logic in
both the _WIN32 and non-WIN32 flows (use native for POSIX signal/exit
classification while preserving the existing error_msg, exe, verbose and
callbackStdErr_ handling), and apply the same normalization to the later similar
block around the 234-243 region.

🧹 Nitpick comments (1)

src/openms/source/SYSTEM/JavaInfo.cpp (1)

47-50: Use \n instead of std::endl in these log messages.

These paths do not need an explicit flush, and std::endl makes every error report pay for one. As per coding guidelines "Use OpenMS logging macros and OpenMS::LogStream; avoid std::cout/err directly; avoid std::endl for performance (use \n)".

Also applies to: 83-95

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openms/source/SYSTEM/JavaInfo.cpp` around lines 47 - 50, Replace uses of
std::endl in the Java-Check logging statements with "\n" to avoid unnecessary
flushes; specifically update the OPENMS_LOG_ERROR output lines in JavaInfo.cpp
(the Java-Check messages around the block that references java_executable and
the later block at lines 83-95) so they end with "\n" instead of std::endl,
preserving the same message text and using the existing OpenMS logging
macros/LogStream.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openms/source/SYSTEM/JavaInfo.cpp`:
- Around line 33-38: The bp::child invocation should resolve relative executable
names via PATH: replace the direct use of java_executable in the bp::child
constructor with bp::search_path(static_cast<std::string>(java_executable)) so
Boost.Process will find "java" on PATH while preserving absolute paths (update
the bp::child call that currently uses java_executable and
bp::args/pipe_out/pipe_err accordingly). Also change the logging statements that
use std::endl (the occurrences around the JavaInfo logging) to use "\n" instead
for performance and to match project guidelines.

In `@src/openms/source/SYSTEM/PythonInfo.cpp`:
- Around line 82-83: In canRun() ensure error_msg is set when the child process
exits non‑zero: before returning false on (child.exit_code() != 0) assign
error_msg from the accumulated output (e.g. ss.str()) and include the exit code
/ context if helpful; update the branch that currently returns
(child.exit_code() == 0) to explicitly set error_msg on the failure path so
callers see the error instead of an empty message.

---

Duplicate comments:
In `@src/openms/source/SYSTEM/ExternalProcess.cpp`:
- Around line 171-197: The POSIX branch incorrectly uses exit_code() instead of
native_exit_code(), causing normal non-zero exits to be classified as CRASH;
update ExternalProcess.cpp to compute a single status from
child.native_exit_code() (e.g., store native = child.native_exit_code()), then
decide between RETURNSTATE::CRASH and RETURNSTATE::NONZERO_EXIT based on that
native value and reuse that logic in both the _WIN32 and non-WIN32 flows (use
native for POSIX signal/exit classification while preserving the existing
error_msg, exe, verbose and callbackStdErr_ handling), and apply the same
normalization to the later similar block around the 234-243 region.

---

Nitpick comments:
In `@src/openms/source/SYSTEM/JavaInfo.cpp`:
- Around line 47-50: Replace uses of std::endl in the Java-Check logging
statements with "\n" to avoid unnecessary flushes; specifically update the
OPENMS_LOG_ERROR output lines in JavaInfo.cpp (the Java-Check messages around
the block that references java_executable and the later block at lines 83-95) so
they end with "\n" instead of std::endl, preserving the same message text and
using the existing OpenMS logging macros/LogStream.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 35c67225-bf36-4374-ad32-d3be1671f620

📥 Commits

Reviewing files that changed from the base of the PR and between 4ac51e1 and a35320d.

📒 Files selected for processing (3)

• src/openms/source/SYSTEM/ExternalProcess.cpp
• src/openms/source/SYSTEM/JavaInfo.cpp
• src/openms/source/SYSTEM/PythonInfo.cpp

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/openms_gui/source/VISUAL/DIALOGS/FLASHDeconvTabWidget.cpp`:
- Around line 297-300: The call to exit(1) inside FLASHDeconvTabWidget when
ep_.run(...) for getFLASHDeconvExe() fails should be replaced with a graceful
tab-level failure: catch the non-success return from ep_.run, log or display an
error to the user (e.g., via QMessageBox::critical or the widget's status area),
disable or mark this tab as unavailable (use the parent QTabWidget or
this->setEnabled(false)), and return from the method instead of terminating the
process; update the block around ep_.run(..., {"-write_ini", tmp_file}, ...) to
perform those steps and ensure tmp_file is cleaned up if needed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: db2dead5-06f5-4ca9-bab8-f16bf2afbd0c

📥 Commits

Reviewing files that changed from the base of the PR and between a35320d and 682787e.

📒 Files selected for processing (3)

• src/openms_gui/include/OpenMS/VISUAL/MISC/ExternalProcessMBox.h
• src/openms_gui/source/VISUAL/DIALOGS/FLASHDeconvTabWidget.cpp
• src/openms_gui/source/VISUAL/DIALOGS/SwathTabWidget.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/openms_gui/source/VISUAL/DIALOGS/SwathTabWidget.cpp

[coderabbitai]

🧹 Nitpick comments (1)

src/openms/include/OpenMS/ANALYSIS/TOPDOWN/FLASHHelperClasses.h (1)

13-15: Remove non-OpenMS uint alias and use UInt from OpenMS/CONCEPT/Types.h

Lines 13–15 introduce a non-standard global type alias in a public header, violating the guideline to use OpenMS primitive types. This creates unnecessary coupling to bare uint across the module. Replace the alias with proper imports and migrate uint usages (index, ms_level) to UInt.

Suggested refactor

Replace:

-#ifdef _MSC_VER
-using uint = unsigned int; // POSIX uint not available on MSVC; was provided transitively via Qt
-#endif
+#include <OpenMS/CONCEPT/Types.h>

Then update member declarations:

-      uint index;
+      UInt index;
-      uint ms_level;
+      UInt ms_level;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openms/include/OpenMS/ANALYSIS/TOPDOWN/FLASHHelperClasses.h` around lines
13 - 15, Remove the non-standard global alias "using uint = unsigned int;" from
FLASHHelperClasses.h, add an `#include` for OpenMS/CONCEPT/Types.h, and change all
uses of the bare `uint` in this header (e.g., member variables named index,
ms_level and any function parameters/returns) to the OpenMS typedef `UInt`;
ensure all forward declarations and public APIs in FLASHHelperClasses
(class/struct names in this file) use `UInt` so the public header no longer
introduces a global `uint` alias.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/openms/include/OpenMS/ANALYSIS/TOPDOWN/FLASHHelperClasses.h`:
- Around line 13-15: Remove the non-standard global alias "using uint = unsigned
int;" from FLASHHelperClasses.h, add an `#include` for OpenMS/CONCEPT/Types.h, and
change all uses of the bare `uint` in this header (e.g., member variables named
index, ms_level and any function parameters/returns) to the OpenMS typedef
`UInt`; ensure all forward declarations and public APIs in FLASHHelperClasses
(class/struct names in this file) use `UInt` so the public header no longer
introduces a global `uint` alias.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: abce75cc-8965-4c98-8dc1-74e0c9fc91eb

📥 Commits

Reviewing files that changed from the base of the PR and between 682787e and 896c73b.

📒 Files selected for processing (1)

• src/openms/include/OpenMS/ANALYSIS/TOPDOWN/FLASHHelperClasses.h