diff --git a/libmamba/include/mamba/core/context.hpp b/libmamba/include/mamba/core/context.hpp index af73c3fa3c..563d692c28 100644 --- a/libmamba/include/mamba/core/context.hpp +++ b/libmamba/include/mamba/core/context.hpp @@ -65,10 +65,18 @@ namespace mamba class Logger; class Context; + struct OutputParams : LoggingParams + { + bool json{ false }; + bool quiet{ false }; + int verbosity{ 0 }; + }; + struct ContextOptions { bool enable_logging = false; bool enable_signal_handling = false; + std::optional output_params = {}; }; // Context singleton class @@ -78,12 +86,7 @@ namespace mamba static void use_default_signal_handler(bool val); - struct OutputParams : LoggingParams - { - bool json{ false }; - bool quiet{ false }; - int verbosity{ 0 }; - }; + using OutputParams = mamba::OutputParams; struct GraphicsParams { diff --git a/libmamba/include/mamba/core/logging.hpp b/libmamba/include/mamba/core/logging.hpp index 61314d7fa3..c3a0ce022f 100644 --- a/libmamba/include/mamba/core/logging.hpp +++ b/libmamba/include/mamba/core/logging.hpp @@ -694,6 +694,13 @@ namespace mamba auto unsafe_get() const -> const std::remove_pointer_t

*; ///@} + /** `true` if the log handler instance is the same for both. + */ + ///@{ + friend bool operator==(const AnyLogHandler&, const AnyLogHandler&) = default; + friend bool operator!=(const AnyLogHandler&, const AnyLogHandler&) = default; + ///@} + private: struct Interface; @@ -792,16 +799,21 @@ namespace mamba /** Changes the logging system configuration. - If a log handler is registered, this function calls `AnyLogHandler::set_params` with the - same arguments. + If a log handler is registered and `update_log_handler` is `true`, + this function calls `AnyLogHandler::set_params` with the same arguments. @see `mamba::logging:LogHandler` and @see `mamba::logging::AnyLogHandler` for details. This call is thread safe as long as the log handler implementation fulfills the thread-safety requirements, @see `mamba::logging::LogHandler`. + Warning: `update_log_handler = false` should only be used when implementing + other operations that need to update the params and then update the log handler + themselves, separately. + @returns The previous configuration of the logging system. */ - auto set_logging_params(LoggingParams new_params) -> LoggingParams; + auto set_logging_params(LoggingParams new_params, bool update_log_handler = true) + -> LoggingParams; // TODO: potential performance improvement: log(record_generator, log_level) where // record_generator is a callable which generates the log record but is only called AFTER we @@ -882,7 +894,7 @@ namespace mamba auto log_backtrace() -> void; /** Sends the log records in the backtrace history to the implementation's logging sinks, - but without filtering the logging level of the log records. + but with filtering the logging level of the log records as if they were logged now. If a log handler is registered, this function calls `AnyLogHandler::log_backtrace_no_guards`, otherwise this function will do nothing. @@ -1024,6 +1036,9 @@ namespace mamba::logging inline auto enable_backtrace(size_t records_buffer_size) -> void { + auto params = get_logging_params(); + params.log_backtrace = records_buffer_size; + set_logging_params(params, false); call_log_handler_if_existing(&AnyLogHandler::enable_backtrace, records_buffer_size); } diff --git a/libmamba/include/mamba/core/logging_tools.hpp b/libmamba/include/mamba/core/logging_tools.hpp index 7787da1f30..65e7069b48 100644 --- a/libmamba/include/mamba/core/logging_tools.hpp +++ b/libmamba/include/mamba/core/logging_tools.hpp @@ -3,6 +3,8 @@ // Distributed under the terms of the BSD 3-Clause License. // // The full license is in the file LICENSE, distributed with this software. +#ifndef MAMBA_CORE_LOGGING_TOOLS_HPP +#define MAMBA_CORE_LOGGING_TOOLS_HPP #include #include @@ -25,6 +27,18 @@ namespace mamba::logging { out.flush() }; }; + /** Transforms a source location into a human-readable string ready for logging purpose. */ + inline auto as_log(const std::source_location& location) -> std::string + { + return fmt::format( + "{}:{}:{} {}", + location.file_name(), + location.line(), + location.column(), + location.function_name() + ); + } + namespace details { inline auto queue_push(std::deque& queue, size_t max_elements, LogRecord record) @@ -37,6 +51,21 @@ namespace mamba::logging } } + inline auto should_be_emitted(const LogRecord& record, log_level current_level) -> bool + { + switch (current_level) + { + case log_level::off: + return false; + + case log_level::all: + return true; + + default: + return record.level >= current_level; + } + } + /** Backtrace feature implementation in it's most basic form. This is the simplest implementation for a backtrace feature @@ -59,22 +88,19 @@ namespace mamba::logging } /** If the backtrace feature is enabled, moves the log record into the backtrace - history and returns `true`. Otherwise do nothing and returns `false`. - - The log record is taken by reference to allow taking ownership of it through - a move, but also not taking ownership and not forcing a copy if we actually - don't need it. + history, otherwise calls the provided invocable with the log record as argument. */ - auto push_if_enabled(LogRecord& record) -> bool + template Func> + auto push_if_enabled(LogRecord&& record, Func&& otherwise_func) -> void { - if (not is_enabled()) + if (is_enabled()) { - return false; + queue_push(backtrace, backtrace_max, std::move(record)); + } + else + { + std::invoke(std::forward(otherwise_func), std::move(record)); } - - queue_push(backtrace, backtrace_max, std::move(record)); - - return true; } /** Changes the number of log records kept in the backtrace history. @@ -138,17 +164,6 @@ namespace mamba::logging } }; - inline auto as_log(const std::source_location& location) -> std::string - { - return fmt::format( - "{}:{}:{} {}", - location.file_name(), - location.line(), - location.column(), - location.function_name() - ); - } - struct log_to_stream_options { bool with_location = false; @@ -158,9 +173,8 @@ namespace mamba::logging log_to_stream(OutputStream auto& out, const LogRecord& record, log_to_stream_options options = {}) -> OutputStream auto& { - auto location_str = options.with_location - ? fmt::format(" ({})", details::as_log(record.location)) - : std::string{}; + auto location_str = options.with_location ? fmt::format(" ({})", as_log(record.location)) + : std::string{}; out << fmt::format( "\n{} {}{} : {}", @@ -234,7 +248,13 @@ namespace mamba::logging auto enable_backtrace(size_t record_buffer_size) -> void; auto disable_backtrace() -> void; - auto log_backtrace() -> void; + + /** @see `AnyLogHandler::log_backtrace()` + + @param filter_current_level If `true`, will filter out log records that would not + pass the log level filter if they were emitted right now. + */ + auto log_backtrace(bool filter_current_level = false) -> void; auto log_backtrace_no_guards() -> void; auto flush(std::optional source = {}) -> void; @@ -245,12 +265,14 @@ namespace mamba::logging //////////////////////////////////////////// // History api - thread-safe - /** @returns A copy of the current log record history. + /** Captures the currently stored sequence of `LogRecord`. + @param `and_clear` Will also clear the existing history if `true`. + @returns A copy of the current log record history. The value should be considered immediately obsolete as new log records could be pushed concurrently. The returned history will be empty if `is_started()` == false. */ - auto capture_history() const -> std::vector; + auto capture_history(bool and_clear = false) const -> std::vector; /** Clears the internal history. @@ -343,7 +365,13 @@ namespace mamba::logging auto enable_backtrace(size_t record_buffer_size) -> void; auto disable_backtrace() -> void; - auto log_backtrace() -> void; + + /** @see `AnyLogHandler::log_backtrace()` + + @param filter_current_level If `true`, will filter out log records that would not + pass the log level filter if they were emitted right now. + */ + auto log_backtrace(bool filter_current_level = false) -> void; auto log_backtrace_no_guards() -> void; auto flush(std::optional source = {}) -> void; @@ -431,16 +459,19 @@ namespace mamba::logging inline auto LogHandler_History::log(LogRecord record) -> void { assert(pimpl); - if (pimpl->current_log_level < record.level) + if (not details::should_be_emitted(record, pimpl->current_log_level)) { return; } auto synched_data = pimpl->data.synchronize(); - if (not synched_data->backtrace.push_if_enabled(record)) - { - details::queue_push(synched_data->history, options.max_records_count, std::move(record)); - } + synched_data->backtrace.push_if_enabled( + std::move(record), + [&](LogRecord&& record_) + { + details::queue_push(synched_data->history, options.max_records_count, std::move(record_)); + } + ); } inline auto LogHandler_History::enable_backtrace(size_t record_buffer_size) -> void @@ -455,12 +486,16 @@ namespace mamba::logging pimpl->data->backtrace.disable(); } - inline auto LogHandler_History::log_backtrace() -> void + inline auto LogHandler_History::log_backtrace(bool filter_current_level) -> void { assert(pimpl); auto synched_data = pimpl->data.synchronize(); for (auto& log : synched_data->backtrace) { + if (filter_current_level and not details::should_be_emitted(log, pimpl->current_log_level)) + { + continue; + } details::queue_push(synched_data->history, options.max_records_count, std::move(log)); } @@ -470,7 +505,8 @@ namespace mamba::logging inline auto LogHandler_History::log_backtrace_no_guards() -> void { assert(pimpl); - log_backtrace(); // Similar in this context + log_backtrace(true); // Similar in this context but we want to filter out logs that would + // not pass the level filter now } inline auto LogHandler_History::flush(std::optional) -> void @@ -485,12 +521,25 @@ namespace mamba::logging // nothing to do, we keep history, there is no flush } - inline auto LogHandler_History::capture_history() const -> std::vector + inline auto LogHandler_History::capture_history(bool and_clear) const -> std::vector { if (pimpl) { auto synched_data = pimpl->data.synchronize(); - return std::vector(synched_data->history.begin(), synched_data->history.end()); + if (and_clear) + { + std::vector result(synched_data->history.size()); + std::ranges::move(synched_data->history, result.begin()); + synched_data->history.clear(); + return result; + } + else + { + return std::vector( + synched_data->history.begin(), + synched_data->history.end() + ); + } } return {}; @@ -504,7 +553,7 @@ namespace mamba::logging } } - auto LogHandler_History::is_started() const -> bool + inline auto LogHandler_History::is_started() const -> bool { return pimpl != nullptr; } @@ -588,17 +637,18 @@ namespace mamba::logging assert(out); assert(pimpl); - if (pimpl->current_log_level > record.level) + if (not details::should_be_emitted(record, pimpl->current_log_level)) { return; } auto level = record.level; - if (not pimpl->backtrace->push_if_enabled(record)) - { - details::log_to_stream(*out, record, { .with_location = pimpl->log_location }); - } + pimpl->backtrace->push_if_enabled( + std::move(record), + [this](LogRecord&& record_) + { details::log_to_stream(*out, record_, { .with_location = pimpl->log_location }); } + ); if (level <= pimpl->flush_threshold) { @@ -625,7 +675,7 @@ namespace mamba::logging } template - inline auto LogHandler_Stream::log_backtrace() -> void + inline auto LogHandler_Stream::log_backtrace(bool filter_current_level) -> void { assert(out); assert(pimpl); @@ -633,6 +683,11 @@ namespace mamba::logging auto synched_backtrace = pimpl->backtrace.synchronize(); for (auto& log_record : *synched_backtrace) { + if (filter_current_level + and not details::should_be_emitted(log_record, pimpl->current_log_level)) + { + continue; + } details::log_to_stream(*out, log_record, { .with_location = pimpl->log_location }); } synched_backtrace->clear(); @@ -644,7 +699,8 @@ namespace mamba::logging assert(out); assert(pimpl); - log_backtrace(); // Similar in this context + log_backtrace(true); // Similar in this context but we want to filter out logs that would + // not pass the level filter now } template @@ -672,3 +728,5 @@ namespace mamba::logging } } + +#endif diff --git a/libmamba/include/mamba/core/output.hpp b/libmamba/include/mamba/core/output.hpp index fc3d0ee4ba..6efd639171 100644 --- a/libmamba/include/mamba/core/output.hpp +++ b/libmamba/include/mamba/core/output.hpp @@ -129,6 +129,11 @@ namespace mamba ///< object to mark it as a success will be ///< done once the other changes are ///< applied. + + /// Construct a json edit based on the members of a json object by using + /// each member as an assignation into the json object that will be edited + /// by this resulgint json edit. + static auto from_json_object_members(nlohmann::json object) -> JSONEdit; }; class Console @@ -183,11 +188,31 @@ namespace mamba */ void set_json_output(JSONEdit edit); + /** Assigns the provided json value to the associated location in the json output object. + This overload is a shorter version for the 1 location 1 value assignation case + which is quite common. + */ + template + requires requires(const T& value) { nlohmann::json{ value }; } + void set_json_output(nlohmann::json::json_pointer location, T&& value) + { + // clang-format off + set_json_output({ + .to_assign{ + { std::move(location), std::forward(value) } + } + }); + // clang-format on + } + /** If json output was requested, calling this before destroying a `Console` instance will not lead to a json output. */ void cancel_json_print(); + // FIXME: documentation + static void setup_log_handling_for_json(); + static void print_buffer(std::ostream& ostream); const Context& context() const; diff --git a/libmamba/src/api/config.cpp b/libmamba/src/api/config.cpp index f7b2f98f35..58485a0a92 100644 --- a/libmamba/src/api/config.cpp +++ b/libmamba/src/api/config.cpp @@ -31,7 +31,7 @@ namespace mamba auto specs = config.at("specs").value>(); int dump_opts = MAMBA_SHOW_CONFIG_DESCS | show_long_desc | show_group; - print_dump(config, dump_opts, specs); + print_dump(config, dump_opts, std::move(specs)); config.operation_teardown(); } @@ -64,7 +64,7 @@ namespace mamba int dump_opts = MAMBA_SHOW_CONFIG_VALUES | show_sources | show_desc | show_long_desc | show_group | show_all_rcs | show_all; - print_dump(config, dump_opts, specs); + print_dump(config, dump_opts, std::move(specs)); config.operation_teardown(); } diff --git a/libmamba/src/api/configuration.cpp b/libmamba/src/api/configuration.cpp index 0c48d2026f..ddb3556e20 100644 --- a/libmamba/src/api/configuration.cpp +++ b/libmamba/src/api/configuration.cpp @@ -815,14 +815,9 @@ namespace mamba mamba::log_level log_level_fallback_hook(Configuration& config) { - const auto& ctx = config.context(); - - if (ctx.output_params.json) - { - return mamba::log_level::critical; - } - else if (config.at("verbose").configured()) + if (config.at("verbose").configured()) { + const auto& ctx = config.context(); switch (ctx.output_params.verbosity) { case 0: @@ -1969,6 +1964,15 @@ namespace mamba .group("Output, Prompt and Flow Control") .set_rc_configurable() .needs({ "print_config_only", "print_context_only" }) + .set_post_merge_hook( + [](const bool enabled) + { + if (enabled) + { + Console::setup_log_handling_for_json(); + } + } + ) .set_env_var_names() .description("Report all output as json")); @@ -2049,6 +2053,15 @@ namespace mamba .set_rc_configurable() .set_env_var_names() .needs({ "json", "print_config_only", "print_context_only" }) + .set_post_merge_hook( + [](const bool enabled) + { + if (enabled) + { + logging::stop_logging(); + } + } + ) .description("Set quiet mode (print less output)")); insert(Configurable("verbose", 0) @@ -2775,9 +2788,17 @@ namespace mamba void print_dump(const Configuration& config, int dump_opts, std::vector dump_names) { - // Note: this function is intended to get more complex with incoming changes and need to be - // isolated in preparation for these changes. const std::string dump_text = hide_secrets(config.dump(dump_opts, std::move(dump_names))); - std::cout << dump_text << std::endl; + if (config.context().output_params.json) + { + // merge the output with existing json output + auto dump_json = nlohmann::json::parse(dump_text); + Console::instance().set_json_output(JSONEdit::from_json_object_members(dump_json)); + } + else + { + std::cout << dump_text << std::endl; // outputs YAML + } } + } diff --git a/libmamba/src/api/create.cpp b/libmamba/src/api/create.cpp index d2ad8c0217..04ca6835e3 100644 --- a/libmamba/src/api/create.cpp +++ b/libmamba/src/api/create.cpp @@ -223,14 +223,17 @@ namespace mamba { if (!is_clone && create_specs.empty() && json_format) { - // Just print the JSON - nlohmann::json output; - output["actions"]["FETCH"] = nlohmann::json::array(); - output["actions"]["PREFIX"] = ctx.prefix_params.target_prefix; - output["dry_run"] = true; - output["prefix"] = ctx.prefix_params.target_prefix; - output["success"] = true; - std::cout << output.dump(2) << std::endl; + // clang-format off + Console::instance().set_json_output({ + .to_assign = { + { "/actions/FETCH"_json_pointer, nlohmann::json::array() }, + { "/actions/PREFIX"_json_pointer, ctx.prefix_params.target_prefix }, + { "/dry_run"_json_pointer, true }, + { "/prefix"_json_pointer, ctx.prefix_params.target_prefix } + }, + .set_success = true + }); + // clang-format on return; } } diff --git a/libmamba/src/api/env.cpp b/libmamba/src/api/env.cpp index efea14d3c8..f85b657807 100644 --- a/libmamba/src/api/env.cpp +++ b/libmamba/src/api/env.cpp @@ -50,7 +50,6 @@ namespace mamba if (ctx.output_params.json) { - nlohmann::json res; const auto pfxs = env_manager.list_all_known_prefixes(); std::vector envs(pfxs.size()); std::transform( @@ -59,8 +58,8 @@ namespace mamba envs.begin(), [](const mamba::fs::u8path& path) { return path.string(); } ); - res["envs"] = envs; - std::cout << res.dump(4) << std::endl; + + Console::instance().set_json_output("/envs"_json_pointer, std::move(envs)); return; } @@ -180,9 +179,7 @@ namespace mamba if (console.context().output_params.json) { - nlohmann::ordered_json j; - j["env_vars"] = env_vars; - std::cout << j.dump(4) << std::endl; + Console::instance().set_json_output("/env_vars"_json_pointer, env_vars); return; } diff --git a/libmamba/src/api/list.cpp b/libmamba/src/api/list.cpp index fc45a75148..aeb7192769 100644 --- a/libmamba/src/api/list.cpp +++ b/libmamba/src/api/list.cpp @@ -205,7 +205,7 @@ namespace mamba } } } - std::cout << jout.dump(4) << std::endl; + Console::instance().set_json_output("/packages"_json_pointer, std::move(jout)); } else { diff --git a/libmamba/src/api/repoquery.cpp b/libmamba/src/api/repoquery.cpp index 84566166f9..2302d7b7e7 100644 --- a/libmamba/src/api/repoquery.cpp +++ b/libmamba/src/api/repoquery.cpp @@ -129,7 +129,9 @@ namespace mamba switch (format) { case QueryResultFormat::Json: - out << res.groupby("name").json().dump(4); + Console::instance().set_json_output( + JSONEdit::from_json_object_members(res.groupby("name").json()) + ); break; case QueryResultFormat::Pretty: res.pretty(out, show_all_builds); @@ -158,7 +160,7 @@ namespace mamba res.tree(out, graphics_params); break; case QueryResultFormat::Json: - out << res.json().dump(4); + Console::instance().set_json_output(JSONEdit::from_json_object_members(res.json())); break; case QueryResultFormat::Table: case QueryResultFormat::RecursiveTable: @@ -185,7 +187,7 @@ namespace mamba res.tree(out, graphics_params); break; case QueryResultFormat::Json: - out << res.json().dump(4); + Console::instance().set_json_output(JSONEdit::from_json_object_members(res.json())); break; case QueryResultFormat::Table: case QueryResultFormat::RecursiveTable: diff --git a/libmamba/src/core/context.cpp b/libmamba/src/core/context.cpp index 75e60353be..2cca030b6e 100644 --- a/libmamba/src/core/context.cpp +++ b/libmamba/src/core/context.cpp @@ -53,6 +53,12 @@ namespace mamba void Context::start_logging(logging::AnyLogHandler log_handler) { + if (output_params.json or output_params.quiet) + { + throw mamba_error{ "cannot start logging with `--json` or `--quiet`", + mamba_error_code::incorrect_usage }; + } + // Only change the log-handler if specified, keep the current one otherwise. if (log_handler) { @@ -98,10 +104,28 @@ namespace mamba enable_signal_handling(); } - if (options.enable_logging) + if (options.output_params) + { + output_params = *options.output_params; + } + + // Do not start logging if `--json` or `--quiet` are used. + if (options.enable_logging and not(output_params.json or output_params.quiet)) { start_logging(std::move(log_handler)); } + + if (output_params.quiet) + { + // Explicitly stop logging in case a log-handler + // has already been installed before `Context`'s creation. + logging::stop_logging(); + } + if (output_params.json) + { + // We still need to capture the log and put it into the json output. + Console::setup_log_handling_for_json(); + } } Context::~Context() diff --git a/libmamba/src/core/logging.cpp b/libmamba/src/core/logging.cpp index 39623385e6..841e1043bf 100644 --- a/libmamba/src/core/logging.cpp +++ b/libmamba/src/core/logging.cpp @@ -116,6 +116,11 @@ namespace mamba::logging auto stop_logging(stop_reason reason) -> AnyLogHandler { + if (not details::current_log_handler) + { + // No installed log-handler: do nothing. + return {}; + } return change_log_handler({}, {}, reason, {}); } @@ -160,12 +165,12 @@ namespace mamba::logging return details::logging_params.value(); } - auto set_logging_params(LoggingParams new_params) -> LoggingParams + auto set_logging_params(LoggingParams new_params, bool update_log_handler) -> LoggingParams { auto synched_params = details::logging_params.synchronize(); LoggingParams previous_params = *synched_params; *synched_params = std::move(new_params); - if (details::current_log_handler) + if (update_log_handler and details::current_log_handler) { details::current_log_handler.set_params(*synched_params); } diff --git a/libmamba/src/core/output.cpp b/libmamba/src/core/output.cpp index 35d676f8ec..532054f0f4 100644 --- a/libmamba/src/core/output.cpp +++ b/libmamba/src/core/output.cpp @@ -21,6 +21,7 @@ #include "mamba/core/context.hpp" #include "mamba/core/execution.hpp" #include "mamba/core/invoke.hpp" +#include "mamba/core/logging_tools.hpp" #include "mamba/core/output.hpp" #include "mamba/core/tasksync.hpp" #include "mamba/core/thread_utils.hpp" @@ -275,6 +276,54 @@ namespace mamba ***********/ + namespace + { + std::unique_ptr log_history_handler; + + auto to_json(const logging::LogRecord& record) -> nlohmann::json + { + nlohmann::json result = { { "message", record.message }, + { "level", name_of(record.level) }, + { "source", name_of(record.source) } }; + if (strlen(record.location.file_name()) > 0 or strlen(record.location.function_name()) > 0) + { + result["location"] = logging::as_log(record.location); + } + return result; + } + + auto capture_log_history_as_json() -> nlohmann::json + { + nlohmann::json json_history = nlohmann::json::array({}); + + if (not log_history_handler) + { + return json_history; + } + + // We clear the history on the way in case this function is called + // more than once in the program's execution. + const auto history = log_history_handler->capture_history(true); + for (const auto& log_record : history) + { + json_history.push_back(to_json(log_record)); + } + return json_history; + } + } + + void Console::setup_log_handling_for_json() + { + if (not log_history_handler) + { + log_history_handler = std::make_unique( + logging::LogHandler_History::Options{ .clear_on_stop = false } + ); + } + + logging::set_log_handler(log_history_handler.get()); // use the existing logging parameters + } + using ConsoleBuffer = std::vector; class ConsoleData @@ -334,8 +383,9 @@ namespace mamba Console::~Console() { - if (/*not context().output_params.quiet and*/ not p_data->is_json_print_cancelled - and not p_data->json_output.is_null()) + // Note: even if the json is empty, we should still print + // and empty json object as long as `--json` is used. + if (context().output_params.json and not p_data->is_json_print_cancelled) { this->print_json_output(); } @@ -548,6 +598,12 @@ namespace mamba void Console::print_json_output() { + if (context().output_params.json and log_history_handler) + { + auto log_history = capture_log_history_as_json(); + set_json_output("/log_history"_json_pointer, std::move(log_history)); + } + print(p_data->json_output.dump(4), true); } @@ -613,5 +669,26 @@ namespace mamba } } + auto JSONEdit::from_json_object_members(nlohmann::json object) -> JSONEdit + { + if (not object.is_object()) + { + throw mamba_error( + "from_json_object_members(json) json value is not an object, object is required", + mamba_error_code::incorrect_usage + ); + } + + JSONEdit edit; + edit.to_assign.reserve(object.size()); + for (auto&& [member_path, value] : object.items()) + { + const nlohmann::json::json_pointer location{ fmt::format("/{}", member_path) }; + edit.to_assign.emplace_back(location, std::move(value)); + } + + return edit; + } + } // namespace mamba diff --git a/libmamba/src/core/util.cpp b/libmamba/src/core/util.cpp index 29571c34b7..30bbb52493 100644 --- a/libmamba/src/core/util.cpp +++ b/libmamba/src/core/util.cpp @@ -76,9 +76,11 @@ namespace mamba const std::regex& token_regex() { - // usernames on anaconda.org can have a underscore, which influences the - // first two characters - static const std::regex token_regex{ "/t/([a-zA-Z0-9-_]{0,2}[a-zA-Z0-9-]*)" }; + // Usernames on anaconda.org can have a underscore, which influences the + // first two characters. + // The `+` is there to make sure we dont match `/t/*` with `*` being literal or anything we + // don't capture here. + static const std::regex token_regex{ "/t/([a-zA-Z0-9-_]{0,2}[a-zA-Z0-9-_]+)" }; return token_regex; } diff --git a/libmamba/tests/libmamba_logging/include/mamba/testing/test_logging_common.hpp b/libmamba/tests/libmamba_logging/include/mamba/testing/test_logging_common.hpp index 572113700b..4a3c1488e2 100644 --- a/libmamba/tests/libmamba_logging/include/mamba/testing/test_logging_common.hpp +++ b/libmamba/tests/libmamba_logging/include/mamba/testing/test_logging_common.hpp @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -32,6 +33,7 @@ namespace mamba::logging::testing std::size_t stop_count = 0; std::size_t log_count = 0; std::size_t real_output_log_count = 0; + std::size_t filtered_out_log_count = 0; std::size_t log_level_change_count = 0; std::size_t params_change_count = 0; std::size_t backtrace_size_change_count = 0; @@ -97,9 +99,16 @@ namespace mamba::logging::testing stats->current_params = std::move(new_params); } - auto log(LogRecord) -> void + auto log(LogRecord record) -> void { auto stats = pimpl->stats.synchronize(); + if (not details::should_be_emitted(record, stats->current_params.logging_level)) + { + // we ignore logs that should be filtered + ++stats->filtered_out_log_count; + return; + } + stats->log_count++; if (stats->backtrace_size == 0) { @@ -279,6 +288,15 @@ namespace mamba::logging::testing } stats.log_count += options.log_count; stats.real_output_log_count += options.log_count; + + // log level handling + if (options.level > log_level::trace) + { + log({ .message = "this log record must be filtered out, if you read this from the log output this test has failed", + .level = log_level::trace, + .source = options.log_sources.front() }); + ++stats.filtered_out_log_count; + } } // backtrace diff --git a/libmamba/tests/libmamba_logging/test_logging_tools.cpp b/libmamba/tests/libmamba_logging/test_logging_tools.cpp index 7b07ddb6d9..e4f8f0b5dd 100644 --- a/libmamba/tests/libmamba_logging/test_logging_tools.cpp +++ b/libmamba/tests/libmamba_logging/test_logging_tools.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -68,7 +69,10 @@ namespace mamba::logging TEST_CASE("details::BasicBacktrace") { - LogRecord log_not_pushed{ .message = "must not be pushed", .source = log_source::tests }; + using namespace std::string_view_literals; + + const std::string must_not_be_published = "must not be pushed"; + LogRecord log_not_pushed{ .message{ must_not_be_published }, .source = log_source::tests }; LogRecord log_a{ .message = "A", .source = log_source::tests }; LogRecord log_b{ .message = "B", .source = log_source::tests }; LogRecord log_c{ .message = "C", .source = log_source::tests }; @@ -77,26 +81,34 @@ namespace mamba::logging LogRecord log_f{ .message = "F", .source = log_source::tests }; LogRecord log_g{ .message = "G", .source = log_source::tests }; + std::vector emitted; + auto emit = [&](LogRecord&& record) { emitted.push_back(std::move(record)); }; + details::BasicBacktrace b; REQUIRE(not b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 0); REQUIRE(b.size() == 0); REQUIRE(b.empty()); - b.push_if_enabled(log_a); + b.push_if_enabled(std::move(log_not_pushed), emit); REQUIRE(not b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 0); REQUIRE(b.size() == 0); REQUIRE(b.empty()); - REQUIRE(not log_a.message.empty()); + REQUIRE(log_not_pushed.message.empty()); + REQUIRE(not emitted.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); b.set_max_trace(2); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 0); REQUIRE(b.size() == 0); REQUIRE(b.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); - b.push_if_enabled(log_a); + b.push_if_enabled(std::move(log_a), emit); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 1); REQUIRE(b.size() == 1); @@ -104,8 +116,10 @@ namespace mamba::logging REQUIRE(b.begin()->message == "A"); REQUIRE(std::next(b.end(), -1)->message == "A"); REQUIRE(log_a.message.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); - b.push_if_enabled(log_b); + b.push_if_enabled(std::move(log_b), emit); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 2); REQUIRE(b.size() == 2); @@ -113,8 +127,10 @@ namespace mamba::logging REQUIRE(b.begin()->message == "A"); REQUIRE(std::next(b.end(), -1)->message == "B"); REQUIRE(log_b.message.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); - b.push_if_enabled(log_c); + b.push_if_enabled(std::move(log_c), emit); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 2); REQUIRE(b.size() == 2); @@ -122,6 +138,8 @@ namespace mamba::logging REQUIRE(b.begin()->message == "B"); REQUIRE(std::next(b.end(), -1)->message == "C"); REQUIRE(log_b.message.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); b.clear(); REQUIRE(b.is_enabled()); @@ -129,7 +147,7 @@ namespace mamba::logging REQUIRE(b.size() == 0); REQUIRE(b.empty()); - b.push_if_enabled(log_d); + b.push_if_enabled(std::move(log_d), emit); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 1); REQUIRE(b.size() == 1); @@ -137,8 +155,10 @@ namespace mamba::logging REQUIRE(b.begin()->message == "D"); REQUIRE(std::next(b.end(), -1)->message == "D"); REQUIRE(log_d.message.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); - b.push_if_enabled(log_e); + b.push_if_enabled(std::move(log_e), emit); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 2); REQUIRE(b.size() == 2); @@ -146,9 +166,11 @@ namespace mamba::logging REQUIRE(b.begin()->message == "D"); REQUIRE(std::next(b.end(), -1)->message == "E"); REQUIRE(log_e.message.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); - b.push_if_enabled(log_f); + b.push_if_enabled(std::move(log_f), emit); REQUIRE(b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 2); REQUIRE(b.size() == 2); @@ -156,26 +178,32 @@ namespace mamba::logging REQUIRE(b.begin()->message == "E"); REQUIRE(std::next(b.end(), -1)->message == "F"); REQUIRE(log_f.message.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); b.disable(); REQUIRE(not b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 0); REQUIRE(b.size() == 0); REQUIRE(b.empty()); + REQUIRE(emitted.size() == 1); + REQUIRE(emitted.back().message == must_not_be_published); - b.push_if_enabled(log_g); + b.push_if_enabled(std::move(log_g), emit); REQUIRE(not b.is_enabled()); REQUIRE(std::distance(b.begin(), b.end()) == 0); REQUIRE(b.size() == 0); REQUIRE(b.empty()); - REQUIRE(not log_g.message.empty()); + REQUIRE(log_g.message.empty()); + REQUIRE(emitted.size() == 2); + REQUIRE(emitted.back().message == "G"); } TEST_CASE("details::log_to_stream") { std::stringstream out; const auto location = std::source_location::current(); - const auto location_str = fmt::format(" ({})", details::as_log(location)); + const auto location_str = fmt::format(" ({})", as_log(location)); details::log_to_stream( out, diff --git a/micromamba/src/env.cpp b/micromamba/src/env.cpp index ea20c7766f..85014534b7 100644 --- a/micromamba/src/env.cpp +++ b/micromamba/src/env.cpp @@ -13,6 +13,7 @@ #include "mamba/api/update.hpp" #include "mamba/core/channel_context.hpp" #include "mamba/core/environments_manager.hpp" +#include "mamba/core/output.hpp" #include "mamba/core/prefix_data.hpp" #include "mamba/core/util.hpp" #include "mamba/specs/conda_url.hpp" @@ -78,6 +79,7 @@ set_env_command(CLI::App* com, mamba::Configuration& config) // Raise a warning if `--json` and `--explicit` are used together. if (json_format && explicit_format) { + // FIXME? std::cerr << "Warning: `--json` and `--explicit` are used together but are incompatible. The `--json` flag will be ignored." << std::endl; } @@ -192,26 +194,21 @@ set_env_command(CLI::App* com, mamba::Configuration& config) dependencies << (first_dependency_printed ? "\n" : ""); - std::cout << "{\n"; - std::cout << " \"channels\": [\n"; - for (auto channel_it = channels.begin(); channel_it != channels.end(); ++channel_it) - { - auto last_channel = std::next(channel_it) == channels.end(); - std::cout << " \"" << *channel_it << "\"" << (last_channel ? "" : ",") << "\n"; - } - std::cout << " ],\n"; - - std::cout << " \"dependencies\": [\n" << dependencies.str() << " ],\n"; - - std::cout << " \"name\": \"" - << mamba::detail::get_env_name(ctx, ctx.prefix_params.target_prefix) - << "\",\n"; - std::cout << " \"prefix\": " << ctx.prefix_params.target_prefix << "\n"; - - std::cout << "}\n"; + auto deps_json = nlohmann::json::parse(fmt::format("[ {} ]", dependencies.str())); + assert(deps_json.is_array()); - std::cout.flush(); + // clang-format off + mamba::JSONEdit out_json{ + .to_assign = { + { "/channels"_json_pointer , channels }, + { "/dependencies"_json_pointer , std::move(deps_json) }, + { "/name"_json_pointer, mamba::detail::get_env_name(ctx, ctx.prefix_params.target_prefix) }, + { "/prefix"_json_pointer , ctx.prefix_params.target_prefix } + } + }; + mamba::Console::instance().set_json_output(std::move(out_json)); + // clang-format on } else { diff --git a/micromamba/src/main.cpp b/micromamba/src/main.cpp index 98430837d3..68df8a0809 100644 --- a/micromamba/src/main.cpp +++ b/micromamba/src/main.cpp @@ -15,6 +15,8 @@ #include "mamba/util/os_win.hpp" #endif +#include + #include #include "mamba/api/configuration.hpp" @@ -31,19 +33,67 @@ using namespace mamba; // NOLINT(build/namespaces) +auto +decide_preconfig_context_options(int argc, char** argv) -> mamba::ContextOptions +{ + using namespace std::literals; + + mamba::ContextOptions options{ + .enable_logging = true, + .enable_signal_handling = true, + }; + + // We want to initialize options.output_params (which is an `std::optional`) + // only if `--json` or `--quiet` appears in the program parameters. + // Otherwise it must stay `std::nullopt`. + + const auto ready_output_params = [&] + { + if (not options.output_params.has_value()) + { + options.output_params.emplace(); + } + }; + + for (const char* arg : std::ranges::subrange(argv, argv + argc)) + { + if (arg == "--json"sv) + { + ready_output_params(); + options.output_params->json = true; + } + else if (arg == "--quiet"sv) + { + ready_output_params(); + options.output_params->quiet = true; + } + } + + return options; +} + +auto +decide_log_handler(const ContextOptions& options) -> mamba::logging::AnyLogHandler +{ + if (options.output_params and (options.output_params->json or options.output_params->quiet)) + { + return {}; + } + + return mamba::logging::spdlogimpl::LogHandler_spdlog{}; +} + int main(int argc, char** argv) { mamba::MainExecutor scoped_threads; - mamba::Context ctx{ { - .enable_logging = true, - .enable_signal_handling = true, - }, - mamba::logging::spdlogimpl::LogHandler_spdlog{} }; + const auto pre_config_options = decide_preconfig_context_options(argc, argv); + mamba::Context ctx{ pre_config_options, decide_log_handler(pre_config_options) }; mamba::Console console{ ctx }; mamba::Configuration config{ ctx }; init_console(); + mamba::on_scope_exit _console_reset{ [] { reset_console(); } }; ctx.command_params.is_mamba_exe = true; @@ -74,7 +124,6 @@ main(int argc, char** argv) if (argc >= 2 && strcmp(argv[1], "completer") == 0) { get_completions(&app, config, argc, utf8argv); - reset_console(); return 0; } @@ -90,15 +139,21 @@ main(int argc, char** argv) ctx.command_params.current_command = full_command.str(); std::optional error_to_report; - auto handle_exception = [&](auto& e) + auto handle_exception = [&](auto& e, const auto&... additional_messages) { - error_to_report = e.what(); + using namespace std::literals; + error_to_report.emplace(e.what()); + (error_to_report->append(additional_messages), ...); set_sig_interrupted(); }; + int return_value = EXIT_SUCCESS; + try { - CLI11_PARSE(app, argc, utf8argv); + // Note: do not use CLI11_PARSE macro as its error handling + // would bypass ours. + app.parse(argc, utf8argv); if (app.get_subcommands().size() == 0) { config.load(); @@ -129,7 +184,6 @@ main(int argc, char** argv) if (is_interruption) { - reset_console(); LOG_WARNING << e.what(); return 0; } @@ -138,18 +192,42 @@ main(int argc, char** argv) handle_exception(e); } } + catch (const CLI::Error& e) + { + using namespace std::literals; + // We only preserve CLI11 output behavior when errors from CLI11 + // occurs because of `--help` or `--version` is used. Otherwise we follow the + // logic that `--json` outputs everything as JSON. + static constexpr std::array non_error_request_names = { "CallForHelp"sv, + "CallForAllHelp"sv, + "CallForVersion"sv }; + const bool is_non_error_request = std::ranges::find(non_error_request_names, e.get_name()) + != non_error_request_names.end(); + + if (ctx.output_params.json and not is_non_error_request) + { + // we want the output to end up in the json log history + std::stringstream output; + return_value = app.exit(e, output, output); + LOG_WARNING << output.str(); + } + else + { + // we don't want any json output even if requested, CLI11 will handle this + console.cancel_json_print(); + return_value = app.exit(e); + } + } catch (const std::exception& e) { handle_exception(e); } - reset_console(); - if (error_to_report) { LOG_CRITICAL << error_to_report.value(); - return 1; // TODO: consider returning EXIT_FAILURE + return_value = EXIT_FAILURE; } - return 0; + return return_value; } diff --git a/micromamba/src/umamba.cpp b/micromamba/src/umamba.cpp index d1ae4753ff..97693b211f 100644 --- a/micromamba/src/umamba.cpp +++ b/micromamba/src/umamba.cpp @@ -31,10 +31,17 @@ set_umamba_command(CLI::App* com, mamba::Configuration& config) context.command_params.caller_version = umamba::version(); - auto print_version = [](int /*count*/) + auto print_version = [&](int /*count*/) { - std::cout << umamba::version() << std::endl; - exit(0); + if (config.context().output_params.json) + { + Console::instance().set_json_output("/version"_json_pointer, umamba::version()); + } + else + { + std::cout << umamba::version() << std::endl; + exit(0); + } }; com->add_flag_function("--version", print_version); diff --git a/micromamba/tests/helpers.py b/micromamba/tests/helpers.py index e129ab1185..2f870748b1 100644 --- a/micromamba/tests/helpers.py +++ b/micromamba/tests/helpers.py @@ -315,7 +315,15 @@ def run_env(*args, f=None, **kwargs): return res.decode() -def umamba_list(*args, **kwargs): +def pkgs_list_from_json_result(json_result): + assert json_result + packages = json_result["packages"] + # TODO: consider if we check here if there was warnings or errors, + # maybe driven by a parameter + return packages + + +def umamba_list(*args, json_as_pkgs_list=True, **kwargs): umamba = get_umamba() cmd = [umamba, "list"] + [str(arg) for arg in args if arg] @@ -323,7 +331,14 @@ def umamba_list(*args, **kwargs): if "--json" in args: j = json.loads(res) - return j + if json_as_pkgs_list: + # TODO: consider checking here that there is no errors + # otherwise any log is lost beyond this point + packages = pkgs_list_from_json_result(j) + assert type(packages) is list + return packages + else: + return j return res.decode() @@ -707,3 +722,11 @@ def assert_state_file(state_file_path: Path, expected_state: dict): assert state[field_name] == expected_value, ( f"Expected {field_name} to be {expected_value}, but got {state[field_name]}" ) + + +def find_message_in_json_logs(json_result, message_to_find): + for log_record in json_result["log_history"]: + if message_to_find in log_record["message"]: + return log_record + + return None diff --git a/micromamba/tests/test_create.py b/micromamba/tests/test_create.py index 7fb3f4401d..c398b9b228 100644 --- a/micromamba/tests/test_create.py +++ b/micromamba/tests/test_create.py @@ -365,6 +365,7 @@ def test_clone_conflicts_with_specs(tmp_home, tmp_root_prefix, tmp_path): env_name = "clone-conflict-specs" helpers.create("-n", "src-env-for-conflict", "xtensor", "--json", no_dry_run=True) + # We expect this run to fail with pytest.raises(subprocess.CalledProcessError) as info: helpers.create( "--clone", @@ -375,8 +376,14 @@ def test_clone_conflicts_with_specs(tmp_home, tmp_root_prefix, tmp_path): "--json", no_dry_run=True, ) - stderr = info.value.stderr.decode() - assert "Cannot use --clone together with package specs." in stderr + json_output = json.loads(info.value.stdout.decode()) + + assert ( + helpers.find_message_in_json_logs( + json_output, "Cannot use --clone together with package specs." + ) + is not None + ) @pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True) @@ -398,9 +405,14 @@ def test_clone_conflicts_with_file(tmp_home, tmp_root_prefix, tmp_path): "--json", no_dry_run=True, ) - stderr = info.value.stderr.decode() + json_output = json.loads(info.value.stdout.decode()) - assert "Cannot use --clone together with package specs." in stderr + assert ( + helpers.find_message_in_json_logs( + json_output, "Cannot use --clone together with package specs." + ) + is not None + ) @pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True) @@ -412,15 +424,23 @@ def test_clone_non_existing_source(tmp_home, tmp_root_prefix, tmp_path): helpers.create( "--clone", "this-env-does-not-exist", "-n", env_name, "--json", no_dry_run=True ) - stderr = info.value.stderr.decode() - assert "Could not find environment to clone: this-env-does-not-exist" in stderr + json_output = json.loads(info.value.stdout.decode()) + assert ( + helpers.find_message_in_json_logs( + json_output, "Could not find environment to clone: this-env-does-not-exist" + ) + is not None + ) # Non-existing prefix path non_existing_prefix = tmp_path / "does-not-exist" with pytest.raises(subprocess.CalledProcessError) as info2: helpers.create("--clone", non_existing_prefix, "-n", env_name, "--json", no_dry_run=True) - stderr2 = info2.value.stderr.decode() - assert f"Source prefix '{non_existing_prefix}" in stderr2 + json_output2 = json.loads(info2.value.stdout.decode()) + assert ( + helpers.find_message_in_json_logs(json_output2, f"Source prefix '{non_existing_prefix}") + is not None + ) @pytest.mark.skipif( @@ -2690,6 +2710,7 @@ def test_create_dry_run_json(tmp_path): "dry_run": True, "prefix": str(env_prefix), "success": True, + "log_history": [], } assert res == expected_output diff --git a/micromamba/tests/test_shell.py b/micromamba/tests/test_shell.py index 24c46b4341..0e96004cf1 100644 --- a/micromamba/tests/test_shell.py +++ b/micromamba/tests/test_shell.py @@ -56,7 +56,7 @@ def test_hook(tmp_home, tmp_root_prefix, shell_type): assert res.count(mamba_exe_posix) == 0 res = helpers.shell("hook", "-s", shell_type, "--json") - expected_keys = {"success", "operation", "context", "actions"} + expected_keys = {"success", "operation", "context", "actions", "log_history"} assert set(res.keys()) == expected_keys assert res["success"] @@ -118,7 +118,7 @@ def custom_shell(shell): res = custom_shell(shell_type) - expected_keys = {"success", "operation", "context", "actions"} + expected_keys = {"success", "operation", "context", "actions", "log_history"} assert set(res.keys()) == expected_keys assert res["success"]