Mir

Merge lp:~josharenson/mir/command_line_config into lp:mir

command_line_config
Merge into development-branch

Proposed by Josh Arenson on 2014-05-14

Status:

Merged

Approved by:

Alan Griffiths on 2014-05-20

Approved revision:

no longer in the source branch.

Merged at revision:

1643

Proposed branch:

lp:~josharenson/mir/command_line_config

Merge into:

lp:mir

Diff against target:

164 lines (+97/-18)

4 files modified

include/test/mir_test_framework/command_line_server_configuration.h (+38/-0)
tests/mir_test_framework/CMakeLists.txt (+1/-0)
tests/mir_test_framework/command_line_server_configuration.cpp (+56/-0)
tests/mir_test_framework/stubbed_server_configuration.cpp (+2/-18)

To merge this branch:

bzr merge lp:~josharenson/mir/command_line_config

Wishlist

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre (community)		2014-05-14	Approve on 2014-05-19
PS Jenkins bot (community)	continuous-integration		Approve on 2014-05-19
Alan Griffiths			Approve on 2014-05-19
Review via email: mp+219619@code.launchpad.net

Commit message

Implements a command_line_server_configuration that allows for creating a DefaultServerConfiguration, but respects command line arguments

Fixes lp:1316987

Description of the change

Implements a command_line_server_configuration that allows for creating a DefaultServerConfiguration, but respects command line arguments

Fixes lp:1316987

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-05-15:

~~~
6 + * Copyright © 2012 Canonical Ltd.
~~~

s/2012/2014

~~~
26 +#include "mir/options/default_configuration.h"
~~~

Foward declare instead:

namespace mir
{
namespace options
{
class DefaultConfiguration;
}
}

~~~
28 +namespace mo=mir::options;
~~~

I believe we avoid using namespace aliases in headers.

~~~
33 + -> std::shared_ptr<mo::DefaultConfiguration>;
~~~

Missing #include <memory> for shared_ptr

~~~
36 +int main(int argc, char** argv);
~~~

Not needed.

~~~
57 + * Copyright © 2012 Canonical Ltd.
~~~

s/2012/2014

~~~
74 +#include "mir/default_configuration.h"
76 +#include "mir/default_server_configuration.h"
83 +#include <memory>
~~~

^-- Not needed

~~~
87 +int argc;
88 +char const** argv;
~~~

Mark them static?

~~~
107 + [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
~~~

Because of the use of strncmp there should be a corresponding #include <cstring>

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1627/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-15:

23 +#ifndef COMMAND_LINE_SERVER_CONFIGURATION
24 +#define COMMAND_LINE_SERVER_CONFIGURATION

Missing MIR_TEST_FRAMEWORK_ prefix

~~~~

26 +#include "mir/options/default_configuration.h"

We only need a declaration: namespace mir { namespace options { class mo::Configuration; } }

But we do need

#include <memory>

~~~~

28 +namespace mo=mir::options;

Not in a header.

~~~~

30 +namespace mir_test_framework
31 +{
32 + auto configuration_from_commandline()
33 + -> std::shared_ptr<mo::DefaultConfiguration>;

The return type should be mir::options::Configuration;

~~~~

36 +int main(int argc, char** argv);

not wanted.

~~~~

74 +#include "mir/default_configuration.h"
75 +#include "mir/options/default_configuration.h"
76 +#include "mir/default_server_configuration.h"
77 +#include "mir_test_framework/command_line_server_configuration.h"

The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)

~~~~

74 +#include "mir/default_configuration.h"
...
76 +#include "mir/default_server_configuration.h"
...
79 +#include <boost/algorithm/string.hpp>

Unused headers

~~~~

125 +#include "mir_test_framework/command_line_server_configuration.h"
126 #include "mir_test_framework/stubbed_server_configuration.h"

The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)

~~~~

stubbed_server_configuration.cpp no longer needs:

#include "mir/options/default_configuration.h"

