Merge pull request #4591 from mwichmann/maint/PkgVariable

PackageVariable now returns the default on "true"

CHANGES.txt

@@ -13,6 +13,10 @@ NOTE: Python 3.6 support is deprecated and will be dropped in a future release.
 RELEASE  VERSION/DATE TO BE FILLED IN LATER
 
   From Mats Wichmann:
+    - PackageVariable now does what the documentation always said it does
+      if the variable is used on the command line with one of the enabling
+      string as the value: the variable's default value is produced (previously
+      it always produced True in this case).
     - Minor updates to test framework. The functional change is that
       test.must_exist() and test.must_exist_one_of() now take an optional
       'message' keyword argument which is passed on to fail_test() if

RELEASE.txt

@@ -32,7 +32,10 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY
 FIXES
 -----
 
-- List fixes of outright bugs
+- PackageVariable now does what the documentation always said it does
+  if the variable is used on the command line with one of the enabling
+  string as the value: the variable's default value is produced (previously
+  it always produced True in this case).
 
 IMPROVEMENTS
 ------------

SCons/Variables/ListVariable.py

@@ -54,6 +54,7 @@
 # since elements can occur twice.
 
 import collections
+import functools
 from typing import Callable, List, Optional, Tuple, Union
 
 import SCons.Util
@@ -223,7 +224,8 @@ def ListVariable(
         default = ','.join(default)
     help = '\n    '.join(
         (help, '(all|none|comma-separated list of names)', names_str))
-    return key, help, default, validator, lambda val: _converter(val, names, map)
+    converter = functools.partial(_converter, allowedElems=names, mapdict=map)
+    return key, help, default, validator, converter
 
 # Local Variables:
 # tab-width:4

SCons/Variables/PackageVariable.py

@@ -51,6 +51,7 @@
 """
 
 import os
+import functools
 from typing import Callable, Optional, Tuple, Union
 
 import SCons.Errors
@@ -60,21 +61,33 @@
 ENABLE_STRINGS = ('1', 'yes', 'true',  'on', 'enable', 'search')
 DISABLE_STRINGS = ('0', 'no',  'false', 'off', 'disable')
 
-def _converter(val: Union[str, bool]) -> Union[str, bool]:
-    """Convert package variables.
+def _converter(val: Union[str, bool], default: str) -> Union[str, bool]:
+    """Convert a package variable.
 
-    Returns True or False if one of the recognized truthy or falsy
-    values is seen, else return the value unchanged (expected to
-    be a path string).
+    Returns *val* if it looks like a path string, and ``False`` if it
+    is a disabling string. If *val* is an enabling string, returns
+    *default* unless *default* is an enabling or disabling string,
+    in which case ignore *default* and return ``True``.
+
+    .. versionchanged: NEXT_RELEASE
+       Now returns the default in case of a truthy value, matching what the
+       public documentation always claimed, except if the default looks
+       like one of the true/false strings.
     """
     if isinstance(val, bool):
-        # mainly for the subst=False case: default might be a bool
-        return val
-    lval = val.lower()
-    if lval in ENABLE_STRINGS:
-        return True
+        # check for non-subst case, so we don't lower() a bool.
+        lval = str(val).lower()
+    else:
+        lval = val.lower()
     if lval in DISABLE_STRINGS:
         return False
+    if lval in ENABLE_STRINGS:
+        # Can't return the default if it is one of the enable/disable strings;
+        # test code expects a bool.
+        if default in ENABLE_STRINGS:
+            return True
+        else:
+            return default
     return val
 
 
@@ -83,8 +96,8 @@ def _validator(key: str, val, env, searchfunc) -> None:
 
     Checks that if a path is given as the value, that pathname actually exists.
     """
-    # NOTE: searchfunc is currently undocumented and unsupported
     if env[key] is True:
+        # NOTE: searchfunc is not in the documentation.
         if searchfunc:
             env[key] = searchfunc(key, val)
             # TODO: need to check path, or be sure searchfunc raises.
@@ -103,21 +116,16 @@ def PackageVariable(
     a tuple with the correct converter and validator appended.
     The result is usable as input to :meth:`~SCons.Variables.Variables.Add`.
 
-    A 'package list' variable may either be a truthy string from
+    A 'package list' variable may be specified as a truthy string from
     :const:`ENABLE_STRINGS`, a falsy string from
-    :const:`DISABLE_STRINGS`, or a pathname string.
+    :const:`DISABLE_STRINGS`, or as a pathname string.
     This information is appended to *help* using only one string
     each for truthy/falsy.
     """
-    # NB: searchfunc is currently undocumented and unsupported
     help = '\n    '.join((help, f'( yes | no | /path/to/{key} )'))
-    return (
-        key,
-        help,
-        default,
-        lambda k, v, e: _validator(k, v, e, searchfunc),
-        _converter,
-    )
+    validator = functools.partial(_validator, searchfunc=searchfunc)
+    converter = functools.partial(_converter, default=default)
+    return key, help, default, validator, converter
 
 # Local Variables:
 # tab-width:4

doc/man/scons.xml

@@ -5135,22 +5135,21 @@ vars.FormatVariableHelpText = my_format
 
 
 <para>
-To make it more convenient to describe custom variables,
-&SCons; provides some pre-defined variable types,
+&SCons; provides five pre-defined variable types,
 acessible through factory functions that generate
-a tuple appropriate for directly passing to
-the &Add; or &AddVariables; method:</para>
+a tuple appropriate for directly passing to the
+<link linkend='v-Add'><function>Add</function></link>
+<link linkend='v-AddVariables'><function>AddVariables</function></link>
+methods.
+</para>
 
 <variablelist>
   <varlistentry id="v-BoolVariable">
   <term><function>BoolVariable</function>(<parameter>key, help, default</parameter>)</term>
   <listitem>
 <para>
-Set up a Boolean variable.
-The variable will use
-the specified name
-<parameter>key</parameter>,
-have a default value of
+Set up a Boolean variable named <parameter>key</parameter>.
+The variable will have a default value of
 <parameter>default</parameter>,
 and <parameter>help</parameter>
 will form the descriptive part of the help text.
@@ -5181,12 +5180,10 @@ as false.</para>
   <term><function>EnumVariable</function>(<parameter>key, help, default, allowed_values, [map, ignorecase]</parameter>)</term>
   <listitem>
 <para>
-Set up a variable
+Set up a variable named <parameter>key</parameter>
 whose value may only be from
 a specified list ("enumeration") of values.
-The variable will have the name
-<parameter>key</parameter>,
-have a default value of
+The variable will have a default value of
 <parameter>default</parameter>
 and <parameter>help</parameter>
 will form the descriptive part of the help text.
@@ -5226,12 +5223,10 @@ converted to lower case.</para>
   <term><function>ListVariable</function>(<parameter>key, help, default, names, [map, validator]</parameter>)</term>
   <listitem>
 <para>
-Set up a variable
+Set up a variable named <parameter>key</parameter>
 whose value may be one or more
 from a specified list of values.
-The variable will have the name
-<parameter>key</parameter>,
-have a default value of
+The variable will have a default value of
 <parameter>default</parameter>,
 and <parameter>help</parameter>
 will form the descriptive part of the help text.
@@ -5240,11 +5235,10 @@ Any value that is not in
 <userinput>all</userinput> or
 <userinput>none</userinput>
 will raise an error.
-More than one value may be specified,
-separated by commas.
+Use a comma separator to specify multiple values.
 <parameter>default</parameter> may be specified
-either as a string of comma-separated value,
-or as a list of values.
+either as a string of comma-separated values,
+or as a &Python; list of values.
 </para>
 <para>
 The optional
@@ -5273,33 +5267,31 @@ The default is to use an internal validator routine.
   <term><function>PackageVariable</function>(<parameter>key, help, default</parameter>)</term>
   <listitem>
 <para>
-Set up a variable for a <emphasis>package</emphasis>,
-where if the variable is specified,
-the &consvar; named by <parameter>key</parameter>
-will end with a value of <literal>True</literal>,
-<literal>False</literal>, or a user-specified value.
-For example,
-a package could be a third-party software component,
-the build could use the information to
-exclude the package, include the package in the standard way,
-or include the package using a specified
-directory path to find the package files.
+Set up a variable named <parameter>key</parameter>
+to help control a build component,
+such as a software package.
+The variable can be specified to disable, enable,
+or enable with a custom path.
+The resulting &consvar; will have a value of
+<literal>True</literal>, <literal>False</literal>, or a path string.
+Interpretation of this value is up to the consumer,
+but a path string must refer to an existing filesystem entry
+or the <function>PackageVariable</function> validator
+will raise an exception.
 </para>
+
 <para>
-The variable will have a default value
-<parameter>default</parameter>,
-and <parameter>help</parameter>
-will form the descriptive part of the help text.
-The variable supports (case-insensitive) truthy values
+Any of the (case-insensitive) strings
 <userinput>1</userinput>,
 <userinput>yes</userinput>,
 <userinput>true</userinput>,
 <userinput>on</userinput>,
 <userinput>enable</userinput>
 and
 <userinput>search</userinput>
+can be used
 to indicate the package is "enabled",
-and the (case-insensitive) falsy values
+and the (case-insensitive) strings
 <userinput>0</userinput>,
 <userinput>no</userinput>,
 <userinput>false</userinput>,
@@ -5308,14 +5300,17 @@ and
 <userinput>disable</userinput>
 to indicate the package is "disabled".
 </para>
+
 <para>
-The value
-of the variable may also be set to an
-arbitrary string,
-which is taken to be the path name to the package
-that is being enabled.
-The validator will raise an exception
-if this path does not exist in the filesystem.
+The <parameter>default</parameter> parameter
+can be either a path string or one of the enabling or disabling strings.
+<parameter>default</parameter> is produced if the variable is not specified,
+or if it is specified with one of the enabling strings,
+except that if <parameter>default</parameter> is one
+of the enabling strings, the boolean literal <literal>True</literal>
+is produced instead of the string.
+The <parameter>help</parameter> parameter
+specifies the descriptive part of the help text.
 </para>
   </listitem>
   </varlistentry>
@@ -5324,22 +5319,18 @@ if this path does not exist in the filesystem.
   <term><function>PathVariable</function>(<parameter>key, help, default, [validator]</parameter>)</term>
   <listitem>
 <para>
-Set up a variable
-whose value is expected to be a path name.
-The &consvar; named by <parameter>key</parameter>
-will have have a default value of
+Set up a variable named <parameter>key</parameter> to hold a path string.
+The variable will have have a default value of
 <parameter>default</parameter>,
-and <parameter>help</parameter>
-will form the descriptive part of the help text.
+and the <parameter>help</parameter> parameter
+will be used as the descriptive part of the help text.
 </para>
 
 <para>
-An optional
-<parameter>validator</parameter> argument
-may be specified.
-The validator will be called to
-verify that the specified path
-is acceptable.
+The optional
+<parameter>validator</parameter> parameter
+describes a callback function which will be called to
+verify that the specified path is acceptable.
 SCons supplies the
 following ready-made validators:</para>
 
@@ -5406,8 +5397,10 @@ if the specified value is not acceptable.</para>
 <para>These functions make it
 convenient to create a number
 of variables with consistent behavior
-in a single call to the &AddVariables;
-method:</para>
+in a single call to the
+<link linkend='v-AddVariables'><function>AddVariables</function></link>
+method:
+</para>
 
 <programlisting language="python">
 vars.AddVariables(

test/Variables/PackageVariable.py

@@ -27,15 +27,18 @@
 Test the PackageVariable canned Variable type.
 """
 
+import os
+from typing import List
 
 import TestSCons
 
 test = TestSCons.TestSCons()
 
 SConstruct_path = test.workpath('SConstruct')
 
-def check(expect):
+def check(expect: List[str]) -> None:
     result = test.stdout().split('\n')
+    # skip first line and any lines beyond the length of expect
     assert result[1:len(expect)+1] == expect, (result[1:len(expect)+1], expect)
 
 test.write(SConstruct_path, """\
@@ -44,11 +47,9 @@ def check(expect):
 
 opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
-    PackageVariable('x11',
-                  'use X11 installed here (yes = search some places',
-                  'yes'),
+    PackageVariable('x11', 'use X11 installed here (yes = search some places', 'yes'),
     PV('package', 'help for package', 'yes'),
-    )
+)
 
 _ = DefaultEnvironment(tools=[])
 env = Environment(variables=opts, tools=[])
