diff --git a/CHANGES.txt b/CHANGES.txt index eda2eac604..d731b52428 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -41,6 +41,14 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER - Fix Appveyor scripting to install unavailable python versions when needed and use them for testing. + From Dillan Mills: + - Fix handling of AddOption for option-arguments with spaces + (when omitting the = sign or when an option takes multiple + option-arguments). Arguments unknown at the time the first pass + splits the command line into arguments and targets would put such + arguments into targets, and they remained there even after the + AddOption calls were seen. Closes #2748, #2805, #2977. + From Mats Wichmann: - Introduce some unit tests for the file locking utility routines - More clarifications in manpage Builder Methods section. @@ -68,6 +76,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER via the SConsignFile(name=None) call. - Test framework: tweak module docstrings - Test suite: end to end tests don't use assert in result checks + - Complete the work from PR #3799 on AddOption handling (original work + credited to Dillan Mills) - The qt3 tool module, deprecated since version 4.3, is removed due to the general unavailablility of the obsolete build-time Qt3 package. - A few more typing cleanups in Environment (and in one case which diff --git a/RELEASE.txt b/RELEASE.txt index 19ad44627a..7e1969f3ad 100644 --- a/RELEASE.txt +++ b/RELEASE.txt @@ -47,6 +47,11 @@ FIXES - Fix --debug=includes for case of multiple source files. +- Fix handling of AddOption for option-arguments with spaces (when omitting + the = sign or when an option takes multiple option-arguments). Arguments + unknown at the time the first pass splits the command line into arguments + and targets would put such arguments into targets, and they remained + there even after the AddOption calls were seen. - Adjust race protection for CacheDir - now uses a unique temporary directory for each writer before doing the move, instead of depending on a one-time uuid to make a path to the file. diff --git a/SCons/Script/SConsOptions.py b/SCons/Script/SConsOptions.py index 52d7fca52e..a519679156 100644 --- a/SCons/Script/SConsOptions.py +++ b/SCons/Script/SConsOptions.py @@ -405,9 +405,18 @@ def _process_long_opt(self, rargs, values) -> None: % (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) @@ -447,11 +456,13 @@ def _process_short_opts(self, rargs, values) -> None: 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: @@ -462,9 +473,19 @@ def _process_short_opts(self, rargs, values) -> None: % (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 @@ -475,6 +496,7 @@ def _process_short_opts(self, rargs, values) -> None: break + # TODO: this is now unused, remove? def reparse_local_options(self) -> None: """Re-parse the leftover command-line options. @@ -574,7 +596,7 @@ def add_local_option(self, *args, **kw) -> SConsOption: # right away. # TODO: what if dest is None? setattr(self.values.__defaults__, result.dest, result.default) - self.reparse_local_options() + self.parse_args(self.largs, self.values) if result.settable: SConsValues.settable.append(result.dest) diff --git a/test/AddOption/.exclude_tests b/test/AddOption/.exclude_tests deleted file mode 100644 index acc32f50ed..0000000000 --- a/test/AddOption/.exclude_tests +++ /dev/null @@ -1,4 +0,0 @@ -# for now, the tests showing problems with processing space-separated -# arguments are excluded, pending an implementation that doesn't fail. -args-and-targets.py -multi-arg.py diff --git a/test/AddOption/args-and-targets.py b/test/AddOption/args-and-targets.py index 900a42f89b..6581989191 100644 --- a/test/AddOption/args-and-targets.py +++ b/test/AddOption/args-and-targets.py @@ -30,14 +30,12 @@ import TestSCons -test = TestSCons.TestSCons() +test = TestSCons.TestSCons(match = TestSCons.match_re_dotall) -test.write( - 'SConstruct', - """\ +test.write('SConstruct', """\ DefaultEnvironment(tools=[]) -env = Environment(tools=[]) AddOption( + '-x', '--extra', nargs=1, dest='extra', @@ -53,8 +51,391 @@ ) # arg using = -test.run('-Q -q --extra=A TARG', status=1, stdout="A\n['TARG']\n") +test.run('-Q -q --extra=A TARG', status=1, stdout=r"A\n\['TARG'\]\n") # arg not using = -test.run('-Q -q --extra A TARG', status=1, stdout="A\n['TARG']\n") +test.run('-Q -q --extra A TARG', status=1, stdout=r"A\n\['TARG'\]\n") +# short arg with space +test.run('-Q -q -x A TARG', status=1, stdout=r"A\n\['TARG'\]\n") +# short arg with no space +test.run('-Q -q -xA TARG', status=1, stdout=r"A\n\['TARG'\]\n") + +test.write('SConstruct1', """\ +DefaultEnvironment(tools=[]) +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( + '-f SConstruct1 -Q -q --extra=A B TARG1 -x C D TARG2 -xE F TARG3 --extra G H TARG4', + status=1, + stdout=r"\[\('A', 'B'\), \('C', 'D'\), \('E', 'F'\), \('G', 'H'\)\]\n\['TARG1', 'TARG2', 'TARG3', 'TARG4'\]\n", +) + +test.write('SConstruct2', """\ +DefaultEnvironment(tools=[]) +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) +print(ARGUMENTS.get('A', None)) +""", +) + +# opt value and target are same name +test.run( + arguments='-f SConstruct2 -Q -q --extra=TARG1 TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q --extra TARG1 TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -xTARG1 TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -x TARG1 TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) + +# target first +test.run( + arguments='-f SConstruct2 -Q -q TARG1 --extra=TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q TARG1 --extra TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q TARG1 -xTARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q TARG1 -x TARG1', + status=1, + stdout=r"TARG1\n\['TARG1'\]\nNone\n", +) + +# equals in opt value +test.run( + arguments='-f SConstruct2 -Q -q --extra=A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q --extra A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -xA=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nNone\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -x A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nNone\n", +) + +# equals in opt value and a different argument +test.run( + arguments='-f SConstruct2 -Q -q --extra=A=B A=C TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) +test.run( + arguments='-f SConstruct2 -Q -q --extra A=B A=C TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -xA=B A=C TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -x A=B A=C TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) + +# different argument first +test.run( + arguments='-f SConstruct2 -Q -q A=C --extra=A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) +test.run( + arguments='-f SConstruct2 -Q -q A=C --extra A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) +test.run( + arguments='-f SConstruct2 -Q -q A=C -xA=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) +test.run( + arguments='-f SConstruct2 -Q -q A=C -x A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nC\n", +) + +# equals in opt value and the same as an argument +test.run( + arguments='-f SConstruct2 -Q -q --extra=A=B A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) +test.run( + arguments='-f SConstruct2 -Q -q --extra A=B A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -xA=B A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) +test.run( + arguments='-f SConstruct2 -Q -q -x A=B A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) + +# same argument first +test.run( + arguments='-f SConstruct2 -Q -q A=B --extra=A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) +test.run( + arguments='-f SConstruct2 -Q -q A=B --extra A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) +test.run( + arguments='-f SConstruct2 -Q -q A=B -xA=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) +test.run( + arguments='-f SConstruct2 -Q -q A=B -x A=B TARG1', + status=1, + stdout=r"A=B\n\['TARG1'\]\nB\n", +) + +test.write('SConstruct3', """\ +DefaultEnvironment(tools=[]) +AddOption( + '-x', + '--extra', + nargs=1, + dest='extra', + action='store', + type='string', + metavar='ARG1', + default=(), + help='An argument to the option', +) +if 'A' in BUILD_TARGETS: + BUILD_TARGETS.append('B') +print(str(GetOption('extra'))) +print(BUILD_TARGETS) +""", +) + +# Nested target +test.run( + arguments='-f SConstruct3 -Q -q -x A TARG1', status=1, stdout=r"A\n\['TARG1'\]\n" +) +test.run( + arguments='-f SConstruct3 -Q -q -x A A TARG1', + status=1, + stdout=r"A\n\['A', 'TARG1', 'B'\]\n", +) +test.run( + arguments='-f SConstruct3 -Q -q A -x A TARG1', + status=1, + stdout=r"A\n\['A', 'TARG1', 'B'\]\n", +) + +test.write('SConstruct4', """\ +DefaultEnvironment(tools=[]) +AddOption( + '-x', + '--extra', + nargs=1, + dest='extra', + action='store', + type='string', + metavar='ARG1', + default=(), + help='An argument to the option', +) +if 'A' in BUILD_TARGETS: + AddOption( + '--foo', + nargs=1, + dest='foo', + action='store', + type='string', + metavar='FOO1', + default=(), + help='An argument to the option', + ) +print(str(GetOption('extra'))) +print(str(GetOption('foo'))) +print(COMMAND_LINE_TARGETS) +""", +) + +# nested option +expect = r"""AttributeError: 'Values' object has no attribute 'foo': + File ".+SConstruct4", line \d+: + print\(str\(GetOption\('foo'\)\)\) + File "[^"]+Main.py", line \d+: + return getattr\(OptionsParser.values, name\) + File "[^"]+SConsOptions.py", line \d+: + return getattr\(self.__dict__\['__defaults__'\], attr\) +""" +test.run( + arguments='-f SConstruct4 -Q -q -x A --foo=C TARG1', + status=2, + stdout=r"A\n", + stderr=expect, +) +test.run( + arguments='-f SConstruct4 -Q -q -x A A --foo=C TARG1', + status=1, + stdout=r"A\nC\n\['A', 'TARG1'\]\n", +) +test.run( + arguments='-f SConstruct4 -Q -q A -x A --foo=C TARG1', + status=1, + stdout=r"A\nC\n\['A', 'TARG1'\]\n", +) + +############################################ +################ Fail Case ################# +############################################ + +test.write('SConstruct5', """\ +DefaultEnvironment(tools=[]) +AddOption( + '-x', + '--extra', + nargs=1, + dest='extra', + action='store', + type='string', + metavar='ARG1', + default=(), + help='An argument to the option', +) +if 'A' in BUILD_TARGETS: + BUILD_TARGETS.append('B') +print(str(GetOption('extra'))) +print(BUILD_TARGETS) +""") + +# Nested target +test.run( + arguments='-f SConstruct5 -Q -q -x A TARG1', status=1, stdout=r"A\n\['TARG1'\]\n" +) +test.run( + arguments='-f SConstruct5 -Q -q -x A A TARG1', + status=1, + stdout=r"A\n\['A', 'TARG1', 'B'\]\n", +) +test.run( + arguments='-f SConstruct5 -Q -q A -x A TARG1', + status=1, + stdout=r"A\n\['A', 'TARG1', 'B'\]\n", +) + +test.write('SConstruct6', """\ +DefaultEnvironment(tools=[]) +AddOption( + '-x', + '--extra', + nargs=1, + dest='extra', + action='store', + type='string', + metavar='ARG1', + default=(), + help='An argument to the option', +) +if 'A' in BUILD_TARGETS: + AddOption( + '--foo', + nargs=1, + dest='foo', + action='store', + type='string', + metavar='FOO1', + default=(), + help='An argument to the option', + ) +print(str(GetOption('extra'))) +print(str(GetOption('foo'))) +print(COMMAND_LINE_TARGETS) +""") + +# nested option +expect=r"""AttributeError: 'Values' object has no attribute 'foo': + File ".+SConstruct6", line \d+: + print\(str\(GetOption\('foo'\)\)\) + File "[^"]+Main.py", line \d+: + return getattr\(OptionsParser.values, name\) + File "[^"]+SConsOptions.py", line \d+: + return getattr\(self.__dict__\['__defaults__'\], attr\) +""" +test.run( + arguments='-f SConstruct6 -Q -q -x A --foo=C TARG1', + status=2, + stdout=r"A\n", + stderr=expect, +) +test.run( + arguments='-f SConstruct6 -Q -q -x A A --foo=C TARG1', + status=1, + stdout=r"A\nC\n\['A', 'TARG1'\]\n", +) +test.run( + arguments='-f SConstruct6 -Q -q A -x A --foo=C TARG1', + status=1, + stdout=r"A\nC\n\['A', 'TARG1'\]\n", +) test.pass_test() diff --git a/test/AddOption/multi-arg.py b/test/AddOption/multi-arg.py index f625277a18..26d18fba2e 100644 --- a/test/AddOption/multi-arg.py +++ b/test/AddOption/multi-arg.py @@ -33,46 +33,50 @@ test = TestSCons.TestSCons() # First, test an option with nargs=2 and no others: -test.write( - 'SConstruct', - """\ +test.write('SConstruct', """\ DefaultEnvironment(tools=[]) -AddOption('--extras', - nargs=2, - dest='extras', - action='store', - type='string', - metavar='FILE1 FILE2', - default=(), - help='two extra files to install') -print(str(GetOption('extras'))) -""", +AddOption( + '-x', + '--extras', + nargs=2, + dest='extras', + action='store', + type='string', + metavar='FILE1 FILE2', + default=(), + help='two extra files to install', ) +print(str(GetOption('extras'))) +print(COMMAND_LINE_TARGETS) +""") # no args -test.run('-Q -q .', stdout="()\n") +test.run('-Q -q .', stdout="()\n['.']\n") # one arg, should fail -test.run( - '-Q -q . --extras A', - status=2, - stderr="""\ +test.run('-Q -q . --extras A', status=2, stderr="""\ usage: scons [OPTIONS] [VARIABLES] [TARGETS] SCons Error: --extras option requires 2 arguments -""", -) +""") +#one arg, short option +test.run('-Q -q . -x A', status=2, stderr="""\ +usage: scons [OPTIONS] [VARIABLES] [TARGETS] + +SCons Error: -x option requires 2 arguments +""") # two args -test.run('-Q -q . --extras A B', status=1, stdout="('A', 'B')\n") +test.run('-Q -q . --extras A B', stdout="('A', 'B')\n['.']\n") +# two args, short option +test.run('-Q -q . -x A B', stdout="('A', 'B')\n['.']\n") # -- means the rest are not processed as args -test.run('-Q -q . -- --extras A B', status=1, stdout="()\n") +test.run('-Q -q . -- --extras A B', status=1, stdout="()\n['.', '--extras', 'A', 'B']\n") # Now test what has been a bug: another option is # also defined, this impacts the collection of args for the nargs>1 opt -test.write( - 'SConstruct', - """\ +test.write('SConstruct', """\ DefaultEnvironment(tools=[]) AddOption( + '-P', '--prefix', nargs=1, dest='prefix', @@ -82,6 +86,7 @@ help='installation prefix', ) AddOption( + '-x', '--extras', nargs=2, dest='extras', @@ -93,26 +98,38 @@ ) print(str(GetOption('prefix'))) print(str(GetOption('extras'))) -""", -) - -# no options -test.run('-Q -q .', stdout="None\n()\n") -# one single-arg option -test.run('-Q -q . --prefix=/home/foo', stdout="/home/foo\n()\n") -# one two-arg option -test.run('-Q -q . --extras A B', status=2, stdout="None\n('A', 'B')\n") -# single-arg option followed by two-arg option +print(COMMAND_LINE_TARGETS) +""") +# no opts +test.run('-Q -q .', stdout="None\n()\n['.']\n") +# first opt long, one arg +test.run('-Q -q . --prefix=/home/foo', stdout="/home/foo\n()\n['.']\n") +test.run('-Q -q . --prefix /home/foo', stdout="/home/foo\n()\n['.']\n") +# first opt short, one arg +test.run('-Q -q . -P/home/foo', stdout="/home/foo\n()\n['.']\n") +test.run('-Q -q . -P /home/foo', stdout="/home/foo\n()\n['.']\n") +# second opt long, two args +test.run('-Q -q . --extras=A B', stdout="None\n('A', 'B')\n['.']\n") +test.run('-Q -q . --extras A B', stdout="None\n('A', 'B')\n['.']\n") +# second opt short, two args +test.run('-Q -q . -xA B', stdout="None\n('A', 'B')\n['.']\n") +test.run('-Q -q . -x A B', stdout="None\n('A', 'B')\n['.']\n") +# both opts long +test.run('-Q -q . --prefix=/home/foo --extras=A B', stdout="/home/foo\n('A', 'B')\n['.']\n") +test.run('-Q -q . --prefix /home/foo --extras A B', stdout="/home/foo\n('A', 'B')\n['.']\n") +# both opts short +test.run('-Q -q . -P/home/foo -xA B', stdout="/home/foo\n('A', 'B')\n['.']\n") +test.run('-Q -q . -P /home/foo -x A B', stdout="/home/foo\n('A', 'B')\n['.']\n") +# don't process test.run( - '-Q -q . --prefix=/home/foo --extras A B', + '-Q -q . -- --prefix=/home/foo --extras=A B', status=1, - stdout="/home/foo\n('A', 'B')\n", + stdout="None\n()\n['.', '--prefix=/home/foo', '--extras=A', 'B']\n", ) -# two-arg option followed by single-arg option test.run( - '-Q -q . --extras A B --prefix=/home/foo', + '-Q -q . -- --prefix /home/foo --extras A B', status=1, - stdout="/home/foo\n('A', 'B')\n", + stdout="None\n()\n['.', '--prefix', '/home/foo', '--extras', 'A', 'B']\n", ) test.pass_test()