Merge lp:~dobey/unity-api/add-simple-logger into lp:unity-api

[dobey]

Add the simple logging wrapper around glib logging API.

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:278
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/167/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4785/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4813
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4636/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4636
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4636/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4636/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4636
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4636/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4636/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4636
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4636/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/167/rebuild

review: Needs Fixing (continuous-integration)

[lp:~dobey/unity-api/add-simple-logger]

279. By dobey on 2017-03-30

Not actually using gmock here.

[Unity8 CI Bot (unity8-ci-bot)]

PASSED: Continuous integration, rev:279
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/168/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/4787
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4815
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4638/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4638
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4638/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/168/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

This doesn't strike me as the correct approach. All the overloads of operator<<() shouldn't be necessary. Why not use an approach more like the Logger class in scopes-api?

It uses a LogStream class that derives from std::ostringstream and stores a reference to the actual ostream to write to. That makes it unnecessary to overload operator<<(), and all the usual mechanisms for allowing user-defined types to be inserted can still be used (such as adding an overloaded operator<<() for a custom type to namespace std).

LogStream can only be move constructed, so there are no life cycle issues with the reference to the real ostream.

I would do away with the whole maybe_space() thing altogether. I find it a *huge* pain in the butt that qDebug() and its ilk insist on adding spaces all the time. More often than not, when cobbling messages together, I end up having to use nospace() (and noquote()) all the time to stop it from messing with my output.

I'd *much* prefer having something that behaves just like a normal ostream. With that, I can also use manipulators to change formatting, and use all the other good things that come with ostreams.

Other niceties, such as adding a time stamp, are possible that way too.

The macros for debug(), warn(), etc. truly suck. Please let's not do this. They tear right through all the namespaces and can cause truly strange errors due to name clashes with something else called "debug" elsewhere (which is quite likely). We can just return a LogStream by value from a function instead (and the implementation of the function can tie it to the correct output stream).

It's not clear to me that this should be a header-only implementation. I'm with you on the desire for low overhead. But this is a heavy-weight implementation already because each and every message that is written hits the heap (and ostreams are not light-weight either).

In the interest of ABI stability, I strongly suspect that it would be better to pimpl the entire implementation. In terms of performance, it will not make an iota of difference, but it'll shield us from problems down the track in terms of ABI stability and applications not picking up new features until they are recompiled (as is the case for header-only).

review: Needs Fixing

[lp:~dobey/unity-api/add-simple-logger]

280. By dobey on 2017-04-03

Move some code into C++ and greatly simplify the implementation.

[dobey (dobey)]

Well, I made the Logger class an ostringstream based class, got rid of all the operator overrides, moved the ctor/dtor bits into a compiled file, and made it much simpler.

However, the macros have to be the way they are, because of the dependency on the G_LOG_DOMAIN value being defined at compile time. I wasn't able to get around this yet, but am open to suggestions. The goal with this was to have something as simple to use as qDebug() but which uses the g_log functionality, so we automatically get the G_DEBUG_MESSAGES env var handling at runtime, as a means of being consistent across the platform.

I unfortunately am also not able to get the output buffer overriding working right in the tests. It seems to work fine for the debug() case, but fails for all the other cases, regardless of how I order them.

I looked at what's in unity-scopes-api and it seems a bit complex for what I was wanting to do here.

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:280
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/170/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4851/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4879
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4690/console
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4690/console

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity-api-ci/170/rebuild

review: Needs Fixing (continuous-integration)

[Michi Henning (michihenning)]

I don't see the need for macros at all. The G_LOG_DOMAIN seems to be a red herring. The underlying g_log() function accepts the domain as a parameter. As far as I can see, G_LOG_DOMAIN is used only by the g_error() and similar macros so you don't have to provide the domain at every point where a log message is produced.

We can do the same thing by having the logger instance hang onto the domain and supply it when a message is written with g_log(). No need for a macro that way.