@@ -77,10 +78,37 @@ def check(expect):
 
 expect_stderr = """
 scons: *** Path does not exist for variable 'x11': '/non/existing/path/'
-""" + test.python_file_line(SConstruct_path, 13)
+""" + test.python_file_line(SConstruct_path, 11)
 
 test.run(arguments='x11=/non/existing/path/', stderr=expect_stderr, status=2)
 
+# test that an enabling value produces the default value
+# as long as that's a path string
+tinycbor_path = test.workpath('path', 'to', 'tinycbor')
+test.subdir(tinycbor_path)
+SConstruct_pathstr = test.workpath('SConstruct.path')
+test.write(SConstruct_pathstr, f"""\
+from SCons.Variables import PackageVariable
+
+vars = Variables(args=ARGUMENTS)
+vars.Add(
+    PackageVariable(
+        'tinycbor',
+        help="use 'tinycbor' at <path>",
+        default=r'{tinycbor_path}'
+    )
+)
+
+_ = DefaultEnvironment(tools=[])
+env = Environment(variables=vars, tools=[])
+
+print(env['tinycbor'])
+Default(env.Alias('dummy', None))
+""")
+
+test.run(arguments=['-f', 'SConstruct.path', 'tinycbor=yes'])
+check([tinycbor_path])
+
 test.pass_test()
 
 # Local Variables: