Merge lp:~mandel/ubuntu-download-manager/use-log-dir into lp:ubuntu-download-manager

[Manuel de la Peña]

Fix login and allow to pass the path via the command line.

Fix login and allow to pass the path via the command line.

[lp:~mandel/ubuntu-download-manager/use-log-dir]

238. By Manuel de la Peña on 2014-02-12

Fix small logging issue.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:237
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/345/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/234/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/234/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/345/rebuild

review: Needs Fixing (continuous-integration)

[lp:~mandel/ubuntu-download-manager/use-log-dir]

239. By Manuel de la Peña on 2014-02-12

Bump num.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:238
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/346/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/235
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/235

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/346/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:239
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/347/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/236

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/347/rebuild

review: Approve (continuous-integration)

[Mike McCracken (mikemc)]

this still names the file UNKNOWN.INFO

I looked into the glog source to see why - InitGoogleLogging() expects to be sent argv[0], which will always be around, so it just stores a pointer to it - it doesn't copy the string or take ownership or anything.

So I'm guessing that the QString you get from applicationName goes out of scope or something, before the filename is generated, and when it checks for the app name, it sees null and just goes with UNKNOWN. (see
https://code.google.com/p/google-glog/source/browse/trunk/src/utilities.cc#162 )

so maybe you need to hand it a heap allocated char array?

PS, I tried messing with it a bit to try to use -log-dir, and I couldn't get that to work, but I wasn't sure I was runnign it correctly. I just ran build/ubuntu-download-manager/ubuntu-download-manager -log-dir=/tmp/foo but it still used the default.

review: Needs Fixing

[lp:~mandel/ubuntu-download-manager/use-log-dir]

240. By Manuel de la Peña on 2014-02-13

Create the passed dir.

[Manuel de la Peña (mandel)]