We can't have the implementation without using a pimpl. It needs to be split into a public and internal API, otherwise we can't change the implementation without breaking ABI. The only data member of the Logger (and the LogWriter, see below) should be a unique_ptr to the implementation in the internal namespace.

Here is how I would approach it:

class Logger
{
public:
    Logger(std::string const& domain);

    Logger(Logger const&);
    Logger(Logger&&);
    Logger& operator=(Logger const&);
    Logger& operator=(Logger&&);
    virtual ~Logger();

    // ...
};

That's a well-behaved class with the big five taken care of. It does *not* derive from ostringstream. It simply is a class that remembers its domain in a data member (of the internal Impl class).

Now add a method to read the domain (for completeness) and a method that writes a log message:

class Logger
{
public:
    // ...

    std::string domain() const;
    LogWriter operator()(LogLevel l);
};

With that, I can write:

Logger log("mydomain");
log(LogLevel::debug) << "hello";

To make things a little more convenient, we can add these:

class Logger
{
public:
    // ...

    LogWriter operator()(); // Synonym for debug()
    LogWriter debug();
    LogWriter info();
    LogWriter message();
    LogWriter warning();
    LogWriter critical();
    LogWriter error();
};

Now I can write:

Logger log("mydomain");
log() << "debug";
log.warning() << "warning";

The LogWriter class takes care of writing the actual log message (note the derivation from ostringstream). I've omitted the pimpl here for brevity. The only data member should be a unique_ptr<internal::LogWriterImpl>; the two data members here should be part of LogWriterImpl:

class LogWriter : public std::ostringstream
{
public:
    LogWriter(LogWriter&&);
    LogWriter& operator=(LogWriter&&) = delete;
    ~LogWriter();

private:
    LogWriter(std::string const& domain);

    // There should be a unique_ptr<internal::LogWriteImpl> here instead.
    // This is only for exposition:
    std::string domain_;
    std::ostringstream str_;

    friend class Logger;
};

The LogWriter can be constructed with a domain only by the Logger. The API client can only move-construct a LogWriter, which enforces that the only way to use it is via the methods on Logger that return a LogWriter.

The LogWriter move constructor looks like this:

LogWriter::LogWriter(LogWriter&& other)
    : ostringstream(std::move(other))
    , domain_(other.domain_)
{
}

The destructor writes the message. Here, I'm just writing to clog, but the method could write using g_log() instead just as easily, passing in the domain:

LogWriter::~LogWriter()
{
    string msg = str();
    if (!msg.empty(...

review: Needs Fixing

Unmerged revisions

280. By dobey on 2017-04-03

Move some code into C++ and greatly simplify the implementation.

279. By dobey on 2017-03-30

Not actually using gmock here.

278. By dobey on 2017-03-30

Add the simple logging wrapper around glib logging API.

=== added file 'include/unity/util/Logger.h'
--- include/unity/util/Logger.h	1970-01-01 00:00:00 +0000
+++ include/unity/util/Logger.h	2017-04-03 21:10:57 +0000
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2015-2017 Canonical Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of version 3 of the GNU Lesser General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#pragma once
+
+#include <sstream>
+
+namespace unity
+{
+
+namespace util
+{
+
+    class Logger: public std::ostringstream
+    {
+    public:
+        enum class Level {
+            DEBUG = 0,
+            INFO,
+            MESSAGE,
+            WARNING,
+            CRITICAL,
+            ERROR,
+        };
+
+        Logger(Logger::Level level, const std::string& domain);
+        Logger(Logger&& logger);
+
+        virtual ~Logger();
+
+    private:
+        Level _level;
+        std::string _domain;
+    };
+
+// These are the APIs to be used for logging.
+#define debug() Logger(Logger::Level::DEBUG, G_LOG_DOMAIN)
+#define info() Logger(Logger::Level::INFO, G_LOG_DOMAIN)
+#define warn() Logger(Logger::Level::WARNING, G_LOG_DOMAIN)
+#define critical() Logger(Logger::Level::CRITICAL, G_LOG_DOMAIN)
+#define error() Logger(Logger::Level::ERROR, G_LOG_DOMAIN)
+
+} // namespace util
+
+} // namespace unity

=== modified file 'src/unity/util/CMakeLists.txt'
--- src/unity/util/CMakeLists.txt	2017-04-03 21:10:57 +0000
+++ src/unity/util/CMakeLists.txt	2017-04-03 21:10:57 +0000
@@ -4,6 +4,7 @@
     ${CMAKE_CURRENT_SOURCE_DIR}/Daemon.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/FileIO.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/IniParser.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/Logger.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/SnapPath.cpp
 )
 

=== added file 'src/unity/util/Logger.cpp'
--- src/unity/util/Logger.cpp	1970-01-01 00:00:00 +0000
+++ src/unity/util/Logger.cpp	2017-04-03 21:10:57 +0000
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2017 Canonical Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of version 3 of the GNU Lesser General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#define G_LOG_DOMAIN nullptr
+
+#include "unity/util/Logger.h"
+
+#include <glib.h>
+
+namespace unity
+{
+
+namespace util
+{
+
+Logger::Logger(Logger::Level level, const std::string& domain):
+    std::ostringstream(""),
+    _level(level),
+    _domain(domain)
+{
+}
+
+Logger::Logger(Logger&& logger):
+    std::ostringstream(std::move(logger)),
+    _level(logger._level)
+{
+}
+
+Logger::~Logger()
+{
+    GLogLevelFlags glib_level;
+    auto message = str();
+
+    if (!message.empty()) {
+        switch (_level) {
+        case Logger::Level::DEBUG:
+            glib_level = G_LOG_LEVEL_DEBUG;
+            break;
+        case Logger::Level::INFO:
+            glib_level = G_LOG_LEVEL_INFO;
+            break;
+        case Logger::Level::MESSAGE:
+            glib_level = G_LOG_LEVEL_MESSAGE;
+            break;
+        case Logger::Level::WARNING:
+            glib_level = G_LOG_LEVEL_WARNING;
+            break;
+        case Logger::Level::CRITICAL:
+            glib_level = G_LOG_LEVEL_CRITICAL;
+            break;
+        case Logger::Level::ERROR:
+            glib_level = G_LOG_LEVEL_ERROR;
+            break;
+        }
+        g_log(_domain.empty() ? nullptr : _domain.c_str(),
+              glib_level, "%s", message.c_str());
+    }
+}
+
+} // namespace util
+
+} // namespace unity

=== modified file 'test/gtest/unity/util/CMakeLists.txt'
--- test/gtest/unity/util/CMakeLists.txt	2017-04-03 21:10:57 +0000
+++ test/gtest/unity/util/CMakeLists.txt	2017-04-03 21:10:57 +0000
@@ -4,6 +4,7 @@
 add_subdirectory(GlibMemory)
 add_subdirectory(GObjectMemory)
 add_subdirectory(IniParser)
+add_subdirectory(Logger)
 add_subdirectory(ResourcePtr)
 add_subdirectory(SnapPath)
 add_subdirectory(internal)

=== added directory 'test/gtest/unity/util/Logger'
=== added file 'test/gtest/unity/util/Logger/CMakeLists.txt'
--- test/gtest/unity/util/Logger/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ test/gtest/unity/util/Logger/CMakeLists.txt	2017-04-03 21:10:57 +0000
@@ -0,0 +1,19 @@
+find_package(Threads)
+find_package (PkgConfig REQUIRED)
+pkg_check_modules(GLIB REQUIRED glib-2.0)
+add_definitions(
+  ${GLIB_CLFAGS}
+  ${GLIB_CFLAGS_OTHER}
+  -DG_LOG_DOMAIN="Logger_test"
+)
+include_directories(${GLIB_INCLUDE_DIRS})
+
+add_executable(Logger_test Logger_test.cpp)
+target_link_libraries(Logger_test
+  ${LIBS}
+  ${TESTLIBS}
+  ${GLIB_LDFLAGS}
+  ${CMAKE_THREAD_LIBS_INIT}
+)
+
+add_test(Logger Logger_test)

=== added file 'test/gtest/unity/util/Logger/Logger_test.cpp'
--- test/gtest/unity/util/Logger/Logger_test.cpp	1970-01-01 00:00:00 +0000
+++ test/gtest/unity/util/Logger/Logger_test.cpp	2017-04-03 21:10:57 +0000
@@ -0,0 +1,105 @@
+/*
+ * Copyright 2015-2017 Canonical Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of version 3 of the GNU Lesser General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include <unity/util/Logger.h>
+
+#include <glib.h>
+#include <gtest/gtest.h>
+#include <unistd.h>
+
+using namespace unity::util;
+
+namespace {
+
+class LoggerTest : public ::testing::Test
+{
+public:
+    LoggerTest()
+    {
+        _handler_id = g_log_set_handler(G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG,
+                                        _log_handler, this);
+        std::cerr << "Set up handler: " << _handler_id << std::endl;
+    }
+
+    virtual ~LoggerTest()
+    {
+        if (_handler_id != 0) {
+            g_log_remove_handler(G_LOG_DOMAIN, _handler_id);
+        }
+    }
+
+    std::string str()
+    {
+        return _stream.str();
+    }
+
+private:
+    static void _log_handler(const gchar*, GLogLevelFlags,
+                             const gchar* message, gpointer gthis)
+    {
+        static_cast<LoggerTest*>(gthis)->log_message(message);
+    }
+
+    void log_message(const char* message)
+    {
+        std::cerr << "Message: " << message << std::endl;
+        _stream << message;
+        usleep(200);
+    }
+
+    std::ostringstream _stream;
+    guint _handler_id;
+};
+
+TEST_F(LoggerTest, testDebug)
+{
+    auto expected = "Some data about things happening.";
+    debug() << expected;
+    ASSERT_STREQ(expected, str().c_str());
+}
+
+TEST_F(LoggerTest, testInfo)
+{
+    auto expected = "Some info.";
+    info() << expected;
+    ASSERT_STREQ(expected, str().c_str());
+}
+
+TEST_F(LoggerTest, testWarning)
+{
+    auto expected = "This shouldn't normally happen.";
+    warn() << expected;
+    ASSERT_STREQ(expected, str().c_str());
+}
+
+TEST_F(LoggerTest, testCritical)
+{
+    auto expected = "Critical fail.";
+    critical() << expected;
+    ASSERT_STREQ(expected, str().c_str());
+}
+
+TEST_F(LoggerTest, testError)
+{
+    auto expected = "Error crash.";
+    ASSERT_EXIT({error() << expected;},
+                ::testing::KilledBySignal(SIGTRAP), expected);
+    ASSERT_STREQ(expected, str().c_str());
+}
+
+} // namespace

=== modified file 'test/headers/CMakeLists.txt'
--- test/headers/CMakeLists.txt	2017-01-19 13:52:32 +0000
+++ test/headers/CMakeLists.txt	2017-04-03 21:10:57 +0000
@@ -13,6 +13,7 @@
 set(exclusions
     "GlibMemory.h"
     "GObjectMemory.h"
+    "Logger.h"
 )
 
 foreach(dir ${subdirs})

=== modified file 'test/headers/includechecker.py'
--- test/headers/includechecker.py	2017-01-19 13:52:32 +0000
+++ test/headers/includechecker.py	2017-04-03 21:10:57 +0000
@@ -38,6 +38,7 @@
     'unity/shell': { 'Qt' }, # Anything under unity/shell can include anything starting with Qt
     'unity/util/GObjectMemory': { 'glib' }, # The unity/util/GObjectMemory header can include anything starting with glib
     'unity/util/GlibMemory': { 'glib' }, # The unity/util/GlibMemory header can include anything starting with glib
+    'unity/util/Logger': { 'glib' }, # The unity/util/Logger header requires glib
 }
 
 def check_file(filename, permitted_includes):