Merge pull request #4596 from mwichmann/AddOption-option-obj

Fix some AddOption issues

CHANGES.txt

@@ -23,6 +23,14 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       the test fails. The rest is cleanup and type annotations. Be more
       careful that the returns from stderr() and stdout(), which *can*
       return None, are not used without checking.
+    - The optparse add_option method supports an additional calling style
+      that is not directly described in SCons docs, but is included
+      by reference ("see the optparse documentation for details"):
+      a single arg consisting of a premade option object. Because optparse
+      detects that case based on seeing zero kwargs and we always
+      added at least one (default=) that would fail for AddOption. Fix
+      for consistency, but don't advertise it further - not addewd to
+      manpage synoposis/description.
     - Temporary files created by TempFileMunge() are now cleaned up on
       scons exit, instead of at the time they're used.  Fixes #4595.
     - Override envirionments, created when giving construction environment

RELEASE.txt

@@ -51,6 +51,11 @@ IMPROVEMENTS
   documentation:  performance improvements (describe the circumstances
   under which they would be observed), or major code cleanups
 
+- For consistency with the optparse "add_option" method, AddOption accepts
+  an SConsOption object as a single argument (this failed previouly).
+  Calling AddOption with the full set of arguments (option names and
+  attributes) to set up the option is still the recommended approach.
+
 PACKAGING
 ---------
 

SCons/Script/Main.py

@@ -34,6 +34,7 @@
 import SCons.compat
 
 import importlib.util
+import optparse
 import os
 import re
 import sys
@@ -59,8 +60,7 @@
 import SCons.Util
 import SCons.Warnings
 import SCons.Script.Interactive
-if TYPE_CHECKING:
-    from SCons.Script import SConsOption
+from .SConsOptions import SConsOption
 from SCons.Util.stats import count_stats, memory_stats, time_stats, ENABLE_JSON, write_scons_stats_file, JSON_OUTPUT_FILE
 
 from SCons import __version__ as SConsVersion
@@ -508,29 +508,41 @@ def __getattr__(self, attr):
     # TODO: to quiet checkers, FakeOptionParser should also define
     #   raise_exception_on_error, preserve_unknown_options, largs and parse_args
 
-    def add_local_option(self, *args, **kw) -> "SConsOption":
+    def add_local_option(self, *args, **kw) -> SConsOption:
         pass
 
 
 OptionsParser = FakeOptionParser()
 
-def AddOption(*args, settable: bool = False, **kw) -> "SConsOption":
+def AddOption(*args, **kw) -> SConsOption:
     """Add a local option to the option parser - Public API.
 
-    If the *settable* parameter is true, the option will be included in the
-    list of settable options; all other keyword arguments are passed on to
-    :meth:`~SCons.Script.SConsOptions.SConsOptionParser.add_local_option`.
+    If the SCons-specific *settable* kwarg is true (default ``False``),
+    the option will allow calling :func:``SetOption`.
 
     .. versionchanged:: 4.8.0
        The *settable* parameter added to allow including the new option
-       to the table of options eligible to use :func:`SetOption`.
-
+       in the table of options eligible to use :func:`SetOption`.
     """
+    settable = kw.get('settable', False)
+    if len(args) == 1 and isinstance(args[0], SConsOption):
+        # If they passed an SConsOption object, ignore kw - the underlying
+        # add_option method relies on seeing zero kwargs to recognize this.
+        # Since we don't support an omitted default, overrwrite optparse's
+        # marker to get the same effect as setting it in kw otherwise.
+        optobj = args[0]
+        if optobj.default is optparse.NO_DEFAULT:
+            optobj.default = None
+        # make sure settable attribute exists; positive setting wins
+        attr_settable = getattr(optobj, "settable")
+        if attr_settable is None or settable > attr_settable:
+            optobj.settable = settable
+        return OptionsParser.add_local_option(*args)
+
     if 'default' not in kw:
         kw['default'] = None
-    kw['settable'] = settable
-    result = OptionsParser.add_local_option(*args, **kw)
-    return result
+    kw['settable'] = settable  # just to make sure it gets set
+    return OptionsParser.add_local_option(*args, **kw)
 
 def GetOption(name: str):
     """Get the value from an option - Public API."""

SCons/Script/SConsOptions.py

@@ -240,6 +240,11 @@ class SConsOption(optparse.Option):
     New function :meth:`_check_nargs_optional` implements the ``nargs=?``
     syntax from :mod:`argparse`, and is added to the ``CHECK_METHODS`` list.
     Overridden :meth:`convert_value` supports this usage.
+
+    .. versionchanged:: NEXT_RELEASE
+       The *settable* attribute is added to ``ATTRS``, allowing it to be
+       set in the option. A parameter to mark the option settable was added
+       in 4.8.0, but was not initially made part of the option object itself.
     """
     # can uncomment to have a place to trap SConsOption creation for debugging:
     # def __init__(self, *args, **kwargs):
@@ -274,10 +279,11 @@ def _check_nargs_optional(self) -> None:
             fmt = "option %s: nargs='?' is incompatible with short options"
             raise SCons.Errors.UserError(fmt % self._short_opts[0])
 
+    ATTRS = optparse.Option.ATTRS + ['settable']  # added for SCons
     CHECK_METHODS = optparse.Option.CHECK_METHODS
     if CHECK_METHODS is None:
         CHECK_METHODS = []
-    CHECK_METHODS = CHECK_METHODS + [_check_nargs_optional]  # added for SCons
+    CHECK_METHODS += [_check_nargs_optional]  # added for SCons
     CONST_ACTIONS = optparse.Option.CONST_ACTIONS + optparse.Option.TYPED_ACTIONS
 
 
@@ -481,19 +487,19 @@ def add_local_option(self, *args, **kw) -> SConsOption:
         by default "local" (project-added) options are not eligible for
         :func:`~SCons.Script.Main.SetOption` calls.
 
-        .. versionchanged:: 4.8.0
-           Added special handling of *settable*.
-
+        .. versionchanged:: NEXT_VERSION
+           If the option's *settable* attribute is true, it is added to
+           the :attr:`SConsValues.settable` list. *settable* handling was
+           added in 4.8.0, but was not made an option attribute at the time.
         """
         group: SConsOptionGroup
         try:
             group = self.local_option_group
         except AttributeError:
             group = SConsOptionGroup(self, 'Local Options')
-            group = self.add_option_group(group)
+            self.add_option_group(group)
             self.local_option_group = group
 
-        settable = kw.pop('settable')
         # this gives us an SConsOption due to the setting of self.option_class
         result = group.add_option(*args, **kw)
         if result:
@@ -508,7 +514,7 @@ def add_local_option(self, *args, **kw) -> SConsOption:
             # TODO: what if dest is None?
             setattr(self.values.__defaults__, result.dest, result.default)
             self.reparse_local_options()
-            if settable:
+            if result.settable:
                 SConsValues.settable.append(result.dest)
 
         return result

test/AddOption/basic.py

@@ -33,6 +33,8 @@
 test = TestSCons.TestSCons()
 
 test.write('SConstruct', """\
+from SCons.Script.SConsOptions import SConsOption
+
 DefaultEnvironment(tools=[])
 env = Environment(tools=[])
 AddOption(
@@ -55,6 +57,9 @@
     action="store_true",
     help="try SetOption of 'prefix' to '/opt/share'"
 )
+z_opt = SConsOption("--zcount", type="int", nargs=1, settable=True)
+AddOption(z_opt)
+
 f = GetOption('force')
 if f:
     f = "True"
@@ -63,6 +68,8 @@
 if GetOption('set'):
     SetOption('prefix', '/opt/share')
     print(GetOption('prefix'))
+if GetOption('zcount'):
+    print(GetOption('zcount'))
 """)
 
 test.run('-Q -q .', stdout="None\nNone\n")
@@ -73,6 +80,8 @@
 test.run('-Q -q . --set', stdout="None\nNone\n/opt/share\n")
 # but the "command line wins" rule is not violated
 test.run('-Q -q . --set --prefix=/home/foo', stdout="None\n/home/foo\n/home/foo\n")
+# also try in case we pass a premade option object to AddOption
+test.run('-Q -q . --zcount=22', stdout="None\nNone\n22\n")
 
 test.pass_test()