-
Notifications
You must be signed in to change notification settings - Fork 222
Route Java console logging through System.out instead of native stdout #1825
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
base: main
Are you sure you want to change the base?
Changes from all commits
2b87330
99a33fc
2973059
aeeb1a1
dba0539
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| diff --git a/src/core/Presolver.c b/src/core/Presolver.c | ||
| index c0bdc9e..426008e 100644 | ||
| --- a/src/core/Presolver.c | ||
| +++ b/src/core/Presolver.c | ||
| @@ -720,6 +720,9 @@ PresolveStatus run_presolver(Presolver *presolver) | ||
| if (status != UNCHANGED) | ||
| { | ||
| // problem detected to be infeasible or unbounded | ||
| - print_infeas_or_unbnd_message(status); | ||
| + if (stgs->verbose) | ||
| + { | ||
| + print_infeas_or_unbnd_message(status); | ||
| + } | ||
| return status; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| /* clang-format off */ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| /* clang-format on */ | ||
|
|
@@ -55,6 +55,25 @@ log_buffer& global_log_buffer() | |
| return buffer; | ||
| } | ||
|
|
||
| // Overrides the sink used when log_to_console is true. Null (the default) keeps writing to | ||
| // std::cout; set by language bindings whose host runtime cannot safely receive writes to the | ||
| // native stdout stream -- for example Java, where a raw write there bypasses System.out and can | ||
| // corrupt tools that intercept it, such as Maven Surefire's forked-process protocol. | ||
| static std::mutex g_console_callback_mutex; | ||
| static log_console_callback_t g_console_callback = nullptr; | ||
|
|
||
| void set_console_log_callback(log_console_callback_t callback) | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_console_callback_mutex); | ||
| g_console_callback = callback; | ||
| } | ||
|
|
||
| static log_console_callback_t console_log_callback() | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_console_callback_mutex); | ||
| return g_console_callback; | ||
| } | ||
|
Comment on lines
+65
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Add regression tests for the console logging bridge.
As per coding guidelines: “Add unit tests. Please refer to 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| // Callback function for the buffer sink | ||
| static void buffer_log_callback(int lvl, const char* msg) | ||
| { | ||
|
|
@@ -161,8 +180,13 @@ init_logger_t::init_logger_t(std::string log_file, bool log_to_console) | |
|
|
||
| // re-initialize sinks | ||
| if (log_to_console) { | ||
| cuopt::default_logger().sinks().push_back( | ||
| std::make_shared<rapids_logger::ostream_sink_mt>(std::cout)); | ||
| if (auto callback = console_log_callback(); callback != nullptr) { | ||
| cuopt::default_logger().sinks().push_back( | ||
| std::make_shared<rapids_logger::callback_sink_mt>(callback)); | ||
| } else { | ||
| cuopt::default_logger().sinks().push_back( | ||
| std::make_shared<rapids_logger::ostream_sink_mt>(std::cout)); | ||
| } | ||
| } | ||
| if (!log_file.empty()) { | ||
| cuopt::default_logger().sinks().push_back( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package com.nvidia.cuopt.mathematicaloptimization; | ||
|
|
||
| /** | ||
| * Receives cuOpt's console log lines from native code and writes them through {@link | ||
| * System#out}, rather than the native library writing to the process's stdout stream directly. | ||
| * | ||
| * <p>A direct native write bypasses {@code System.out}, so it is invisible to anything that | ||
| * intercepts or redirects it -- {@link System#setOut}, a logging framework bridge, or Maven | ||
| * Surefire, which uses the forked JVM's stdout as its own communication channel and can | ||
| * misinterpret an unexpected raw write on it as the forked process having crashed. | ||
| * | ||
| * <p>Called from {@code cuopt_jni.cpp}; not part of the public API. | ||
| */ | ||
| final class NativeLogSink { | ||
| private NativeLogSink() {} | ||
|
|
||
| static void onLogLine(String message) { | ||
| System.out.print(message); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| #include <cuopt/mathematical_optimization/io/parser.hpp> | ||
| #include <cuopt/mathematical_optimization/optimization_problem_utils.hpp> | ||
| #include <pdlp/cuopt_c_internal.hpp> | ||
| #include <utilities/logger.hpp> | ||
|
|
||
| #include <jni.h> | ||
|
|
||
|
|
@@ -378,6 +379,57 @@ void mip_set_solution_callback(cuopt_float_t* solution, | |
| if (detach) { g_jvm->DetachCurrentThread(); } | ||
| } | ||
|
|
||
| jclass g_log_sink_class = nullptr; | ||
| jmethodID g_log_sink_method = nullptr; | ||
| std::once_flag g_log_sink_once; | ||
|
|
||
| // cuopt::log_console_callback_t: forwards a console log line to NativeLogSink.onLogLine, so it | ||
| // is written through System.out instead of directly to the native stdout stream. See | ||
| // register_console_log_sink for why that distinction matters. | ||
| void console_log_callback(int /* level */, const char* message) | ||
| { | ||
| if (g_log_sink_class == nullptr || g_log_sink_method == nullptr) { return; } | ||
|
|
||
| bool detach = false; | ||
| JNIEnv* env = get_callback_env(detach); | ||
| if (env == nullptr) { return; } | ||
|
|
||
| jstring line = env->NewStringUTF(message); | ||
| if (line != nullptr) { | ||
| env->CallStaticVoidMethod(g_log_sink_class, g_log_sink_method, line); | ||
| // A logging call is not the place to raise a Java exception; drop it rather than leave it | ||
| // pending for whatever JNI call happens to run next on this thread. | ||
| if (env->ExceptionCheck() == JNI_TRUE) { env->ExceptionClear(); } | ||
| env->DeleteLocalRef(line); | ||
| } | ||
|
|
||
| if (detach) { g_jvm->DetachCurrentThread(); } | ||
|
Comment on lines
+397
to
+406
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- applicable repository knowledge files ---'
find /tmp/coderabbit-repo-knowledge/nvidia-cuopt-f5e5051e -type f -name '*.md' -print
printf '%s\n' '--- target file outline ---'
ast-grep outline java/cuopt/src/main/native/cuopt_jni.cpp
printf '%s\n' '--- target source around logging and callers ---'
sed -n '350,455p' java/cuopt/src/main/native/cuopt_jni.cpp
printf '%s\n' '--- JNI-related symbols in target file ---'
rg -n -C 3 'NewStringUTF|ExceptionCheck|ExceptionClear|DetachCurrentThread|NativeLogSink|log_sink' java/cuopt/src/main/native/cuopt_jni.cppRepository: NVIDIA/cuopt Length of output: 17586 🌐 Web query:
💡 Result: According to the JNI specification, native code must not call most JNI functions while an exception is pending [1][2][3]. If an exception occurs, the native code is expected to either return immediately to the JVM or clear the exception using ExceptionClear before making further JNI calls [1][2][3]. NewStringUTF Calling NewStringUTF while an exception is pending is not permitted [1][2][3]. Doing so results in undefined behavior, which often manifests as a JNI-detected error or a fatal crash in debug builds [4]. Native developers must check for pending exceptions after calls that can throw them (e.g., via ExceptionCheck or by checking return values for NULL) and handle them appropriately before invoking any further JNI functions [1][3]. DetachCurrentThread DetachCurrentThread is a special case. Historically, it was not explicitly safe to call with a pending exception [5]. However, the JNI specification was amended to include DetachCurrentThread in the restricted list of JNI functions that are safe to call when an exception is pending [1][6]. If an exception is pending when DetachCurrentThread is called, the behavior is implementation-defined; specifically, the JVM may choose to report the existence of the pending exception (e.g., via the thread's uncaught exception handler) [5][7][6]. Top results: [1][5][7][6][3] Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- callback environment helper and adjacent callback flow ---'
sed -n '1,120p' java/cuopt/src/main/native/cuopt_jni.cpp
sed -n '220,345p' java/cuopt/src/main/native/cuopt_jni.cpp
printf '%s\n' '--- scoped repository conventions ---'
cat /tmp/coderabbit-repo-knowledge/nvidia-cuopt-f5e5051e/conventions/repo-wide.md
cat /tmp/coderabbit-repo-knowledge/nvidia-cuopt-f5e5051e/conventions/cpp-src.mdRepository: NVIDIA/cuopt Length of output: 16482 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- registration and solver entry points ---'
sed -n '455,525p' java/cuopt/src/main/native/cuopt_jni.cpp
sed -n '1015,1065p' java/cuopt/src/main/native/cuopt_jni.cpp
printf '%s\n' '--- logger callback declarations and uses ---'
rg -n -C 4 'set_console_log_callback|log_console_callback_t|console_log_callback' --glob '!build/**' --glob '!dist/**' .Repository: NVIDIA/cuopt Length of output: 10963 Clear a failed When 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // Registers console_log_callback with the native logger, once. Done lazily on first use (rather | ||
| // than in JNI_OnLoad) because FindClass needs the caller's classloader, which JNI_OnLoad does not | ||
| // reliably have. | ||
| void register_console_log_sink(JNIEnv* env) | ||
| { | ||
| std::call_once(g_log_sink_once, [env]() { | ||
| jclass local_cls = env->FindClass("com/nvidia/cuopt/mathematicaloptimization/NativeLogSink"); | ||
| if (local_cls == nullptr) { | ||
| env->ExceptionClear(); | ||
| return; | ||
| } | ||
| jmethodID method = env->GetStaticMethodID(local_cls, "onLogLine", "(Ljava/lang/String;)V"); | ||
| if (method == nullptr) { | ||
| env->ExceptionClear(); | ||
| env->DeleteLocalRef(local_cls); | ||
| return; | ||
| } | ||
| g_log_sink_class = static_cast<jclass>(env->NewGlobalRef(local_cls)); | ||
| g_log_sink_method = method; | ||
| env->DeleteLocalRef(local_cls); | ||
| cuopt::set_console_log_callback(&console_log_callback); | ||
| }); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| extern "C" jint JNI_OnLoad(JavaVM* vm, void*) | ||
|
|
@@ -421,6 +473,7 @@ Java_com_nvidia_cuopt_mathematicaloptimization_NativeCuOpt_readProblemWithFormat | |
| extern "C" JNIEXPORT jlong JNICALL | ||
| Java_com_nvidia_cuopt_mathematicaloptimization_NativeCuOpt_createSolverSettings(JNIEnv* env, jclass) | ||
| { | ||
| register_console_log_sink(env); | ||
| cuOptSolverSettings settings = nullptr; | ||
| if (!check_status(env, cuOptCreateSolverSettings(&settings), "cuOptCreateSolverSettings")) { | ||
| return 0; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: NVIDIA/cuopt
Length of output: 20033
🌐 Web query:
CMake ExternalProject_Add PATCH_COMMAND non-zero exit status causes configure or build failure FetchContent_MakeAvailable💡 Result:
A non-zero exit status from a PATCH_COMMAND in CMake results in a failure of the step, which stops the build process for that specific external project [1]. The behavior depends significantly on whether you are using ExternalProject_Add or FetchContent, as they serve different purposes and operate at different stages of the CMake lifecycle [2][3]. ExternalProject_Add When using ExternalProject_Add, commands like PATCH_COMMAND are executed at build time [4][2]. If the patch command returns a non-zero exit code, the build step fails, and subsequent steps (configure, build, install) for that external project are not executed [1]. Because these steps run as part of the build system (e.g., make or ninja), a failure halts the dependency's build process [4]. FetchContent The FetchContent module is designed to make content available during the CMake configure step, not at build time [2][3]. When you call FetchContent_Declare, it merely records the details for populating the content. When FetchContent_MakeAvailable is called, it triggers the population process [2][5]. It is important to note that FetchContent primarily manages downloading and updating; it does not support the same configure, build, and install steps as ExternalProject_Add [2][3]. While some versions of FetchContent may utilize underlying ExternalProject machinery, passing PATCH_COMMAND options to FetchContent_Declare may be ignored or cause unexpected behavior because FetchContent expects to manage the content directly [6][2]. If you need to apply patches to content managed by FetchContent, it is generally recommended to use CMake's file operations (like file(PATCH...)) or custom commands during the configure step, rather than relying on ExternalProject-style patch commands [2]. If your build is failing due to a patch error, ensure that: 1. The command provided in PATCH_COMMAND is robust and correctly handles its environment [4]. 2. You are using the appropriate tool for your goal: use ExternalProject_Add if you must build a non-CMake project or need complex build-time steps [3], and use FetchContent if you want to integrate CMake-based projects directly into your build configuration [2]. 3. If using FetchContent, consider avoiding manual patching via shell commands and instead use CMake's built-in file manipulation capabilities [2].
Citations:
🌐 Web query:
site:cmake.org/cmake/help/latest/module/FetchContent.html PATCH_COMMAND FetchContent_Declare ExternalProject_Add💡 Result:
Both FetchContent_Declare and ExternalProject_Add support the PATCH_COMMAND option to apply patches to downloaded content [1]. In FetchContent_Declare, you can use any download, update, or patch options that are supported by ExternalProject_Add [1]. Since FetchContent is designed to make content available at configure time, it leverages the underlying machinery of ExternalProject for these steps [1]. Note that while FetchContent supports patch options, it explicitly prohibits steps related to building, configuring, installing, or testing the content [1]. For ExternalProject_Add, the PATCH_COMMAND option is used to specify a command that runs after the content has been downloaded to apply modifications. An example of this usage would look like: ExternalProject_Add(my_project URL "..." PATCH_COMMAND patch -p1 < ${CMAKE_CURRENT_SOURCE_DIR}/my_patch.patch) When using PATCH_COMMAND, ensure that the specified command or script is available in your environment, as CMake will execute it during the build process for ExternalProject_Add or during the population process for FetchContent [1].
Citations:
Fail configuration when the PSLP patch cannot be applied.
The trailing
; truealways returns success aftergit apply --checkorgit applyfails.FetchContent_MakeAvailable(pslp)can then continue with unpatched PSLP, leaving its native stdout output enabled. Remove the unconditional success.🤖 Prompt for AI Agents
Source: Path instructions