Skip to content

[core] Add ROOT_LOG env var support for RLogger#21455

Open
deveshbervar wants to merge 6 commits intoroot-project:masterfrom
deveshbervar:feature/rlogger-env-var
Open

[core] Add ROOT_LOG env var support for RLogger#21455
deveshbervar wants to merge 6 commits intoroot-project:masterfrom
deveshbervar:feature/rlogger-env-var

Conversation

@deveshbervar
Copy link

Why

Enables per-channel verbosity to be configured at startup via an
environment variable, without recompiling ROOT. This allows tracing
(e.g. CMS autoparsing after #7609) by simply setting ROOT_LOG.

What

Setting ROOT_LOG at startup now configures RLogger channel verbosity:

export ROOT_LOG='ROOT.InterpreterPerf=Debug(3),ROOT.RBrowser=Error'

Supported levels: Fatal, Error, Warning, Info, Debug, Debug(N).
gDebug is also applied as a global verbosity floor.

Changes

  • RLogger.hxx: added fEnvVerbosity map to RLogManager, added
    GetEnvVerbosity(), updated GetEffectiveVerbosity() to check
    per-channel env overrides before falling back to global verbosity
  • RLogger.cxx: constructor now parses ROOT_LOG and applies gDebug

Tests

Since RLogManager is a static singleton initialized once at process
startup, setenv() mid-test does not affect it. I would appreciate
guidance from the maintainer on the recommended approach for testing
this feature. Happy to add tests based on your feedback.

@deveshbervar
Copy link
Author

@dpiparo ping! PR ready for review. I have implemented the ROOT_LOG
environment variable support as described in the issue. Please let me
know if any changes are needed, especially regarding the test approach
for the singleton RLogManager. Thank you!

@dpiparo
Copy link
Member

dpiparo commented Mar 3, 2026

Is testing perhaps still missing?

@deveshbervar
Copy link
Author

Hi @dpiparo, tests have been added in core/foundation/test/RLoggerEnvVar.cxx.

Since RLogManager is a static singleton that parses ROOT_LOG once at
process startup, I used CMake's set_tests_properties ENVIRONMENT to
set ROOT_LOG before the test process starts, rather than setenv() mid-test.

Please let me know if this approach is acceptable or if you'd prefer
a different testing strategy. Thank you!

Comment on lines -88 to -94
// Is there a specific level for the channel? If so, take that,
// overruling the global one.
if (channel->GetEffectiveVerbosity(*this) < entry.fLevel)
return true;

// Lock-protected extraction of handlers, such that they don't get added during the
// handler iteration.
Copy link
Member

@pcanal pcanal Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are those code comments no longer relevant/helpful?

@deveshbervar
Copy link
Author

Hi @pcanal,
Sorry, those comments were accidentally removed during editing.
Restored them now.

@deveshbervar deveshbervar requested a review from bellenot as a code owner March 5, 2026 15:05
@deveshbervar
Copy link
Author

Thank you @silverweed for the detailed review! I have addressed all the points:

  • Restored the missing comment in RLogHandlerDefault
  • Changed TrimWhitespace to use string_view to avoid copies
  • Added Warning() calls for bad Debug(N) parsing and unrecognised levels
  • Changed channelName and levelStr to string_view
  • Added RLoggerEnvVar test to CMakeLists.txt

Regarding the roottest integration test suggestion - I would appreciate
guidance on the roottest structure if that is preferred over the current
unit test approach. Happy to add it.

@silverweed
Copy link
Contributor

Thanks for the changes. Maybe a roottest integration test is not needed since ROOT_ADD_GTEST supports passing the environment...
Could you please add another test case where you explicitly set the verbosity and see that it takes precedence over the environment one?

@deveshbervar
Copy link
Author

Hi @silverweed, I Added a test case that sets the channel verbosity explicitly to kInfo
while ROOT_LOG has it set to Error, and verifies the explicit setting
takes precedence. Thank you for the suggestion!

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 40d8a5f.

@silverweed
Copy link
Contributor

silverweed commented Mar 6, 2026

@deveshbervar all CI builds failed in the configuration step: did you test your changes locally before updating the PR?

@deveshbervar
Copy link
Author

Hi @silverweed, Sorry for not catching this before pushing.

Fixed the clang-format issues and string_view conversion errors.
Verified that the project builds successfully locally before pushing this update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants