Mir

Merge lp:~alan-griffiths/mir/helptext-mentions-config-file into lp:mir

helptext-mentions-config-file
Merge into development-branch

Proposed by Alan Griffiths on 2015-05-28

Status:	Merged
Approved by:	Alan Griffiths on 2015-06-02
Approved revision:	no longer in the source branch.
Merged at revision:	2612
Proposed branch:	lp:~alan-griffiths/mir/helptext-mentions-config-file
Merge into:	lp:mir
Diff against target:	180 lines (+63/-33) 4 files modified src/include/platform/mir/options/default_configuration.h (+10/-4) src/platform/options/default_configuration.cpp (+48/-7) src/server/server.cpp (+3/-21) tests/mir_test_framework/command_line_server_configuration.cpp (+2/-1)
To merge this branch:	bzr merge lp:~alan-griffiths/mir/helptext-mentions-config-file
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-06-02
Alexandros Frantzis (community)			Approve on 2015-06-02
Kevin DuBois (community)		2015-05-28	Approve on 2015-06-01
Review via email: mp+260439@code.launchpad.net

Commit message

options: incorporate the program name and any (optional) config file into the description text

Description of the change

options: incorporate the program name and any (optional) config file into the description text

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3931/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-28:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3938/rebuild

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-06-01:

88+ "The config file (" + config_file + ") is located via the XDG Base Directory Specification.\n\n";
Recommending $XDG_RUNTIME_DIR as the first place to look would be more helpful, but the changes lgtm overall.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3959/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-06-01:

subprocess.CalledProcessError: Command 'phablet-network --skip-setup' returned non-zero exit status 124
SETTING UP SUDO
[sudo] password for phablet: Sorry, try again.
[sudo] password for phablet:
sudo: 1 incorrect password attempt

CI infrastructure

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-01:

PASSED: Continuous integration, rev:2610
http://jenkins.qa.ubuntu.com/job/mir-ci/3962/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2650
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2598
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/118
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/118/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2598
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2598/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5448
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20875

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3962/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-06-02:

Looks good from a functionality perspective.

A couple of formatting issues:

13+ DefaultConfiguration(
14+ int argc, char const* argv[], std::function <void(
15+ int argc, char const* const* argv)> const& handler);

Clearer if std::function<> is on its own line (i.e. like it was before), instead of breaking after the parenthesis.

16+ DefaultConfiguration(
17+ int argc, char const* argv[], std::function <void(
18+ int argc, char const* const* argv)> const& handler,
19+ std::string const& config_file);

Clearer if std::function<> is on its own line, instead of breaking after the parenthesis.
Also, the indentation is too deep.

51+ DefaultConfiguration(argc, argv, [](int argc, char const* const* argv)
52 {

Clearer (IMO) if [](int argc, ...) is on its own line before the opening brace, to keep the lambda together (i.e. at the same indentation level).

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-06-02:

Looks good.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-02:

PASSED: Continuous integration, rev:2611
http://jenkins.qa.ubuntu.com/job/mir-ci/3970/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2659
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/170
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2607
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/126
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/126/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2607
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2607/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5455
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20887

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3970/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-02:

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-06-02:

[0;32m[ RUN ] [mGLMark2Test.benchmark_fullscreen_default
[1433254279.005835] mirplatform: Found graphics driver: android
[1433254279.006018] mirplatform: Found graphics driver: dummy
[1433254279.006659] mirplatform: Found graphics driver: mesa
glmark2-es2-mir: relocation error: /usr/lib/arm-linux-gnueabihf/libmirclient.so.8: symbol _ZN3mir8dispatch20SimpleDispatchThreadC1ERKSt10shared_ptrINS0_12DispatchableEE, version MIR_COMMON_4 not defined in file libmircommon.so.4 with link time reference
/tmp/buildd/mir-0.14.0bzr2612pkg0wily2609+autopilot0/tests/performance-tests/test_glmark2-es2-mir.cpp:63: Failure
Value of: score
Expected: is >= 52
Actual: -1 (of type int)
[0;31m[ FAILED ] [mGLMark2Test.benchmark_fullscreen_default (858 ms)
[0;32m[----------] [m1 test from GLMark2Test (860 ms total)

Oh! We've still got ABI issues. :(

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2015-06-02:

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 'src/include/platform/mir/options/default_configuration.h'
 --- src/include/platform/mir/options/default_configuration.h	2015-03-31 02:35:42 +0000
 +++ src/include/platform/mir/options/default_configuration.h	2015-06-02 09:07:30 +0000
@@ -32,10 +32,14 @@
+ {
  public:
      DefaultConfiguration(int argc, char const* argv[]);
--    DefaultConfiguration(
--        int argc,
--        char const* argv[],
--        std::function<void(int argc, char const* const* argv)> const& handler);
++    DefaultConfiguration(int argc, char const* argv[], std::string const& config_file);
++    DefaultConfiguration(
++        int argc, char const* argv[],
++        std::function <void(int argc, char const* const* argv)> const& handler);
++    DefaultConfiguration(
++        int argc, char const* argv[],
++        std::function <void(int argc, char const* const* argv)> const& handler,
++        std::string const& config_file);
      virtual ~DefaultConfiguration() = default;
      // add_options() allows users to add their own options. This MUST be called
@@ -47,6 +51,8 @@
      // call destructors in DSOs we've unloaded.
      std::shared_ptr<SharedLibrary> platform_graphics_library;
++    std::string const config_file;
++
      void add_platform_options();
      // accessed via the base interface, when access to add_options() has been "lost"
      std::shared_ptr<options::Option> the_options() const override;
 === modified file 'src/platform/options/default_configuration.cpp'
 --- src/platform/options/default_configuration.cpp	2015-05-19 21:34:34 +0000
 +++ src/platform/options/default_configuration.cpp	2015-06-02 09:07:30 +0000
@@ -65,8 +65,15 @@
+ }
  mo::DefaultConfiguration::DefaultConfiguration(int argc, char const* argv[]) :
--    DefaultConfiguration(
--        argc, argv,
++    DefaultConfiguration(argc, argv, std::string{})
++{
++}
++
++mo::DefaultConfiguration::DefaultConfiguration(
++    int argc,
++    char const* argv[],
++    std::string const& config_file) :
++    DefaultConfiguration(argc, argv,
          [](int argc, char const* const* argv)
+         {
              if (argc)
@@ -77,7 +84,8 @@
                      help_text << ' ' << *opt;
                  BOOST_THROW_EXCEPTION(mir::AbnormalExit(help_text.str()));
+             }
--        })
++        },
++        config_file)
+ {
+ }
@@ -85,12 +93,43 @@
      int argc,
      char const* argv[],
      std::function<void(int argc, char const* const* argv)> const& handler) :
++    DefaultConfiguration(argc, argv, handler, std::string{})
++{
++}
++
++namespace
++{
++std::string description_text(char const* program, std::string const& config_file)
++{
++    std::string result{
++        "Command-line options (e.g. \"--host-socket=/tmp/mir_socket\").\n\n"
++        "Environment variables capitalise long form with prefix \"MIR_SERVER_\" and \"_\" in place of \"-\".\n"
++        "(E.g. \"MIR_SERVER_HOST_SOCKET=/tmp/mir_socket\")\n\n"};
++
++    if (program)
++        result = std::string{"usage: "} + program + " [options]\n\n" + result;
++
++    if (!config_file.empty())
++        result +=
++        "Config file entries are long form (e.g. \"host-socket=/tmp/mir_socket\").\n"
++        "The config file (" + config_file + ") is located via the XDG Base Directory Specification.\n"
++        "($XDG_CONFIG_HOME or $HOME/.config followed by $XDG_CONFIG_DIRS)\n\n";
++
++    return result + "user options";
++}
++}
++
++mo::DefaultConfiguration::DefaultConfiguration(
++    int argc,
++    char const* argv[],
++    std::function<void(int argc, char const* const* argv)> const& handler,
++    std::string const& config_file) :
++    config_file{config_file},
      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 \"-\""))
++        description_text(argv[0], config_file)))
+ {
      using namespace options;
      namespace po = boost::program_options;
@@ -268,7 +307,9 @@
+ }
  void mo::DefaultConfiguration::parse_config_file(
--    boost::program_options::options_description& /*desc*/,
--    mo::ProgramOption& /*options*/) const
++    boost::program_options::options_description& desc,
++    mo::ProgramOption& options) const
+ {
++    if (!config_file.empty())
++        options.parse_file(desc, config_file);
+ }
 === modified file 'src/server/server.cpp'
 --- src/server/server.cpp	2015-05-19 21:34:34 +0000
 +++ src/server/server.cpp	2015-06-02 09:07:30 +0000
@@ -224,36 +224,18 @@
  namespace
+ {
--class ConfigurationOptions : public mo::DefaultConfiguration
--{
--public:
--    using mo::DefaultConfiguration::DefaultConfiguration;
--
--    std::string config_file;
--
--    void parse_config_file(
--        boost::program_options::options_description& options_description,
--        mo::ProgramOption& options) const override
--    {
--        if (!config_file.empty())
--            options.parse_file(options_description, config_file);
--    }
--};
--
  std::shared_ptr<mo::DefaultConfiguration> configuration_options(
      int argc,
      char const** argv,
      std::function<void(int argc, char const* const* argv)> const& command_line_hander,
      std::string const& config_file)
+ {
--    std::shared_ptr<ConfigurationOptions> result;
++    std::shared_ptr<mo::DefaultConfiguration> result;
      if (command_line_hander)
--        result = std::make_shared<ConfigurationOptions>(argc, argv, command_line_hander);
++        result = std::make_shared<mo::DefaultConfiguration>(argc, argv, command_line_hander, config_file);
      else
--        result = std::make_shared<ConfigurationOptions>(argc, argv);
--
--    result->config_file = config_file;
++        result = std::make_shared<mo::DefaultConfiguration>(argc, argv, config_file);
      return result;
+ }
 === modified file 'tests/mir_test_framework/command_line_server_configuration.cpp'
 --- tests/mir_test_framework/command_line_server_configuration.cpp	2015-05-20 14:00:02 +0000
 +++ tests/mir_test_framework/command_line_server_configuration.cpp	2015-06-02 09:07:30 +0000
@@ -28,8 +28,9 @@
  namespace
+ {
++char const* args[] = {nullptr};
  int argc;
--char const** argv;
++char const** argv = args;
+ }
  auto mir_test_framework::configuration_from_commandline()