-
Notifications
You must be signed in to change notification settings - Fork 284
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
Fix logger level defaulting to TRACE in CMake #1212
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #1212 +/- ##
========================================
- Coverage 82.9% 82.8% -0.2%
========================================
Files 82 81 -1
Lines 14647 14564 -83
========================================
- Hits 12153 12061 -92
- Misses 2494 2503 +9
Continue to review full report at Codecov.
|
45f6644
to
7680574
Compare
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @nurupo)
CMakeLists.txt, line 105 at r2 (raw file):
add_definitions(-DMIN_LOGGER_LEVEL=LOGGER_LEVEL_${MIN_LOGGER_LEVEL}) else() message(FATAL_ERROR "Unknown value provided for MIN_LOGGER_LEVEL: \"${MIN_LOGGER_LEVEL}\", must be one of TRACE, DEBUG, INFO, WARNING or ERROR")
If you don't add this, you save 8 lines of cmake code and the (arguably small) maintenance burden of keeping the log levels here in sync with the ones in logger.h, and avoid increasing duplication of this enumeration by 2.
Right, it will fail to compile if user specifies the wrong logger level, but it's clearer if CMake catches that before compiling and tells what the issue in plain English, rather than deciphering compile error. Also, how often are we planning on modifying the logger level enum anyway for it to be a burden?
vs
|
There, added a small script that regexes out logger levels and diffs them. |
66fe264
to
67320b8
Compare
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.
Reviewed 6 of 7 files at r2, 3 of 3 files at r3.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @nurupo)
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.
Reviewable status: complete! 2 of 1 approvals obtained
@nurupo lgtm. |
Feel free to merge when ready. |
That way CMake's behavior matches what autotools does -- letting toxcore/logger.h handle the default case.
67320b8
to
56432a4
Compare
Is ready, please merge. Just rebased on master. I can squash first two commits if desired. I'd like to keep the 2nd one and 3rd one separate though. |
Apparently we defaulted to TRACE logger level for v0.2.4, v0.2.5, v0.2.6 and v0.2.7 releases when using CMake. We default to INFO logger level when using autotools, since autotools doesn't set any and
logger.h
defaults to INFO if the level is undefined.There is also this gem which is completely incorrect, logger level won't be set to DEBUG in this case, it will default to whatever
logger.h
defaults to -- INFO.c-toxcore/configure.ac
Line 85 in 36f0caa
This change is