On Thu, Feb 13, 2014 at 1:45 AM, Mike McCracken <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> this still names the file UNKNOWN.INFO
>
> I looked into the glog source to see why - InitGoogleLogging() expects to
> be sent argv[0], which will always be around, so it just stores a pointer
> to it - it doesn't copy the string or take ownership or anything.
>
> So I'm guessing that the QString you get from applicationName goes out of
> scope or something, before the filename is generated, and when it checks
> for the app name, it sees null and just goes with UNKNOWN. (see
>
> https://code.google.com/p/google-glog/source/browse/trunk/src/utilities.cc#162)
>

I have tested the current code in the branch and it works in my system:

mandel@ironman:~/Desktop$ ubuntu-download-manager -log-dir
~/Desktop/udm-testing
^C
mandel@ironman:~/Desktop$ ls ~/Desktop/udm-testing/
ubuntu-download-manager.INFO
ubuntu-download-manager.log20140213-120124.20614
mandel@ironman:~/Desktop$

Can you please double check, I'm sure it is ok.

> so maybe you need to hand it a heap allocated char array?
>
> PS, I tried messing with it a bit to try to use -log-dir, and I couldn't
> get that to work, but I wasn't sure I was runnign it correctly. I just ran
> build/ubuntu-download-manager/ubuntu-download-manager -log-dir=/tmp/foo but
> it still used the default.
>

Sorry, that was a bug, the code was expecting to find the path and would
fall back in the default one. I have changed the code to create the path
when is possible, if the dir cannot be created (user rights etc..) it will
use the default one.

> --
>
> https://code.launchpad.net/~mandel/ubuntu-download-manager/use-log-dir/+merge/206051
> You are the owner of lp:~mandel/ubuntu-download-manager/use-log-dir.
>

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:240
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/348/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-amd64-ci/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-trusty-armhf-ci/237

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-download-manager-ci/348/rebuild

review: Approve (continuous-integration)

[Mike McCracken (mikemc)]

abstain to unblock, I couldn't get it to work and don't have time to try again right now

review: Abstain

=== modified file 'ubuntu-download-manager-priv/downloads/daemon.cpp'
--- ubuntu-download-manager-priv/downloads/daemon.cpp	2014-02-03 15:30:58 +0000
+++ ubuntu-download-manager-priv/downloads/daemon.cpp	2014-02-13 11:02:05 +0000
@@ -31,6 +31,7 @@
     const QString DISABLE_TIMEOUT = "-disable-timeout";
     const QString SELFSIGNED_CERT = "-self-signed-certs";
     const QString STOPPABLE =  "-stoppable";
+    const QString LOG_DIR= "-log-dir";
 }
 
 namespace Ubuntu {
@@ -161,8 +162,23 @@
  private:
     void parseCommandLine() {
         QStringList args = _app->arguments();
+        int index;
+
+        // set logging
+        if (args.contains(LOG_DIR)) {
+            index = args.indexOf(LOG_DIR);
+            if (args.count() > index + 1) {
+                auto logPath = args[index + 1];
+                Logger::setupLogging(logPath);
+		LOG(INFO) << "Log path is" << logPath;
+            } else {
+                LOG(ERROR) << "Missing log dir path.";
+                Logger::setupLogging();
+            }
+        }
+
         if (args.contains(SELFSIGNED_CERT)) {
-            int index = args.indexOf(SELFSIGNED_CERT);
+            index = args.indexOf(SELFSIGNED_CERT);
             if (args.count() > index + 1) {
                 QString certsRegex = args[index + 1];
                 _certs = QSslCertificate::fromPath(certsRegex);
@@ -200,9 +216,6 @@
         q->connect(_downInterface,
             SIGNAL(sizeChanged(int)),  // NOLINT (readability/function)
             q, SLOT(onDownloadManagerSizeChanged(int))); // NOLINT (readability/function)
-
-        // set logging
-        Logger::setupLogging();
     }
 
  private:

=== modified file 'ubuntu-download-manager-priv/system/logger.cpp'
--- ubuntu-download-manager-priv/system/logger.cpp	2014-01-24 11:48:06 +0000
+++ ubuntu-download-manager-priv/system/logger.cpp	2014-02-13 11:02:05 +0000
@@ -25,7 +25,6 @@
 #include <QStringList>
 #include <QUrl>
 #include <QSslError>
-#include <syslog.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include "logger.h"
@@ -64,6 +63,10 @@
     return out;
 }
 
+namespace {
+    const QString LOG_NAME = "ubuntu-download-manager.log";
+}
+
 namespace Ubuntu {
 
 namespace DownloadManager {
@@ -73,18 +76,26 @@
 static bool _init = false;
 
 void
-Logger::setupLogging(const QString filename) {
-    auto path = filename;
-    if (filename == "" ){
-        path = getLogDir() + "/ubuntu-download-manager.log";
+Logger::setupLogging(const QString logDir) {
+    auto path = logDir;
+    if (path == "" ){
+        path = getLogDir() + QDir::separator() + LOG_NAME;
+    } else {
+        bool wasCreated = QDir().mkpath(logDir);
+        if (wasCreated) {
+            path = QDir(logDir).absoluteFilePath(LOG_NAME);
+        } else {
+            qCritical() << "Could not create the logging path" << logDir;
+            path = getLogDir() + QDir::separator() + LOG_NAME;
+        }
     }
 
     auto appName = QCoreApplication::instance()->applicationName();
 
-    google::SetLogDestination(google::INFO, toStdString(path).c_str());
     if (!_init) {
         _init = true;
         google::InitGoogleLogging(toStdString(appName).c_str());
+        google::SetLogDestination(google::INFO, toStdString(path).c_str());
     }
 }
 

=== modified file 'ubuntu-download-manager-priv/system/logger.h'
--- ubuntu-download-manager-priv/system/logger.h	2014-01-28 16:03:24 +0000
+++ ubuntu-download-manager-priv/system/logger.h	2014-02-13 11:02:05 +0000
@@ -53,7 +53,7 @@
 
  public:
 
-    static void setupLogging(const QString filename = "");
+    static void setupLogging(const QString logDir= "");
     static bool setLogLevel(QtMsgType level);
     static QString getLogDir();
     static std::string toStdString(const QString& str);