Fix issue with spaces causing option values to be duplicated as targets, SCons#3436, SCons#3798

Author: Dillan Mills

CHANGES.txt

@@ -45,6 +45,8 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
 
   From Dillan Mills:
     - Fix support for short options (`-x`). 
+    - Add support for long and short options with nargs != '?' to contain spaces:
+      - `--extra A B C` and `-x A B C` will both work as expected
 
 
 RELEASE 4.0.1 - Mon, 16 Jul 2020 16:06:40 -0700

SCons/Script/SConsOptions.py

@@ -291,11 +291,14 @@ def _process_short_opts(self, rargs, values):
                 raise
 
             if option.takes_value():
+                had_explicit_value = False
+
                 # Any characters left in arg?  Pretend they're the
                 # next arg, and stop consuming characters of arg.
                 if i < len(arg):
                     rargs.insert(0, arg[i:])
                     stop = True
+                    had_explicit_value = True
 
                 nargs = option.nargs
                 if len(rargs) < nargs:
@@ -306,9 +309,19 @@ def _process_short_opts(self, rargs, values):
                                    % (opt, nargs))
                 elif nargs == 1:
                     value = rargs.pop(0)
+                    if not had_explicit_value:
+                        SCons.Script._Remove_Target(value)
+                        if '=' in value:
+                            SCons.Script._Remove_Argument(value)
                 else:
                     value = tuple(rargs[0:nargs])
                     del rargs[0:nargs]
+                    for i in range(len(value)):
+                        if not had_explicit_value or i > 0:
+                            SCons.Script._Remove_Target(value[i])
+                            if '=' in value[i]:
+                                SCons.Script._Remove_Argument(value[i])
+
 
             else:                       # option doesn't take a value
                 value = None
@@ -379,9 +392,18 @@ def _process_long_opt(self, rargs, values):
                                % (opt, nargs))
             elif nargs == 1:
                 value = rargs.pop(0)
+                if not had_explicit_value:
+                    SCons.Script._Remove_Target(value)
+                    if '=' in value:
+                        SCons.Script._Remove_Argument(value)
             else:
                 value = tuple(rargs[0:nargs])
                 del rargs[0:nargs]
+                for i in range(len(value)):
+                    if not had_explicit_value or i > 0:
+                        SCons.Script._Remove_Target(value[i])
+                        if '=' in value[i]:
+                            SCons.Script._Remove_Argument(value[i])
 
         elif had_explicit_value:
             self.error(_("%s option does not take a value") % opt)
@@ -391,69 +413,6 @@ def _process_long_opt(self, rargs, values):
 
         option.process(opt, value, values, self)
 
-    def reparse_local_options(self):
-        """ Re-parse the leftover command-line options.
-
-        Parse options stored in `self.largs`, so that any value
-        overridden on the command line is immediately available
-        if the user turns around and does a :func:`GetOption` right away.
-
-        We mimic the processing of the single args
-        in the original OptionParser :func:`_process_args`, but here we
-        allow exact matches for long-opts only (no partial argument names!).
-        Otherwise there could be  problems in :func:`add_local_option`
-        below. When called from there, we try to reparse the
-        command-line arguments that
-
-        1. haven't been processed so far (`self.largs`), but
-        2. are possibly not added to the list of options yet.
-
-        So, when we only have a value for "--myargument" so far,
-        a command-line argument of "--myarg=test" would set it,
-        per the behaviour of :func:`_match_long_opt`,
-        which allows for partial matches of the option name,
-        as long as the common prefix appears to be unique.
-        This would lead to further confusion, because we might want
-        to add another option "--myarg" later on (see issue #2929).
-
-        """
-        rargs = []
-        largs_restore = []
-        # Loop over all remaining arguments
-        skip = False
-        for l in self.largs:
-            if skip:
-                # Accept all remaining arguments as they are
-                largs_restore.append(l)
-            else:
-                if len(l) > 2 and l[0:2] == "--":
-                    # Check long option
-                    lopt = (l,)
-                    if "=" in l:
-                        # Split into option and value
-                        lopt = l.split("=", 1)
-
-                    if lopt[0] in self._long_opt:
-                        # Argument is already known
-                        rargs.append('='.join(lopt))
-                    else:
-                        # Not known yet, so reject for now
-                        largs_restore.append('='.join(lopt))
-                else:
-                    if l == "--" or l == "-":
-                        # Stop normal processing and don't
-                        # process the rest of the command-line opts
-                        largs_restore.append(l)
-                        skip = True
-                    else:
-                        rargs.append(l)
-
-        # Parse the filtered list
-        self.parse_args(rargs, self.values)
-        # Restore the list of remaining arguments for the
-        # next call of AddOption/add_local_option...
-        self.largs = self.largs + largs_restore
-
     def add_local_option(self, *args, **kw):
         """
         Adds a local option to the parser.
@@ -481,8 +440,8 @@ def add_local_option(self, *args, **kw):
             # available if the user turns around and does a GetOption()
             # right away.
             setattr(self.values.__defaults__, result.dest, result.default)
-            self.reparse_local_options()
-
+            self.parse_args(self.largs, self.values)
+            
         return result
 
 class SConsIndentedHelpFormatter(optparse.IndentedHelpFormatter):

SCons/Script/__init__.py

@@ -215,6 +215,13 @@ def _Add_Arguments(alist):
         ARGUMENTS[a] = b
         ARGLIST.append((a, b))
 
+def _Remove_Argument(aarg):
+    if aarg:
+        a, b = aarg.split('=', 1)
+        ARGUMENTS.pop(a, None)
+        if (a, b) in ARGLIST:
+            ARGLIST.remove((a, b))
+
 def _Add_Targets(tlist):
     if tlist:
         COMMAND_LINE_TARGETS.extend(tlist)
@@ -225,6 +232,15 @@ def _Add_Targets(tlist):
         _build_plus_default._add_Default = _build_plus_default._do_nothing
         _build_plus_default._clear = _build_plus_default._do_nothing
 
+def _Remove_Target(targ):
+    if targ:
+        if targ in COMMAND_LINE_TARGETS:
+            COMMAND_LINE_TARGETS.remove(targ)
+        if targ in BUILD_TARGETS:
+            BUILD_TARGETS.remove(targ)
+        if targ in _build_plus_default:
+            _build_plus_default.remove(targ)
+
 def _Set_Default_Targets_Has_Been_Called(d, fs):
     return DEFAULT_TARGETS
 

test/AddOption/args-and-targets.py

@@ -0,0 +1,111 @@
+#!/usr/bin/env python
+#
+# __COPYRIGHT__
+#
+# Permission is hereby granted, free of charge, to any person obtaining
+# a copy of this software and associated documentation files (the
+# "Software"), to deal in the Software without restriction, including
+# without limitation the rights to use, copy, modify, merge, publish,
+# distribute, sublicense, and/or sell copies of the Software, and to
+# permit persons to whom the Software is furnished to do so, subject to
+# the following conditions:
+#
+# The above copyright notice and this permission notice shall be included
+# in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
+# KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+# WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+#
+
+__revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
+
+"""
+Verify that when an option is specified which takes args,
+those do not end up treated as targets.
+"""
+
+import TestSCons
+
+test = TestSCons.TestSCons()
+
+test.write('SConstruct', """\
+env = Environment()
+AddOption('-x', '--extra',
+          nargs=1,
+          dest='extra',
+          action='store',
+          type='string',
+          metavar='ARG1',
+          default=(),
+          help='An argument to the option')
+print(str(GetOption('extra')))
+print(COMMAND_LINE_TARGETS)
+""")
+
+# arg using =
+test.run('-Q -q --extra=A TARG', status=1, stdout="A\n['TARG']\n")
+# arg not using =
+test.run('-Q -q --extra A TARG', status=1, stdout="A\n['TARG']\n")
+# short arg with space
+test.run('-Q -q -x A TARG', status=1, stdout="A\n['TARG']\n")
+# short arg with no space
+test.run('-Q -q -xA TARG', status=1, stdout="A\n['TARG']\n")
+
+test.write('SConstruct', """\
+env = Environment()
+AddOption('-x', '--extra',
+          nargs=2,
+          dest='extra',
+          action='append',
+          type='string',
+          metavar='ARG1',
+          default=[],
+          help='An argument to the option')
+print(str(GetOption('extra')))
+print(COMMAND_LINE_TARGETS)
+""")
+
+# many args and opts
+test.run('-Q -q --extra=A B TARG1 -x C D TARG2 -xE F TARG3 --extra G H TARG4', 
+        status=1, stdout="[('A', 'B'), ('C', 'D'), ('E', 'F'), ('G', 'H')]\n['TARG1', 'TARG2', 'TARG3', 'TARG4']\n")
+
+test.write('SConstruct', """\
+env = Environment()
+AddOption('-x', '--extra',
+          nargs=1,
+          dest='extra',
+          action='store',
+          type='string',
+          metavar='ARG1',
+          default=(),
+          help='An argument to the option')
+print(str(GetOption('extra')))
+print(COMMAND_LINE_TARGETS)
+""")
+
+# opt value and target are same name
+test.run('-Q -q --extra=TARG1 TARG1', status=1, stdout="TARG1\n['TARG1']\n")
+test.run('-Q -q --extra TARG1 TARG1', status=1, stdout="TARG1\n['TARG1']\n")
+test.run('-Q -q -xTARG1 TARG1', status=1, stdout="TARG1\n['TARG1']\n")
+test.run('-Q -q -x TARG1 TARG1', status=1, stdout="TARG1\n['TARG1']\n")
+
+# equals in opt value
+test.run('-Q -q --extra=A=B TARG1', status=1, stdout="A=B\n['TARG1']\n")
+test.run('-Q -q --extra A=B TARG1', status=1, stdout="A=B\n['TARG1']\n")
+test.run('-Q -q -xA=B TARG1', status=1, stdout="A=B\n['TARG1']\n")
+test.run('-Q -q -x A=B TARG1', status=1, stdout="A=B\n['TARG1']\n")
+
+
+
+test.pass_test()
+
+# Local Variables:
+# tab-width:4
+# indent-tabs-mode:nil
+# End:
+# vim: set expandtab tabstop=4 shiftwidth=4: