Handle wrapper binary options also for symlinks.
authorTeemu Murtola <teemu.murtola@gmail.com>
Tue, 23 Jul 2013 04:08:19 +0000 (07:08 +0300)
committerGerrit Code Review <gerrit@gerrit.gromacs.org>
Tue, 30 Jul 2013 04:10:42 +0000 (06:10 +0200)
Now the -quiet, -version, and -copyright options also work when the
binary is invoked through a symlink (so, e.g., g_angle -quiet suppresses
the startup headers).  They also work for the user tools, implemented
using the single-module wrapper.

Extended the command-line parser to support parsing only recognized
options and removing those from the command line while leaving the rest
untouched.

Related to #685 and #1209.

Change-Id: I740f70386d89694246c3e25ba0a1c1c4df17dc6b

src/gromacs/commandline/cmdlinemodulemanager.cpp
src/gromacs/commandline/cmdlineparser.cpp
src/gromacs/commandline/cmdlineparser.h
src/gromacs/commandline/tests/cmdlineparser.cpp
src/gromacs/options/optionsassigner.cpp
src/gromacs/options/optionsassigner.h
src/testutils/cmdlinetest.cpp
src/testutils/cmdlinetest.h

index be6dcc239845d5dd16a187252a941b8b89c44cd8..97d9fad59b16bd90786240cedea213ed46746382 100644 (file)
@@ -437,70 +437,98 @@ CommandLineModuleManager::Impl::findModuleFromBinaryName(
 CommandLineModuleInterface *
 CommandLineModuleManager::Impl::processCommonOptions(int *argc, char ***argv)
 {
-    if (singleModule_ != NULL)
-    {
-        // TODO: Process common options also in this case.
-        return singleModule_;
-    }
-    // Check if the module is called through a symlink.
-    CommandLineModuleMap::const_iterator module
-        = findModuleFromBinaryName(programInfo_);
-    if (module != modules_.end())
-    {
-        // TODO: Process common options also in this case.
-        return module->second.get();
-    }
-    // If not, process options to the wrapper binary.
-    // TODO: This should be done by CommandLineParser
-    // (together with the above TODO).
-    int moduleArgOffset = 1;
-    while (moduleArgOffset < *argc && (*argv)[moduleArgOffset][0] == '-')
+    // Check if we are directly invoking a certain module.
+    CommandLineModuleInterface *module = singleModule_;
+    if (module == NULL)
     {
-        ++moduleArgOffset;
+        // Also check for invokation through named symlinks.
+        CommandLineModuleMap::const_iterator moduleIter
+            = findModuleFromBinaryName(programInfo_);
+        if (moduleIter != modules_.end())
+        {
+            module = moduleIter->second.get();
+        }
     }
+
     bool bHelp      = false;
     bool bVersion   = false;
     bool bCopyright = false;
-    if (moduleArgOffset > 1)
+    // TODO: Print the common options into the help.
+    // TODO: It would be nice to propagate at least the -quiet option to
+    // the modules so that they can also be quiet in response to this.
+    // TODO: Consider handling -h and related options here instead of in the
+    // modules (also -hidden needs to be transfered here to make that work).
+    // That would mean that with -h, all module-specific options would get
+    // ignored.  This means that the help output would not depend on the
+    // command line, but would always show the default values (making it
+    // possible to simplify it further), but also that mdrun -h could not be
+    // used for option validation in g_tune_pme.
+    Options options(NULL, NULL);
+    if (module == NULL)
     {
-        // TODO: Print these options into the help.
-        // TODO: It would be nice to propagate at least the -quiet option to
-        // the modules so that they can also be quiet in response to this.
-        Options options(NULL, NULL);
         options.addOption(BooleanOption("h").store(&bHelp));
-        options.addOption(BooleanOption("quiet").store(&bQuiet_));
-        options.addOption(BooleanOption("version").store(&bVersion));
-        options.addOption(BooleanOption("copyright").store(&bCopyright));
-        CommandLineParser(&options).parse(&moduleArgOffset, *argv);
-        options.finish();
-        binaryInfoSettings_.extendedInfo(bVersion);
-        binaryInfoSettings_.copyright(bCopyright);
     }
+    options.addOption(BooleanOption("quiet").store(&bQuiet_));
+    options.addOption(BooleanOption("version").store(&bVersion));
+    options.addOption(BooleanOption("copyright").store(&bCopyright));
+
+    if (module == NULL)
+    {
+        // If not in single-module mode, process options to the wrapper binary.
+        // TODO: Ideally, this could be done by CommandLineParser.
+        int argcForWrapper = 1;
+        while (argcForWrapper < *argc && (*argv)[argcForWrapper][0] == '-')
+        {
+            ++argcForWrapper;
+        }
+        if (argcForWrapper > 1)
+        {
+            CommandLineParser(&options).parse(&argcForWrapper, *argv);
+        }
+        // If no action requested and there is a module specified, process it.
+        if (argcForWrapper < *argc && !bHelp && !bVersion && !bCopyright)
+        {
+            const char *moduleName = (*argv)[argcForWrapper];
+            CommandLineModuleMap::const_iterator moduleIter
+                = findModuleByName(moduleName);
+            if (moduleIter == modules_.end())
+            {
+                std::string message =
+                    formatString("'%s' is not a GROMACS command.", moduleName);
+                GMX_THROW(InvalidInputError(message));
+            }
+            module = moduleIter->second.get();
+            programInfo_.setDisplayName(
+                    programInfo_.realBinaryName() + "-" + moduleIter->first);
+            *argc -= argcForWrapper;
+            *argv += argcForWrapper;
+            // After this point, argc and argv are the same independent of
+            // which path is taken: (*argv)[0] is the module name.
+        }
+    }
+    else
+    {
+        // In single-module mode, recognize the common options also after the
+        // module name.
+        CommandLineParser(&options).skipUnknown(true).parse(argc, *argv);
+    }
+    options.finish();
+    binaryInfoSettings_.extendedInfo(bVersion);
+    binaryInfoSettings_.copyright(bCopyright);
     if (bVersion || bCopyright)
     {
         bQuiet_      = false;
         bStdOutInfo_ = true;
         return NULL;
     }
-    // If no module or help requested, show the help.
-    if (moduleArgOffset == *argc || bHelp)
+    // If no module specified and no other action, show the help.
+    // Also explicitly specifying -h for the wrapper binary goes here.
+    if (module == NULL)
     {
         *argc = 1;
         return helpModule_;
     }
-    // Find the module to run and arguments to it.
-    const char *moduleName = (*argv)[moduleArgOffset];
-    module = findModuleByName(moduleName);
-    if (module == modules_.end())
-    {
-        std::string message = formatString("'%s' is not a GROMACS command.", moduleName);
-        GMX_THROW(InvalidInputError(message));
-    }
-    programInfo_.setDisplayName(
-            programInfo_.realBinaryName() + "-" + module->first);
-    *argc -= moduleArgOffset;
-    *argv += moduleArgOffset;
-    return module->second.get();
+    return module;
 }
 
 /********************************************************************
index 80e7f7d119a15cb907660d6b40177f603c0e69f0..bdd44ed1a8141ee107ac9eed7099ec0f4f3e3bfa 100644 (file)
@@ -80,10 +80,12 @@ class CommandLineParser::Impl
 
         //! Helper object for assigning the options.
         OptionsAssigner         assigner_;
+        //! Whether to allow and skip unknown options.
+        bool                    bSkipUnknown_;
 };
 
 CommandLineParser::Impl::Impl(Options *options)
-    : assigner_(options)
+    : assigner_(options), bSkipUnknown_(false)
 {
     assigner_.setAcceptBooleanNoPrefix(true);
     assigner_.setNoStrictSectioning(true);
@@ -125,21 +127,27 @@ CommandLineParser::~CommandLineParser()
 {
 }
 
+CommandLineParser &CommandLineParser::skipUnknown(bool bEnabled)
+{
+    impl_->bSkipUnknown_ = bEnabled;
+    return *this;
+}
+
 void CommandLineParser::parse(int *argc, char *argv[])
 {
     ExceptionInitializer errors("Invalid command-line options");
     std::string          currentContext;
-    // Start in the discard phase to skip options that can't be understood.
-    bool                 bDiscard = true;
+    bool                 bInOption = false;
 
     impl_->assigner_.start();
+    int newi = 1;
     for (int i = 1; i != *argc; ++i)
     {
         const char *const arg        = argv[i];
         const char *const optionName = impl_->toOptionName(arg);
         if (optionName != NULL)
         {
-            if (!bDiscard)
+            if (bInOption)
             {
                 try
                 {
@@ -150,23 +158,36 @@ void CommandLineParser::parse(int *argc, char *argv[])
                     ex.prependContext(currentContext);
                     errors.addCurrentExceptionAsNested();
                 }
-                currentContext.clear();
             }
             currentContext = "In command-line option " + std::string(arg);
-            bDiscard       = false;
             try
             {
-                impl_->assigner_.startOption(optionName);
+                bInOption = impl_->assigner_.tryStartOption(optionName);
+                if (!bInOption)
+                {
+                    currentContext.clear();
+                    if (!impl_->bSkipUnknown_)
+                    {
+                        std::string message =
+                            "Unknown command-line option " + std::string(arg);
+                        GMX_THROW(InvalidInputError(message));
+                    }
+                }
             }
             catch (UserInputError &ex)
             {
-                bDiscard = true;
+                // If tryStartOption() throws, make sure that the rest gets
+                // ignored.
+                // TODO: Consider whether we should remove the option from the
+                // command line nonetheless, as it is recognized, but just
+                // invalid.
+                bInOption = false;
                 ex.prependContext(currentContext);
                 errors.addCurrentExceptionAsNested();
                 currentContext.clear();
             }
         }
-        else if (!bDiscard)
+        else if (bInOption)
         {
             try
             {
@@ -178,8 +199,20 @@ void CommandLineParser::parse(int *argc, char *argv[])
                 errors.addCurrentExceptionAsNested();
             }
         }
+        // Remove recognized options if applicable.
+        if (!bInOption && impl_->bSkipUnknown_)
+        {
+            argv[newi] = argv[i];
+            ++newi;
+        }
+    }
+    // Update the argc count if argv was modified.
+    if (impl_->bSkipUnknown_)
+    {
+        *argc = newi;
     }
-    if (!bDiscard)
+    // Finish the last option.
+    if (bInOption)
     {
         try
         {
index 4249dbb51115a64bfa03956cd8f8036fb25e6598..b71f126daf25168baa2526ffcf80faf9669ce811 100644 (file)
@@ -80,6 +80,24 @@ class CommandLineParser
         CommandLineParser(Options *options);
         ~CommandLineParser();
 
+        /*! \brief
+         * Makes the parser skip unknown options and keep them in \c argv.
+         *
+         * \param[in] bEnabled  Whether to skip and keep unknown options.
+         * \returns   *this
+         *
+         * Setting this option to true has dual effect: unknown options are
+         * silently skipped, and all recognized options are removed from
+         * \c argc and \c argv in parse().  These effects should be easy to
+         * separate into different flags if there is need for it.
+         *
+         * The default is false: unknown options result in exceptions and
+         * \c argc and \c argv are not modified.
+         *
+         * Does not throw.
+         */
+        CommandLineParser &skipUnknown(bool bEnabled);
+
         /*! \brief
          * Parses the command line.
          *
@@ -89,9 +107,9 @@ class CommandLineParser
          * All command-line arguments are parsed, and an aggregate exception
          * with all the detected errors is thrown in the end.
          *
-         * Currently, the input parameters are not modified, but this may
-         * change if/when support for parsing only part of the options is
-         * implemented.
+         * If skipUnknown() is false, the input parameters are not modified.
+         * If skipUnknown() is true, recognized options and their values are
+         * removed from the argument list.  \c argv[0] is never modified.
          */
         void parse(int *argc, char *argv[]);
 
index 4e8ee66f8fa04792a8852237f048b24328c77425..87cf7bd8777ffdb19b77b8a171f23a429013758a 100644 (file)
@@ -147,4 +147,28 @@ TEST_F(CommandLineParserTest, HandlesOptionsStartingWithNumbers)
     EXPECT_EQ(-12, ivalue1p_);
 }
 
+TEST_F(CommandLineParserTest, HandlesSkipUnknown)
+{
+    const char *const cmdline[] = {
+        "test", "-opt1", "-flag", "-opt2", "value", "-mvi", "2", "-mvd", "2.7", "-opt3"
+    };
+    CommandLine       args(CommandLine::create(cmdline));
+    parser_.skipUnknown(true);
+    ASSERT_NO_THROW_GMX(parser_.parse(&args.argc(), args.argv()));
+    ASSERT_NO_THROW_GMX(options_.finish());
+
+    ASSERT_EQ(5, args.argc());
+    EXPECT_STREQ("test", args.arg(0));
+    EXPECT_STREQ("-opt1", args.arg(1));
+    EXPECT_STREQ("-opt2", args.arg(2));
+    EXPECT_STREQ("value", args.arg(3));
+    EXPECT_STREQ("-opt3", args.arg(4));
+
+    EXPECT_TRUE(flag_);
+    ASSERT_EQ(1U, ivalues_.size());
+    EXPECT_EQ(2, ivalues_[0]);
+    ASSERT_EQ(1U, dvalues_.size());
+    EXPECT_DOUBLE_EQ(2.7, dvalues_[0]);
+}
+
 } // namespace
index 3c297c7f70e1edff7f131f26d41c575e23505c3a..d92ce70c2502878f6ba5dc36c3e1d13efd6b56ec 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2010,2011,2012, by the GROMACS development team, led by
+ * Copyright (c) 2010,2011,2012,2013, by the GROMACS development team, led by
  * David van der Spoel, Berk Hess, Erik Lindahl, and including many
  * others, as listed in the AUTHORS file in the top-level source
  * directory and at http://www.gromacs.org.
@@ -229,16 +229,25 @@ void OptionsAssigner::startSubSection(const char *name)
 }
 
 void OptionsAssigner::startOption(const char *name)
+{
+    if (!tryStartOption(name))
+    {
+        GMX_THROW(InvalidInputError("Unknown option"));
+    }
+}
+
+bool OptionsAssigner::tryStartOption(const char *name)
 {
     GMX_RELEASE_ASSERT(impl_->currentOption_ == NULL, "finishOption() not called");
     AbstractOptionStorage *option = impl_->findOption(name);
     if (option == NULL)
     {
-        GMX_THROW(InvalidInputError("Unknown option"));
+        return false;
     }
     option->startSet();
     impl_->currentOption_     = option;
     impl_->currentValueCount_ = 0;
+    return true;
 }
 
 void OptionsAssigner::appendValue(const std::string &value)
index 506ba395cfa8766495f2d3710ec4bd62046ead46..19ee5fcfae7fd9129543fb3063a319021677d1ff 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2010,2011,2012, by the GROMACS development team, led by
+ * Copyright (c) 2010,2011,2012,2013, by the GROMACS development team, led by
  * David van der Spoel, Berk Hess, Erik Lindahl, and including many
  * others, as listed in the AUTHORS file in the top-level source
  * directory and at http://www.gromacs.org.
@@ -127,13 +127,13 @@ class OptionsAssigner
         void setNoStrictSectioning(bool bEnabled);
 
         /*! \brief
-         * Start assigning values.
+         * Starts assigning values.
          *
          * Does not throw.
          */
         void start();
         /*! \brief
-         * Start assigning values to options in a subsection.
+         * Starts assigning values to options in a subsection.
          *
          * \param[in] name  Name of the subsection to start assigning to.
          * \throws InvalidInputError if such a subsection is not found.
@@ -142,15 +142,22 @@ class OptionsAssigner
          */
         void startSubSection(const char *name);
         /*! \brief
-         * Start assigning values for an option.
+         * Starts assigning values for an option.
          *
          * \param[in] name  Name of the option to start assigning to.
          * \throws InvalidInputError if such an option is not found, or if the
          *      option is specified more than once but doesn't support it.
-         *
-         * Strong exception safety guarantee.
          */
         void startOption(const char *name);
+        /*! \brief
+         * Starts assigning values for an option.
+         *
+         * \param[in] name  Name of the option to start assigning to.
+         * \returns   true if \p name is a valid option name.
+         * \throws InvalidInputError if the option is specified more than once
+         *      but doesn't support it.
+         */
+        bool tryStartOption(const char *name);
         /*! \brief
          * Appends a value to the value list of the current option.
          *
index 23ba787b2c6d5398767772601e3856266b12ebd7..a3397caa740db9f60847e7cd2a8d3ee72f790db9 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2012, by the GROMACS development team, led by
+ * Copyright (c) 2012,2013, by the GROMACS development team, led by
  * David van der Spoel, Berk Hess, Erik Lindahl, and including many
  * others, as listed in the AUTHORS file in the top-level source
  * directory and at http://www.gromacs.org.
@@ -47,6 +47,7 @@
 #include <new>
 #include <vector>
 
+#include "gromacs/utility/gmxassert.h"
 #include "gromacs/utility/programinfo.h"
 
 namespace gmx
@@ -119,6 +120,8 @@ CommandLine::~CommandLine()
 
 void CommandLine::append(const char *arg)
 {
+    GMX_RELEASE_ASSERT(impl_->argc_ == static_cast<int>(impl_->args_.size()),
+                       "Command-line has been modified externally");
     size_t newSize = impl_->args_.size() + 1;
     impl_->args_.reserve(newSize);
     impl_->argv_.reserve(newSize);
index 32ea8f32e0725d88853355d8400cf7754a786228..ca979042ae3241ffa196251e8a5fb632008cea89 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2012, by the GROMACS development team, led by
+ * Copyright (c) 2012,2013, by the GROMACS development team, led by
  * David van der Spoel, Berk Hess, Erik Lindahl, and including many
  * others, as listed in the AUTHORS file in the top-level source
  * directory and at http://www.gromacs.org.
@@ -64,9 +64,8 @@ namespace test
  * Note that although the interface allows passing the argc and argv pointers
  * to methods that modify them (typically as \p f(&argc(), argv())), currently
  * the CommandLine object is not in a consistent state internally if the
- * parameters are actually modified.
- * Currently, the C++ methods with this signature do not modify their
- * parameters, so this is not yet a problem.
+ * parameters are actually modified.  Reading the command line is possible
+ * afterwards, but modification is not.
  *
  * All constructors and methods that modify this class may throw an
  * std::bad_alloc.  Const methods and accessors do not throw.