Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix some log levels and reduce (debug) log spam #1690

Merged
merged 7 commits into from
Jun 3, 2018
Merged

Fix some log levels and reduce (debug) log spam #1690

merged 7 commits into from
Jun 3, 2018

Conversation

uklotzde
Copy link
Contributor

While testing I noticed that we still write too many debug logs that should only be written while debug logging is enabled explicitly.

I've divided the modifications into commits with appropriate commit messages for easy verification.

@uklotzde uklotzde added this to the 2.1.1 milestone May 27, 2018
daschuer
daschuer reviewed May 27, 2018
View reviewed changes
src/sources/metadatasourcetaglib.cpp Outdated
<< "into file" << m_fileName
<< "with type" << m_fileType;
if (kLogger.debugEnabled()) {
kLogger.debug() << "Exporting track metadata"
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if the file name is logged somewhere else as well. If not this one is important to identify a faulty file that might be the cause for a taglib crash.

daschuer
daschuer reviewed May 27, 2018
View reviewed changes
src/sources/soundsourceproxy.cpp Outdated
<< openMode;
if (kLogger.debugEnabled()) {
kLogger.debug() << "Opening file"
<< getUrl().toString()
Copy link
Member

Choose a reason for hiding this comment

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

This one can be also a helper for identifying a crashing file.

@daschuer
Copy link
Member

I think we should keep at least the file related messages to identify a faulty file.

@uklotzde
Copy link
Contributor Author

Ok. I've re-enabled the two file related logs and also added comments why we decided do this.

@daschuer
Copy link
Member

Thank you. Unfortunately Mac build fails

Undefined symbols for architecture x86_64:
  "mixxx::Logging::s_logLevel", referenced from:
      mixxx::Logging::enabled(mixxx::LogLevel) in metadatasourcetaglib.os
      mixxx::Logging::enabled(mixxx::LogLevel) in trackmetadatataglib.os
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
scons: *** [osx64_build/plugins/osx64_build/m4a/libsoundsourcem4a.dylib] Error 1
scons: building terminated because of errors.

@uklotzde
Copy link
Contributor Author

Well, I know that our logging code is broken in the context of plugins. It's simply not a good idea to share code with static global variables. I already noticed that the correct log level is not reflected in the plugins. It was just a matter of time until this breaks.

@daschuer
Copy link
Member

daschuer commented Jun 1, 2018

Do you have a idea to fix the builds soon, or should we remove this from the 2.1.1 milestone?

@uklotzde uklotzde changed the title Fix some log levels and reduce (debug) log spam [WiP] Fix some log levels and reduce (debug) log spam Jun 2, 2018
uklotzde added 2 commits June 2, 2018 19:33
@uklotzde
Merge branch '2.1' into 2.1_reduce_log_spam
4a5596c
@uklotzde
Fix propagation of log levels into plugins with an ugly hack
2442f53 
The configured log levels have to be propagated into the plugins
upon initialization! This was already broken before I submitted
the previous changes that finally broke the plugin build on macOS.
@daschuer
Copy link
Member

daschuer commented Jun 2, 2018

LGTM, Is this still [WIP]?

@uklotzde
Copy link
Contributor Author

uklotzde commented Jun 2, 2018

No, I just wanted to await the build results. Building locally with clang on Linux worked, I was not able to reproduce the linker error.

Unfortunately I had to bump the API version. This is a really ugly hack and only a partial fix. But we can't fix the whole logging with its global state variables that don't get initialized properly in the plugins. We really need to get rid of the plugins.

@uklotzde uklotzde changed the title [WiP] Fix some log levels and reduce (debug) log spam Fix some log levels and reduce (debug) log spam Jun 2, 2018
@daschuer
Copy link
Member

daschuer commented Jun 2, 2018

OK, the CI fails unrelated. Merge?

@uklotzde
Copy link
Contributor Author

uklotzde commented Jun 2, 2018

There is still a small chance that the Windows build might fail. Although according to my understanding this new version should at least be less broken than before ;)

@uklotzde uklotzde changed the title Fix some log levels and reduce (debug) log spam [WiP] Fix some log levels and reduce (debug) log spam Jun 2, 2018
@uklotzde
Copy link
Contributor Author

uklotzde commented Jun 2, 2018 •
edited
Loading

I need to test it again. Currently the plugins fail to load even on Linux!?! Looks like I messed up my local branches. Plugins are loading successfully.

@uklotzde uklotzde changed the title [WiP] Fix some log levels and reduce (debug) log spam Fix some log levels and reduce (debug) log spam Jun 2, 2018
ninomp
ninomp reviewed Jun 2, 2018
View reviewed changes
src/sources/soundsourcepluginapi.h Outdated
@@ -3,6 +3,7 @@

#define MIXXX_SOUNDSOURCEPLUGINAPI_VERSION 17
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be 18?

@uklotzde
Copy link
Contributor Author

uklotzde commented Jun 2, 2018

@ninomp Thanks for the hint. Not the first time that this happened.

@daschuer
Copy link
Member

daschuer commented Jun 3, 2018

Ok then let's merge it, and see what happens.

@daschuer daschuer merged commit 55b4f3e into mixxxdj:2.1 Jun 3, 2018
@daschuer
Copy link
Member

daschuer commented Jun 3, 2018

This was one of the last 2.1.1 PRs.
Would you mind to look into: #1691?

@uklotzde uklotzde deleted the 2.1_reduce_log_spam branch July 8, 2018 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ninomp ninomp ninomp left review comments

@daschuer daschuer daschuer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.1
Development

Successfully merging this pull request may close these issues.
3 participants
@uklotzde @daschuer @ninomp