Mir

Merge lp:~alan-griffiths/mir/command-line-handling into lp:mir

command-line-handling
Merge into development-branch

Proposed by Alan Griffiths on 2014-08-01

Status:

Merged

Approved by:

Alan Griffiths on 2014-08-06

Approved revision:

no longer in the source branch.

Merged at revision:

1826

Proposed branch:

lp:~alan-griffiths/mir/command-line-handling

Merge into:

lp:mir

Diff against target:

290 lines (+167/-6)

9 files modified

include/platform/mir/options/default_configuration.h (+5/-0)
include/platform/mir/options/program_option.h (+3/-0)
server-ABI-sha1sums (+2/-2)
src/platform/options/default_configuration.cpp (+27/-0)
src/platform/options/program_option.cpp (+9/-2)
tests/acceptance-tests/CMakeLists.txt (+1/-0)
tests/acceptance-tests/test_command_line_handling.cpp (+96/-0)
tests/acceptance-tests/test_nested_mir.cpp (+2/-2)
tests/unit-tests/options/test_program_option.cpp (+22/-0)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/command-line-handling

Wishlist

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-08-06
Alan Griffiths			Abstain on 2014-08-05
Robert Carr (community)			Approve on 2014-08-04
Alexandros Frantzis (community)		2014-08-01	Approve on 2014-08-04
Review via email: mp+229255@code.launchpad.net

Commit message

config: make it easier to separate command line options used by Mir from those used elsewhere

Description of the change

config: make it easier to separate command line options used by Mir from those used elsewhere

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-01:

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

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-08-04:

Looks good.

review: Approve

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-08-04:

Looks ok to me. I am not sure that std::function callback is the best API. Certainly

bla_init(int *argc, char ***argv)

is most popular in our ecosystem. So people will use like:

gst_init(argc, argv)
gtk_init(argc, argv)
myapp_init(argc, argv)

I guess server configuration construction is a little different though and I am open to a new pattern.

review: Approve

Revision history for this message

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

> Looks ok to me. I am not sure that std::function callback is the best API.
> Certainly
>
> bla_init(int *argc, char ***argv)
>
> is most popular in our ecosystem. So people will use like:
>
> gst_init(argc, argv)
> gtk_init(argc, argv)
> myapp_init(argc, argv)
>
> I guess server configuration construction is a little different though and I
> am open to a new pattern.

Well, it isn't hard to do blah_init(unparsed_arguments.size(), unparsed_arguments.data()) in the handler. But I guess I could also do that outside the call.

What do people think the best?

function<void(int *argc, char const* const* argv)> or function<void(std::vector<char const*> const&)>

Revision history for this message

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

*Needs Discussion*

> > Looks ok to me. I am not sure that std::function callback is the best API.
> > Certainly
> >
> > bla_init(int *argc, char ***argv)
> >
> > is most popular in our ecosystem. So people will use like:
> >
> > gst_init(argc, argv)
> > gtk_init(argc, argv)
> > myapp_init(argc, argv)
> >
> > I guess server configuration construction is a little different though and I
> > am open to a new pattern.
>
> Well, it isn't hard to do blah_init(unparsed_arguments.size(),
> unparsed_arguments.data()) in the handler. But I guess I could also do that
> outside the call.
>
> What do people think the best?
>
> function<void(int *argc, char const* const* argv)> or
> function<void(std::vector<char const*> const&)>

review: Needs Information

Revision history for this message

Gerry Boland (gerboland) wrote on 2014-08-05:

I'd vote for function<void(int *argc, char const* const* argv)> - just as it's what other toolkits expect.

Revision history for this message

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

> I'd vote for function<void(int *argc, char const* const* argv)> - just as it's
> what other toolkits expect.

Done

review: Abstain

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-06:

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-08-06:

> Start 6: mir_acceptance_tests
> Build timed out (after 240 minutes). Marking the build as failed.
> java.lang.InterruptedException
> Build step 'Debian PBuilder NG' marked build as failure
> Archiving artifacts
> Recording test results
> Build was marked for publishing on https://jenkins.qa.ubuntu.com/
> Finished: FAILURE

Do we have hanging tests? Re-approving as this is unlikely to be related.

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-08-06:

review: Approve (continuous-integration)

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:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/platform/mir/options/default_configuration.h'
 --- include/platform/mir/options/default_configuration.h	2014-04-15 14:47:19 +0000
 +++ include/platform/mir/options/default_configuration.h	2014-08-05 15:50:09 +0000
@@ -30,6 +30,10 @@
+ {
  public:
      DefaultConfiguration(int argc, char const* argv[]);
++    DefaultConfiguration(
++        int argc,
++        char const* argv[],
++        std::function<void(int argc, char const* const* argv)> const& handler);
      virtual ~DefaultConfiguration() = default;
      // add_options() allows users to add their own options. This MUST be called
@@ -57,6 +61,7 @@
      int const argc;
      char const** const argv;
++    std::function<void(int argc, char const* const* argv)> const unparsed_arguments_handler;
      std::shared_ptr<boost::program_options::options_description> const program_options;
      std::shared_ptr<Option> mutable options;
  };
 === modified file 'include/platform/mir/options/program_option.h'
 --- include/platform/mir/options/program_option.h	2014-03-06 06:05:17 +0000
 +++ include/platform/mir/options/program_option.h	2014-08-05 15:50:09 +0000
@@ -54,8 +54,11 @@
      boost::any const& get(char const* name) const override;
      using Option::get;
++    std::vector<std::string> unparsed_command_line() const;
++
  private:
      boost::program_options::variables_map options;
++    std::vector<std::string> unparsed_tokens;
  };
+ }
+ }
 === modified file 'server-ABI-sha1sums'
 --- server-ABI-sha1sums	2014-08-04 15:54:08 +0000
 +++ server-ABI-sha1sums	2014-08-05 15:50:09 +0000
@@ -37,9 +37,9 @@
 f201741a465de33e55ffc1ea775b507a5be950  include/platform/mir/graphics/renderable.h
 b640ec72b04cc2de7671afd7c78942fb9e775a7  include/platform/mir/graphics/tessellation_helpers.h
 b5be510366571c992e0927cc5662af9b0611ae1  include/platform/mir/options/configuration.h
--5879da8056f9a4ff8960f586d27117c87b110b2a  include/platform/mir/options/default_configuration.h
++3a961c5e85b0b9d20eadddd938677776288c06c6  include/platform/mir/options/default_configuration.h
  b45f14082c4f8b29efaa1b13de795dcb29deb738  include/platform/mir/options/option.h
--f42afacda33e598f9c29f2f5b72932f75dd7d27c  include/platform/mir/options/program_option.h
++1133726e66a246bd3988616e205e186278a64731  include/platform/mir/options/program_option.h
 d0213652adc4821a2a180ced74808ca0bbf2c45  include/platform/mir/shared_library_loader.h
 aeb61d0f55893a7ba0998b25c2041f76c4  include/server/mir/asio_main_loop.h
  a4c8f0b0c30784ea6930a8ee6651f1d6efa47231  include/server/mir/compositor/buffer_stream.h
 === modified file 'src/platform/options/default_configuration.cpp'
 --- src/platform/options/default_configuration.cpp	2014-07-09 10:48:47 +0000
 +++ src/platform/options/default_configuration.cpp	2014-08-05 15:50:09 +0000
@@ -63,8 +63,29 @@
+ }
  mo::DefaultConfiguration::DefaultConfiguration(int argc, char const* argv[]) :
++    DefaultConfiguration(
++        argc, argv,
++        [](int argc, char const* const* argv)
++        {
++            if (argc)
++            {
++                std::ostringstream help_text;
++                help_text << "Unknown command line options:";
++                for (auto opt = argv; opt != argv+argc ; ++opt)
++                    help_text << ' ' << *opt;
++                BOOST_THROW_EXCEPTION(mir::AbnormalExit(help_text.str()));
++            }
++        })
++{
++}
++
++mo::DefaultConfiguration::DefaultConfiguration(
++    int argc,
++    char const* argv[],
++    std::function<void(int argc, char const* const* argv)> const& handler) :
      argc(argc),
      argv(argv),
++    unparsed_arguments_handler{handler},
      program_options(std::make_shared<boost::program_options::options_description>(
      "Command-line options.\n"
      "Environment variables capitalise long form with prefix \"MIR_SERVER_\" and \"_\" in place of \"-\""))
@@ -187,6 +208,12 @@
          options.parse_arguments(desc, argc, argv);
++        auto const unparsed_arguments = options.unparsed_command_line();
++        std::vector<char const*> tokens;
++        for (auto const& token : unparsed_arguments)
++            tokens.push_back(token.c_str());
++        unparsed_arguments_handler(tokens.size(), tokens.data());
++
          if (options.is_set("help"))
+         {
              std::ostringstream help_text;
 === modified file 'src/platform/options/program_option.cpp'
 --- src/platform/options/program_option.cpp	2014-03-06 06:05:17 +0000
 +++ src/platform/options/program_option.cpp	2014-08-05 15:50:09 +0000
@@ -43,9 +43,16 @@
      int argc,
      char const* argv[])
+ {
--    // TODO: Don't allow unregistered options, once we allow better overriding of option parsing
--    po::store(po::command_line_parser(argc, argv).options(desc).allow_unregistered().run(), options);
++    auto parsed_command_line = po::command_line_parser(argc, argv).options(desc).allow_unregistered().run();
++    po::store(parsed_command_line, options);
      po::notify(options);
++
++    unparsed_tokens = collect_unrecognized(parsed_command_line.options, po::include_positional);
++}
++
++std::vector<std::string> mo::ProgramOption::unparsed_command_line() const
++{
++    return unparsed_tokens;
+ }
  void mo::ProgramOption::parse_environment(
 === modified file 'tests/acceptance-tests/CMakeLists.txt'
 --- tests/acceptance-tests/CMakeLists.txt	2014-07-30 15:15:16 +0000
 +++ tests/acceptance-tests/CMakeLists.txt	2014-08-05 15:50:09 +0000
@@ -18,6 +18,7 @@
    test_client_library.cpp
    test_client_library_old.cpp
    test_client_surface_events.cpp
++  test_command_line_handling.cpp
    test_custom_input_dispatcher.cpp
    test_client_surfaces.cpp
    test_test_framework.cpp
 === added file 'tests/acceptance-tests/test_command_line_handling.cpp'
 --- tests/acceptance-tests/test_command_line_handling.cpp	1970-01-01 00:00:00 +0000
 +++ tests/acceptance-tests/test_command_line_handling.cpp	2014-08-05 15:50:09 +0000
@@ -0,0 +1,96 @@
++/*
++ * 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: Alan Griffiths <alan@octopull.co.uk>
++ */
++
++#include "mir/run_mir.h"
++#include "mir/default_server_configuration.h"
++#include "mir/options/default_configuration.h"
++
++#include <gtest/gtest.h>
++#include <gmock/gmock.h>
++
++using namespace testing;
++
++namespace mo = mir::options;
++
++struct CommandLineHandling : Test
++{
++    MOCK_METHOD1(callback, void(std::vector<char const*> const&));
++
++    std::function<void(int argc, char const* const* argv)> callback_functor =
++        [this](int argc, char const* const* argv)
++        {
++            // Copy to a vector as ElementsAre() is convenient for checking
++            std::vector<char const*> const copy{argv, argv+argc};
++            callback(copy);
++        };
++
++    void invoke_parsing(mo::Configuration& config) { config.the_options(); }
++
++};
++
++TEST_F(CommandLineHandling, valid_options_are_accepted_by_default_callback)
++{
++    char const* argv[] =
++     { "dummy-exe-name", "--file", "test", "--enable-input", "off"};
++
++    int const argc = std::distance(std::begin(argv), std::end(argv));
++
++    mo::DefaultConfiguration config{argc, argv};
++
++    invoke_parsing(config);
++}
++
++TEST_F(CommandLineHandling, unrecognised_tokens_cause_default_callback_to_throw)
++{
++    char const* argv[] =
++     { "dummy-exe-name", "--file", "test", "--hello", "world", "--enable-input", "off"};
++
++    int const argc = std::distance(std::begin(argv), std::end(argv));
++
++    mo::DefaultConfiguration config{argc, argv};
++
++    EXPECT_THROW(invoke_parsing(config), std::runtime_error);
++}
++
++TEST_F(CommandLineHandling, valid_options_are_not_passed_to_callback)
++{
++    char const* argv[] =
++     { "dummy-exe-name", "--file", "test", "--enable-input", "off"};
++
++    int const argc = std::distance(std::begin(argv), std::end(argv));
++
++    EXPECT_CALL(*this, callback(ElementsAre()));
++
++    mo::DefaultConfiguration config{argc, argv, callback_functor};
++
++    invoke_parsing(config);
++}
++
++TEST_F(CommandLineHandling, unrecognised_tokens_are_passed_to_callback)
++{
++    char const* argv[] =
++     { "dummy-exe-name", "--file", "test", "--hello", "world", "--enable-input", "off"};
++
++    int const argc = std::distance(std::begin(argv), std::end(argv));
++
++    EXPECT_CALL(*this, callback(ElementsAre(StrEq("--hello"), StrEq("world"))));
++
++    mo::DefaultConfiguration config{argc, argv, callback_functor};
++
++    invoke_parsing(config);
++}
 === modified file 'tests/acceptance-tests/test_nested_mir.cpp'
 --- tests/acceptance-tests/test_nested_mir.cpp	2014-07-21 03:35:31 +0000
 +++ tests/acceptance-tests/test_nested_mir.cpp	2014-08-05 15:50:09 +0000
@@ -92,13 +92,13 @@
  struct FakeCommandLine
+ {
--    static int const argc = 6;
++    static int const argc = 7;
      char const* argv[argc];
      FakeCommandLine(std::string const& host_socket)
+     {
          char const** to = argv;
--        for(auto from : { "--file", "NestedServer", "--host-socket", host_socket.c_str(), "--enable-input", "off"})
++        for(auto from : { "dummy-exe-name", "--file", "NestedServer", "--host-socket", host_socket.c_str(), "--enable-input", "off"})
+         {
              *to++ = from;
+         }
 === modified file 'tests/unit-tests/options/test_program_option.cpp'
 --- tests/unit-tests/options/test_program_option.cpp	2014-03-06 06:05:17 +0000
 +++ tests/unit-tests/options/test_program_option.cpp	2014-08-05 15:50:09 +0000
@@ -192,6 +192,28 @@
      EXPECT_THROW(po.get<int>("flag-yes,y"), std::bad_cast);
+ }
++TEST_F(ProgramOption, unparsed_command_line_returns_unprocessed_tokens)
++{
++    using namespace testing;
++
++    mir::options::ProgramOption po;
++
++    const int argc = 11;
++    char const* argv[argc] = {
++        __PRETTY_FUNCTION__,
++        "--flag-yes", "yes",
++        "--hello",
++        "-f", "test_file",
++        "world",
++        "-c", "27",
++        "--answer", "42"
++    };
++
++    po.parse_arguments(desc, argc, argv);
++
++    EXPECT_THAT(po.unparsed_command_line(), ElementsAre("--hello", "world", "--answer", "42"));
++}
++
  TEST(ProgramOptionEnv, parse_environment)
+ {
      // Env variables should be uppercase and "_" delimited