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.

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

Integrate spdlog #148

Closed
wants to merge 2 commits into from
Closed

Conversation

baconpaul
Copy link
Collaborator

As mentioned in #94 I banged together a small logging class which helped me a lot with getting AU running. After chatting with @jsakkine and @asimilon about how to get a useful logger, we thought about integrating someone else's written fast C++ logger.

I chose spdlog https://github.com/gabime/spdlog

With this PR, spdlog works and you can use it anywhere if you #include "SurgeLogger.h" and then use their native spdlog::info() or what not.

And I gotta say, spdlog works great. I accidentally put a log into the render loop on every process block while debugging #146 and while my log file got big, my sound didn't waver.

@jsakkine @asimilon @kzantow and @kurasu interested in review or opinion from any of you. Especially note I added a new git submodule. I hope you like it, because wow, this is super handy for me squashing bugs.

build-osx.sh Show resolved Hide resolved
src/au/aulayer.cpp Outdated Show resolved Hide resolved
@jarkkojs
Copy link
Collaborator

jarkkojs commented Dec 26, 2018

I gave some :D Many of them are bigger than just this commit. Want to get a better process and conventions nailed down. Whatever are the conclusions I can update README.md to document them. It will make writing new code easier, not harder. Less thinking. Should anyway update the release process as I promised...

@baconpaul
Copy link
Collaborator Author

OK @jsakkine I figured out how to squash commits. That's really super duper handy. I also integrated all your comments except the typename. If you get a chance, another look would be appreciated!

@jarkkojs
Copy link
Collaborator

jarkkojs commented Dec 26, 2018

Looked into your update. Only one remark. What was your take on indentation BTW?

@baconpaul
Copy link
Collaborator Author

baconpaul commented Dec 26, 2018

Looking into your update. What was your take on indentation BTW?

I went to 4 spaces no tabs and set emacs to use Linux-style rather than gnu-style brace positioning which roughly matches what Xcode does by default too. I changed if( you_need_space ) to if(you_need_space) but muscle memory will have me getting that one wrong forever I fear.

And I chose to not indent the first-level namespace. So

Namespace foo
{

Class don’t indent
{
}

Namespace bar
{ 
    Class in_foo_bar gets indented one
}.

Which was in the standard you circulated earlier.

src/au/aulayer.cpp Outdated Show resolved Hide resolved
To allow us to have easy scalable logging available while developing and
for error reporting and the like, I integrated spdlog. See issue surge-synthesizer#94 for
further conversation on this decision. The logger initializes to
write to stderr and a file (on linux and mac) both and registers
a default logger at dll load time.
.gitignore Outdated Show resolved Hide resolved
build-osx.sh Outdated Show resolved Hide resolved
#include "spdlog/logger.h"
#include "spdlog/async.h"
#include <memory>
#include <filesystem.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be "filesystem.h" ?

#if MAC
char lfb[ PATH_MAX ];
sprintf(lfb, "%s/Library/Logs/Surge.log", getenv("HOME"));
auto fileSink = make_shared<spdlog::sinks::basic_file_sink_mt> (lfb);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be opt-in instead of always on? Is the log always appended to - is there some auto-truncation size? What happens when multiple processes might be accessing the same log? Seems to me logging to the console by default is much preferred and opting in to dump to a log if necessary is a nicer default behavior. Just my 2c.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I had any idea at all what happened to stderr or stdout ejected from a plugin in logic, au lab, and hosting au, I would be fine to skip this. Do you know?

That said: SPD has an auto rotating log. I could use that. Or I could comment this section out and just leave it on in my development area inside an #ifdef log-to-file or some such.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@baconpaul can you run Logic from the command line? If I run Reaper, Live, and Bitwig from command line, I get all the stdout dumped to the terminal; don't have Logic to see if it's the same, but I assume so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(an aside: I usually set up REAPER as the app to run in XCode with a project that just loads the plugin and opens it by default, in this case, the terminal output is just dumped to the XCode console)

@baconpaul
Copy link
Collaborator Author

Huh. Yeah of course that works. I am simply amazed I didn't think of that. Like really amazed.

Well that's mortally embarrassing. None of this code is needed. I'm going to close this PR and delete the code. I can just use printf when I'm debugging.

Sorry for the noise.

@baconpaul baconpaul closed this Dec 27, 2018
@kzantow
Copy link
Collaborator

kzantow commented Dec 27, 2018

@baconpaul now we can argue about tabs vs. spaces and such extremely important programming topics ;)

@baconpaul
Copy link
Collaborator Author

@kzantow. Sure! spaces.
Oh, and emacs.

@baconpaul baconpaul deleted the logging-94 branch December 27, 2018 02:24
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