fix: Don't log if `--json` or `--quiet` by Klaim · Pull Request #4202 · mamba-org/mamba

Klaim · 2026-03-19T15:14:52Z

Description

While working on #4126 when checking issues on the CI I regularly hit failing tests where the json output failed to parse because of an error (usually because of #4201) plus the error was logged while --json was used, which should never happen.
This is a known and old issue, mainly visible while PRs are not stabilized yet.

This PR relies on the recently introduced logging system by completely deactivating it when --json or --quiet is parsed or if the related params passed to the Context constructor are true.

If --quiet is used, the logging system is then stopped, which means whatever existing logging handler will be removed, leading to logging records being completely ignored.
If --json is used, a special log-handler is setup to keep track of the log-records which pass the log-level filter (with no maximum count limit) and these records will be added to an array "log_history" as member of the JSON output object.
--json also guarantees that a JSON output will appear even if it's an empty JSON object (when returning without a log, which should be rare).
- This breaks compatibility with previous JSON output as previously some commands resulted in a JSON array being output instead of an object.

Some consequences of the solution implemented in this PR:

using current (micro)mamba will never output any log-record as long as --json or --quiet (this doesnt apply to outputs which are not logs, like the json output);
It is possible for a libmamba user code to inject a new log-handler after processing the output params, which will potentially renabled log-record being output if libmamba's code doesnt properly prevent it because of --json or --quiet; although in the present codebase we never do that and don't expect users to try switching log-handlers after initialization (except for testing).
It is possible to emit logs before reaching the point where the logging system is stopped, that is at Context construction or when --json or --quiet are parsed (we are not doing that in (micro)mamba).

Some alternative solutions we could have used here but rejected for now:

Make the logging system simply ignore all log-records if one of these flags is set, even if a log-handler is installed. This is more drastic, but might be overkill. It also implies some communication between the output and logging systems.
Implement the choice of handling log-records or not depending on these flags in the default log-handler. This solution moves the responsibility into the log-handler but it also is quite fragile if libmamba users want to use another log-handler (python handling for example).

Note: I also fixed an incorrect log-level filter in LogHandler_History which is now revealed by a more complete set of tests.

Type of Change

Bugfix
Feature / enhancement
CI / Documentation
Maintenance

Checklist

My code follows the general style and conventions of the codebase, ensuring consistency
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have run pre-commit run --all locally in the source folder and confirmed that there are no linter errors.
I have added tests that prove my fix is effective or that my feature works
New and existing tests pass locally with my changes

Klaim · 2026-03-19T15:23:49Z

Might fix #3763

codecov · 2026-03-19T15:33:13Z

Codecov Report

❌ Patch coverage is 12.06897% with 153 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.58%. Comparing base (a3c2580) to head (855eb58).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
libmamba/include/mamba/core/logging_tools.hpp	0.00%	46 Missing ⚠️
micromamba/src/main.cpp	0.00%	36 Missing ⚠️
libmamba/src/core/output.cpp	2.94%	33 Missing ⚠️
libmamba/src/api/configuration.cpp	50.00%	7 Missing ⚠️
micromamba/src/env.cpp	0.00%	7 Missing ⚠️
micromamba/src/umamba.cpp	0.00%	5 Missing ⚠️
libmamba/src/api/repoquery.cpp	0.00%	4 Missing ⚠️
libmamba/src/core/context.cpp	55.55%	4 Missing ⚠️
libmamba/include/mamba/core/output.hpp	0.00%	3 Missing ⚠️
libmamba/src/api/create.cpp	0.00%	3 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4202      +/-   ##
==========================================
- Coverage   54.75%   54.58%   -0.18%     
==========================================
  Files         240      240              
  Lines       30145    30254     +109     
  Branches     3214     3234      +20     
==========================================
+ Hits        16505    16513       +8     
- Misses      13637    13738     +101     
  Partials        3        3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Klaim · 2026-03-27T14:30:56Z

