-
Notifications
You must be signed in to change notification settings - Fork 86
chore: bump chronicles to 0.12 #18553
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
Conversation
https://github.com/status-im/nim-chronicles/releases/tag/v0.12.0 less memory and CPU usage - more runtime config options
Jenkins BuildsClick to see older builds (9)
|
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.
LGTM, just seems to be breaking the resident NIM tests
Turns out the tests didn't initialize logging which, due to a regression in support for uninitialized log streams, crashes: status-im/nim-chronicles#181 |
switch("define", "chronicles_sinks=textlines[stdout],textlines[nocolors,dynamic],textlines[file,nocolors]") | ||
switch("define", "chronicles_runtime_filtering=on") | ||
switch("define", "chronicles_default_output_device=dynamic") | ||
switch("define", "chronicles_log_level=trace") |
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.
Just wondering whether this will propagate correctly to the mobile builds @alexjba ?
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.
config.nims
gets picked up for all nim-related builds that happen in this folder, so mobile builds in particular should not be special this way - when the build system was set up, NIMFLAGS
was intended for command-line overrides and other "local" preferences to the build (much like CFLAGS
) while config.nims
represents the "permanent" compile-time configuration of the application.
The biggest difference is that with these flags inside config.nims
, when you "manually" build things with nim c
without using the makefile, you'll get a consistent logging experience (ie when you build and run a single unit test file).
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.
we've had some issues with chronicles on android and we'll need to go back to it later on. It cannot stdout
. It needs to use a similar implementation as echo
and import the android headers. We're currently using syslog
sink as a workaround.
https://github.com/status-im/status-desktop/blob/master/mobile/scripts/buildNimStatusClient.sh#L24
But I'm totally for this change. I see no reason not to use the config.nims
. We can add an additional sink for Android if needed until we'll fix chronicles.
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.
command line
Ah, command line should override what's in config.nims
, so it should be safe that way as well - though for long-term, it would turn into a when(android)
section in config.nims
proper.
https://github.com/status-im/nim-chronicles/releases/tag/v0.12.0
less memory and CPU usage - more runtime config options