It does need "mir/options/configuration.h"

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-15:

> ~~~
> 87 +int argc;
> 88 +char const** argv;
> ~~~
>
> Mark them static?

No put them in an anonymous namespace (as they were in the "before" code).

review: Needs Fixing

Revision history for this message

Josh Arenson (josharenson) wrote on 2014-05-15:

>
> ~~~~
>
> 30 +namespace mir_test_framework
> 31 +{
> 32 + auto configuration_from_commandline()
> 33 + -> std::shared_ptr<mo::DefaultConfiguration>;
>
> The return type should be mir::options::Configuration;

If Configuration is returned, can options be added later (using boost::options)?

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-05-15:

Just some more minor nits:

~~~
23 +#ifndef COMMAND_LINE_SERVER_CONFIGURATION
~~~

Like Alan mentioned, the convention is:

MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION_H_

~~~
78 +#include "mir/default_configuration.h"
~~~
Not needed.

~~~
100 + return make_shared<mo::DefaultConfiguration>(::argc, ::argv);
~~~
Per the style guide: http://unity.ubuntu.com/mir/cppguide/index.html?showone=Spaces_vs._Tabs#Spaces_vs._Tabs

It should be indented 4 spaces from the curly bracket.

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1640/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1643/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-16:

> > The return type should be mir::options::Configuration;
>
>
> If Configuration is returned, can options be added later (using
> boost::options)?

Ack

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-16:

78 +#include "mir/default_configuration.h"

Not used

~~~~

126 +#include "mir_test_framework/command_line_server_configuration.h"
127 #include "mir_test_framework/stubbed_server_configuration.h"

The first header should be the one that corresponds to the .cpp (which proves the header is self-contained)

~~~~

96 + using namespace std;

Just use std::make_shared a few lines later.

~~~~

97 + auto configuration_from_commandline()
98 + -> shared_ptr<mo::DefaultConfiguration>
99 + {
100 + return make_shared<mo::DefaultConfiguration>(::argc, ::argv);
101 + }
...
107 + std::cout << "Running main() from " << basename(__FILE__) << std::endl;
108 + ::argc = std::remove_if(
109 + argv,
110 + argv+argc,
111 + [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
112 + ::argv = const_cast<char const**>(argv);
113 +
114 + ::testing::InitGoogleTest(&argc, argv);
115 +
116 + return RUN_ALL_TESTS();

We don't use tabs on this codebase.

~~~~

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1652/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-05-19:

89 +int argc;
90 +char const** argv;

I'm curious that these are now uninitialized when they were previously initialized with "safe" defaults. Was this intentional? And why?

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-05-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1662/rebuild

review: Approve (continuous-integration)

Revision history for this message

Josh Arenson (josharenson) wrote on 2014-05-19:

> 89 +int argc;
> 90 +char const** argv;
>
> I'm curious that these are now uninitialized when they were previously
> initialized with "safe" defaults. Was this intentional? And why?

Since this _should always_ be called from a command line application, and since a program cannot have more than one main function, argv will always be populated with _at least_ the executable name. I thought it might be confusing to initialize variables that are typically not initialized, however, there isn't any harm in initializing them as such:

132 -char const* dummy[] = {0};
133 -int argc = 0;
134 -char const** argv = dummy;

for safety's sake. Thoughts?

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-05-19:

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Emanuele Antonio Faraone

Gerry Boland

Josh Arenson

Mir development team

 === added file 'include/test/mir_test_framework/command_line_server_configuration.h'
 --- include/test/mir_test_framework/command_line_server_configuration.h	1970-01-01 00:00:00 +0000
 +++ include/test/mir_test_framework/command_line_server_configuration.h	2014-05-16 15:47:38 +0000
@@ -0,0 +1,38 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * 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 General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Josh Arenson <joshua.arenson@canonical.com>
++ */
++
++#ifndef MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION
++#define MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION
++
++#include <memory>
++
++namespace mir
++{
++namespace options
++{
++class DefaultConfiguration;
++}
++}
++
++namespace mir_test_framework
++{
++    auto configuration_from_commandline()
++    -> std::shared_ptr<mir::options::DefaultConfiguration>;
++}
++
++#endif /* MIR_TEST_FRAMEWORK_COMMAND_LINE_SERVER_CONFIGURATION */
 === modified file 'tests/mir_test_framework/CMakeLists.txt'
 --- tests/mir_test_framework/CMakeLists.txt	2014-05-14 06:34:25 +0000
 +++ tests/mir_test_framework/CMakeLists.txt	2014-05-16 15:47:38 +0000
@@ -8,6 +8,7 @@
  set(
    TEST_FRAMEWORK_SRCS
++  command_line_server_configuration.cpp
    cross_process_sync.cpp
    in_process_server.cpp
    testing_server_options.cpp
 === added file 'tests/mir_test_framework/command_line_server_configuration.cpp'
 --- tests/mir_test_framework/command_line_server_configuration.cpp	1970-01-01 00:00:00 +0000
 +++ tests/mir_test_framework/command_line_server_configuration.cpp	2014-05-16 15:47:38 +0000
@@ -0,0 +1,56 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU General Public License version 3,
++ * 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 General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ */
++
++#include "mir_test_framework/command_line_server_configuration.h"
++#include "mir/options/default_configuration.h"
++
++#include <gtest/gtest.h>
++
++#include <algorithm>
++#include <cstring>
++
++namespace mo=mir::options;
++
++namespace
++{
++int argc;
++char const** argv;
++}
++
++namespace mir_test_framework
++{
++    auto configuration_from_commandline()
++    -> std::shared_ptr<mo::DefaultConfiguration>
++    {
++        return std::make_shared<mo::DefaultConfiguration>(::argc, ::argv);
++    }
++}
++
++int main(int argc, char** argv)
++{
++    // Override this standard gtest message
++    std::cout << "Running main() from " << basename(__FILE__) << std::endl;
++    ::argc = std::remove_if(
++        argv,
++        argv+argc,
++        [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
++    ::argv = const_cast<char const**>(argv);
++
++    ::testing::InitGoogleTest(&argc, argv);
++
++    return RUN_ALL_TESTS();
++}
 === modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp'
 --- tests/mir_test_framework/stubbed_server_configuration.cpp	2014-05-12 04:29:42 +0000
 +++ tests/mir_test_framework/stubbed_server_configuration.cpp	2014-05-16 15:47:38 +0000
@@ -17,6 +17,7 @@
   */
  #include "mir_test_framework/stubbed_server_configuration.h"
++#include "mir_test_framework/command_line_server_configuration.h"
  #include "mir/options/default_configuration.h"
  #include "mir/geometry/rectangle.h"
@@ -58,10 +59,6 @@
  namespace
+ {
--char const* dummy[] = {0};
--int argc = 0;
--char const** argv = dummy;
--
  class StubFDBuffer : public mtd::StubBuffer
+ {
  public:
@@ -222,7 +219,7 @@
  mtf::StubbedServerConfiguration::StubbedServerConfiguration() :
      DefaultServerConfiguration([]
+     {
--        auto result = std::make_shared<mo::DefaultConfiguration>(::argc, ::argv);
++        auto result = mtf::configuration_from_commandline();
          namespace po = boost::program_options;
@@ -278,16 +275,3 @@
      else
          return std::make_shared<mi::NullInputDispatcherConfiguration>();
+ }
--
--int main(int argc, char** argv)
--{
--    ::argc = std::remove_if(
--        argv,
--        argv+argc,
--        [](char const* arg) { return !strncmp(arg, "--gtest_", 8); }) - argv;
--    ::argv = const_cast<char const**>(argv);
--
--  ::testing::InitGoogleTest(&argc, argv);
--
--  return RUN_ALL_TESTS();
--}