The failing test is actually not outputting any json, which suggests that the error is not properly handled with --json.

Klaim · 2026-03-30T13:05:34Z

@JohanMabille @jjerphan

I had to go a bit farther for this pr:

Some commands like (micro)mamba config list --json expects a JSON or YAML output which is generated by some other code than the Console, so when they succeed I had to not output the normal JSON to avoid output 2 JSON objects.
-> I suspect that it would be better to fusion the normal Console json output with the generated one? Otherwise no log history in these cases. If yes, how should I handle the YAML case?
The errors at (micro)mamba arguments parsing step were emitted before switching to the json output mode, as expected because we only know that we will output json after doing that parsing. However that leads to harder to predict output when --json is enabled.
-> I decided to attempt to pre-parse --json and --quiet so that we setup the JSON output (with log history) as soon as possible. This leads to this output at the moment (with a randomly named flag):

{
  "log_history": [
      {
          "level": "critical",
          "location": "E:\\QuantStack\\mamba-origin\\micromamba\\src\\main.cpp:188:9 int __cdecl main(int,char **)",
          "message": "The following argument was not expected: --fgsonsdgniodfsgndfoigndfoigndfo",
          "source": "libmamba"
      }
  ]
}

The missing part is the message to use --help, I can add it. It was injected automatically by CLI11's macro which was completely bypassing our error handling system so I changed that too.
Is this still consistent with the expectation to get JSON output whatever is happening when requestion --json? Or do we expect normal error output on arguments parsing errors? If we do, I can remove the specific commit(s) forcing json.

jjerphan · 2026-03-30T14:15:42Z

-> I suspect that it would be better to fusion the normal Console json output with the generated one? Otherwise no log history in these cases. If yes, how should I handle the YAML case?

Yes, the output should be handle via the Console IMO, and I would convert YAML output to JSON (#4158 once finished should eventually helped).

Is this still consistent with the expectation to get JSON output whatever is happening when requestion --json? Or do we expect normal error output on arguments parsing errors? If we do, I can remove the specific commit(s) forcing json.

To me the behavior that you describe is consistent with what we are expecting.

Klaim · 2026-03-31T15:08:12Z

Yes, the output should be handle via the Console IMO, and I would convert YAML output to JSON (#4158 once finished should eventually helped).

It looks like the YAML output is only when --json is not used, so I think it should be fine.

…elated tests

… warning by default)

…story

jjerphan

LGTM from a functional point of view modulo a few comments and questions.

As discussed privately, there is still some fuzz between directs usage of std::{cout,cerr} and of the Console, but this goes way beyond the scope of this PR.

jjerphan · 2026-06-11T12:47:33Z

We can simplify / fix the explicit environment export in another PR.

I'm not 100% sure I understand what you mean, but to clarify why this file is modified:
this PR is fixing the whole json output so that it all go into the same system and we can properly make sure that no non-json (usually errors) is emitted in that case. The change in this file is necessary to achieve that (along with any other json output code that was not using Console).
Note that the resulting output was not changed, it just goes into the proper system but the output json object is the same (with just the "log_history" field added, but otherwise same).
Or am I missing something/misunderstanding your comment?

I meant that there are still many direct usages of stdout here and some TODO which we can treat later in another PR.

jjerphan · 2026-06-11T13:21:53Z

    assert any(pkg["name"] == "x264" for pkg in res["actions"]["LINK"])


-@pytest.mark.timeout(30)


With the recent changes regarding should_be_emitted, might have logs been reduced? On my machine, it now takes 14.47s to run.

Can we try reverting this?

It's the same issue we see in the rest of the CI so I dont think it has anything to do with any of the changes here?
Reverting this will probably work as long as the issue is random.

I guess I meant that we can ignore this failure and treat it independently so that the timeout does not get increased in this PR.

Yes will revert

jjerphan · 2026-06-11T13:28:11Z

+    mamba::OutputParams output_params;
+    for (const char* arg : std::ranges::subrange(argv, argv + argc))
+    {
+        if (arg == "--json"sv)
+        {
+            output_params.json = true;
+        }
+        else if (arg == "--quiet"sv)
+        {
+            output_params.quiet = true;
+        }
+    }
+
+    if (output_params.json or output_params.quiet)
+    {
+        options.output_params = output_params;
+    }


Nit: Why use an intermediary mamba::OutputParams?

mamba::ContextOptions::output_params is an optional which is empty by default, so here we only set it if one of the parameters is set. The behavior of the program is different if that optional is empty vs not-empty with members set to false. That's why we need to set the optional only if the right condition is met.

For reading clarity I will rewrite this with .emplace.

jjerphan · 2026-06-11T13:34:08Z


        if (this->at("print_config_only").value<bool>())
        {
+            // TODO: fix this for the case where `--json` is used


Should we open an issue which tracks all the TODO introduced in this PR such as this one?

I'll double check but I thinkt his TODO is not up to date, print_dump now outputs to the normal json output, if I'm not mistaken, when --json is used, was not the case before.

jjerphan · 2026-06-11T13:34:53Z

        */
        void cancel_json_print();

+        // FIXME: documentation


Did you mean to introduce a small docstring?

I forgot to add the docstring yes, will add.

JohanMabille · 2026-06-11T14:19:34Z

@@ -67,14 +96,15 @@ namespace mamba::logging
            */
            auto push_if_enabled(LogRecord& record) -> bool


I am still uncomfortabe with this API and the move below. Given that this function is called here and here only, that the LogRecord is passed by value and is not used after the call to push_if_enabled, I think it is safe to accept it by rvalue reference here instead of lvalue reference.

When this function returns false, in both usage we move the record in another container, so I can't just take by r-value-reference without at least returning the object again so that it can be used by the caller, otherwise at the call point it would look like we move the object twice, which is even weirder.

if (not backtrace.push_if_enabled(std::move(record))) { something_else(std::move(record)); // looks like UB }

My goal here is to have this algorithm written only once, as having the same logic in both place lead to some annoying errors while developing. But I also agree it's quite weird api.

I'll try to find another way to achieve that goal with a nicer api. I was thinking maybe something in that direction:

// free function: auto push_to_backtrace_or(Record&& record, BasicBacktrace& backtrace, std::invocable<Record&&> auto or_func); // in call site: push_to_backtrace_or(std::move(record), backtrace, [&](LogRecord&& record){ // do the work if not pushed to backtrace something_else(std::move(record)); });

The passing of the same object to the lambda is a bit weird but maybe less weird than this current api?

I was thinking of having push_to_backtrace returning the log record if not pushed (can be an optional, or a pair<bool, LogRecord>), but that would make the syntax heavier. Passing a lambda as you suggest seems the best option in the end, I don't find it weird to pass the same object to the lambda; and it is not that obvious at the call site.

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

This reverts commit e495113.

… the backtrace is disabled

…cope" :/

JohanMabille

LGTM, the error on Windows is unrelated to this PR. The failure on OSX is flaky (timeout), restarting the job should fix it.

Klaim · 2026-06-16T13:28:46Z

LGTM, the error on Windows is unrelated to this PR. The failure on OSX is flaky (timeout), restarting the job should fix it.

Yes, the OSX timeout was observed in other PRs (also on Linux runs, here and there) and I had doubled the timeout in this pr but we agreed with @jjerphan that it should be treated in another pr.

However, restarting the job is not guaranteed to fix it, it seems to happen often.

jjerphan

LGTM. Can we use conventional commit for this PR title?

Klaim · 2026-06-16T15:00:37Z

LGTM. Can we use conventional commit for this PR title?

Sure.
Although, with reluctance: I don't like how that "conventional" commit naming is reducing things like this pr which is not "just a bugfix" by misleadingly putting it into a narrow category. I'm also not convince that it helps tooling.
But I dont really mind otherwise, will rename👍🏽

jjerphan · 2026-06-16T15:25:57Z

I agree that this the types of commit of conventional commits are a bit too coarse (I find it useful when considering doing a release), but is is this PR doing something else than fixing a wrong behavior of mamba?

Klaim · 2026-06-16T15:33:21Z

I agree that this the types of commit of conventional commits are a bit too coarse (I find it useful when considering doing a release), but is is this PR doing something else than fixing a wrong behavior of mamba?

It does fix the wrong general behavior around the json output but the issue is what that implies, the consequences of the fix:

it changes the output of many --json cases (notably the ones which previously resulting in arrays [ ... ] instead of an object { ... })
it adds loggs where they were not previously, in the json object, which is an important consequence of making sure errors dont appear in json output (we still need them to appear somewhere). Taken alone you could consider this a "feature" in some ways, or an improvement.

I wouldnt call that a feature either so fix is the closest I guess.

Personally for helping with release changelogs (and general communication around releases) in other projects I worke(d) on I tend to prefer distinguishing "changes" from "fixes" only. This means "fixes" are only changing the implementation, not the interfaces, and correcting behaviors. "Changes" are everything else. Some are big, some small, some might or not break the interface or widely change the behavior.
That's just personal preference though.

But really I'm nitpicking and acting like an old man waving his cane to the clouds, don't mind me 🙈 It's not that important :)

jjerphan · 2026-06-16T15:43:11Z

I see and I agree. My assumption is/was that one can split changes into smaller ones fitting the types of conventional commits.

Here "enhancement" (enh) could work?

Anyway, it is not really a big deal.

Klaim · 2026-06-16T15:56:40Z

I see and I agree. My assumption is/was that one can split changes into smaller ones fitting the types of conventional commits.

Yes, IFF it does make sense to split. Not the case here though IMO. The things I pointed are direct consequences of the core changes of the pr, not really separable except if we accept weird situations where main commits are allowed to not actually working correctly/pass all tests, which AFAIK we don't.

(to clarify, the only things that could be separated here is the improvement + fixes on LogHandler_History etc., which had some issues, but that doesnt change my point about the consequences which stem from the core changes)

Here "enhancement" (enh) could work?

If we could I would say it's both fix and enhancement. My understanding is that we cannot set both?

jjerphan · 2026-06-17T07:16:04Z

To me, fixes are one kind of enhancements which themselves are more generic (enhancements as improvements of existing aspect of a project).

github-actions Bot added the release::bug_fixes For PRs fixing bugs label Mar 19, 2026

Klaim mentioned this pull request Mar 19, 2026

[micromamba] Using --json for micromamba create eats up all error messages #1945

Open

Klaim force-pushed the no-logging-json-quiet branch from d8e6bdd to 91e8d11 Compare March 25, 2026 18:22

Klaim force-pushed the no-logging-json-quiet branch from 475f7cd to c42b253 Compare March 30, 2026 10:06

Klaim force-pushed the no-logging-json-quiet branch from d07bc8e to c68cff6 Compare March 31, 2026 15:02

Klaim mentioned this pull request Apr 3, 2026

config sub-command output hides secrets #4223

Merged

11 tasks

Klaim force-pushed the no-logging-json-quiet branch 3 times, most recently from ae08936 to d7a5aab Compare April 8, 2026 15:59

Klaim force-pushed the no-logging-json-quiet branch 3 times, most recently from da0d44a to 0ac5416 Compare April 17, 2026 11:21

Klaim force-pushed the no-logging-json-quiet branch from 0ac5416 to 5e50a61 Compare April 22, 2026 09:41

Klaim force-pushed the no-logging-json-quiet branch from 5e50a61 to f413d1c Compare May 4, 2026 09:01

Klaim added 10 commits May 11, 2026 16:50

Don't log if --json or --quiet

d9ac264

fixed LogHandler_History incorrect filtering of log records + added r…

243d928

…elated tests

improved logging tests

bb89645

json output will contain log records passing the log level filter (>=…

58dc097

… warning by default)

attempt to get some info from tests

9ca3a20

fixed test expecting an error in cerr, looking into json instead

22a1177

LogHandler_History allows capture with clear

fe59cb8

output a potentially empty json if --json is used even if no log hi…

5515fb1

…story

console must capture and output json after error logging

f529c23

(micro)mamba always reset the console whatever the way it returns

dee6f12

Klaim marked this pull request as ready for review June 9, 2026 13:51

jjerphan reviewed Jun 9, 2026

View reviewed changes

Klaim added 3 commits June 9, 2026 17:33

removed unclear comment

240d53d

improved log record filtering clarity (addressing reviews)

daeb4ee

Merge branch 'main' into no-logging-json-quiet

3e21106

jjerphan reviewed Jun 11, 2026

View reviewed changes

JohanMabille reviewed Jun 11, 2026

View reviewed changes

Klaim and others added 8 commits June 11, 2026 18:25

fix typo

da56561

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

fix typo

2146be8

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Revert "increased probably too big timeout"

3b3ebae

This reverts commit e495113.

rewrote basic_backtrace::push_if_enabled to take an invocable used if…

ace8813

… the backtrace is disabled

(micro)mamba: rewrote options intiialization for clarity

395cbcb

removed comments that is not up to date

05c46ac

appease warnings that assumes developers ignores of the concept of "s…

ba8cfd5

…cope" :/

formatting

fa0f430

JohanMabille approved these changes Jun 16, 2026

View reviewed changes

jjerphan approved these changes Jun 16, 2026

View reviewed changes

Klaim changed the title ~~Don't log if --json or --quiet~~ fix: Don't log if --json or --quiet Jun 16, 2026

Merge remote-tracking branch 'origin/main' into no-logging-json-quiet

fd5a8e8

Merge branch 'main' into no-logging-json-quiet

855eb58

Klaim merged commit 1cdb572 into mamba-org:main Jun 17, 2026
34 of 36 checks passed

Klaim mentioned this pull request Jun 17, 2026

Micromamba is not silent on errors even when you pass --quiet #3763

Open

3 tasks

Hind-M mentioned this pull request Jun 18, 2026

Handle pre/post-link/unlink scripts #4313

Merged

11 tasks

		assert any(pkg["name"] == "x264" for pkg in res["actions"]["LINK"])


		@pytest.mark.timeout(30)

		@@ -67,14 +96,15 @@ namespace mamba::logging
		*/
		auto push_if_enabled(LogRecord& record) -> bool

Uh oh!

Conversation

Klaim commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Checklist

Uh oh!

Klaim commented Mar 19, 2026

Uh oh!

codecov Bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Klaim commented Mar 27, 2026

Uh oh!

Klaim commented Mar 30, 2026

Uh oh!

jjerphan commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Klaim commented Mar 31, 2026

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Klaim Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohanMabille Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohanMabille left a comment

Choose a reason for hiding this comment

Uh oh!

Klaim commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

Klaim commented Jun 16, 2026

Klaim commented Mar 19, 2026 •

edited

Loading

codecov Bot commented Mar 19, 2026 •

edited

Loading

jjerphan commented Mar 30, 2026 •

edited

Loading

Klaim Jun 16, 2026 •

edited

Loading

JohanMabille Jun 11, 2026 •

edited

Loading

Klaim commented Jun 16, 2026 •

edited

Loading

Klaim commented Jun 16, 2026 •

edited

Loading

jjerphan commented Jun 16, 2026 •

edited

Loading

Klaim commented Jun 16, 2026 •

edited

Loading