From afd59b1f66ffeb53d86a361d2622a1fe50cfbdc9 Mon Sep 17 00:00:00 2001 From: Mats Wichmann Date: Sat, 31 Aug 2024 08:24:44 -0600 Subject: [PATCH] PackageVariable now returns the default on "true" In all doc versions until 4.8.0, PackageVariable had wording like: "The option will support the values yes, true, on, enable or search, in which case the specified default will be used", but the code didn't actually do that, it just returned True. With this change it now returns the default value, with a slight tweak - if the default is one of the spelled out enabling strigs, it returns the boolean True instead. The indication that the default is produced if a truthy string is given is restored to the manpage (it was never dropped from the User Guide). Signed-off-by: Mats Wichmann --- CHANGES.txt | 8 ++- RELEASE.txt | 5 +- SCons/Variables/ListVariable.py | 4 +- SCons/Variables/PackageVariable.py | 50 ++++++++++------- doc/man/scons.xml | 111 +++++++++++++++++-------------------- test/Variables/PackageVariable.py | 40 +++++++++++-- 6 files changed, 127 insertions(+), 91 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 7ee45aa2a..fad40c6f1 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -12,9 +12,11 @@ NOTE: Python 3.6 support is deprecated and will be dropped in a future release. RELEASE VERSION/DATE TO BE FILLED IN LATER - From John Doe: - - - Whatever John Doe did. + 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). RELEASE 4.8.1 - Tue, 03 Sep 2024 17:22:20 -0700 diff --git a/RELEASE.txt b/RELEASE.txt index 439c8630d..196103cfb 100644 --- a/RELEASE.txt +++ b/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 ------------ diff --git a/SCons/Variables/ListVariable.py b/SCons/Variables/ListVariable.py index 7a0ce49c9..2c79ee7f1 100644 --- a/SCons/Variables/ListVariable.py +++ b/SCons/Variables/ListVariable.py @@ -54,6 +54,7 @@ Usage example:: # 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 diff --git a/SCons/Variables/PackageVariable.py b/SCons/Variables/PackageVariable.py index 9fa514088..7271cfbf8 100644 --- a/SCons/Variables/PackageVariable.py +++ b/SCons/Variables/PackageVariable.py @@ -51,6 +51,7 @@ Can be used as a replacement for autoconf's ``--with-xxx=yyy`` :: """ import os +import functools from typing import Callable, Optional, Tuple, Union import SCons.Errors @@ -60,21 +61,33 @@ __all__ = ['PackageVariable',] 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 diff --git a/doc/man/scons.xml b/doc/man/scons.xml index cea021c99..ca1937260 100644 --- a/doc/man/scons.xml +++ b/doc/man/scons.xml @@ -5135,22 +5135,21 @@ vars.FormatVariableHelpText = my_format -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: +a tuple appropriate for directly passing to the +Add +AddVariables +methods. + BoolVariable(key, help, default) -Set up a Boolean variable. -The variable will use -the specified name -key, -have a default value of +Set up a Boolean variable named key. +The variable will have a default value of default, and help will form the descriptive part of the help text. @@ -5181,12 +5180,10 @@ as false. EnumVariable(key, help, default, allowed_values, [map, ignorecase]) -Set up a variable +Set up a variable named key whose value may only be from a specified list ("enumeration") of values. -The variable will have the name -key, -have a default value of +The variable will have a default value of default and help will form the descriptive part of the help text. @@ -5226,12 +5223,10 @@ converted to lower case. ListVariable(key, help, default, names, [map, validator]) -Set up a variable +Set up a variable named key whose value may be one or more from a specified list of values. -The variable will have the name -key, -have a default value of +The variable will have a default value of default, and help will form the descriptive part of the help text. @@ -5240,11 +5235,10 @@ Any value that is not in all or none will raise an error. -More than one value may be specified, -separated by commas. +Use a comma separator to specify multiple values. default 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. The optional @@ -5273,24 +5267,21 @@ The default is to use an internal validator routine. PackageVariable(key, help, default) -Set up a variable for a package, -where if the variable is specified, -the &consvar; named by key -will end with a value of True, -False, 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 key +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 +True, False, 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 PackageVariable validator +will raise an exception. + -The variable will have a default value -default, -and help -will form the descriptive part of the help text. -The variable supports (case-insensitive) truthy values +Any of the (case-insensitive) strings 1, yes, true, @@ -5298,8 +5289,9 @@ The variable supports (case-insensitive) truthy values enable and search +can be used to indicate the package is "enabled", -and the (case-insensitive) falsy values +and the (case-insensitive) strings 0, no, false, @@ -5308,14 +5300,17 @@ and disable to indicate the package is "disabled". + -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 default parameter +can be either a path string or one of the enabling or disabling strings. +default is produced if the variable is not specified, +or if it is specified with one of the enabling strings, +except that if default is one +of the enabling strings, the boolean literal True +is produced instead of the string. +The help parameter +specifies the descriptive part of the help text. @@ -5324,22 +5319,18 @@ if this path does not exist in the filesystem. PathVariable(key, help, default, [validator]) -Set up a variable -whose value is expected to be a path name. -The &consvar; named by key -will have have a default value of +Set up a variable named key to hold a path string. +The variable will have have a default value of default, -and help -will form the descriptive part of the help text. +and the help parameter +will be used as the descriptive part of the help text. -An optional -validator argument -may be specified. -The validator will be called to -verify that the specified path -is acceptable. +The optional +validator parameter +describes a callback function which will be called to +verify that the specified path is acceptable. SCons supplies the following ready-made validators: @@ -5406,8 +5397,10 @@ if the specified value is not acceptable. These functions make it convenient to create a number of variables with consistent behavior -in a single call to the &AddVariables; -method: +in a single call to the +AddVariables +method: + vars.AddVariables( diff --git a/test/Variables/PackageVariable.py b/test/Variables/PackageVariable.py index e87164cab..1e32eeecf 100644 --- a/test/Variables/PackageVariable.py +++ b/test/Variables/PackageVariable.py @@ -27,6 +27,8 @@ Test the PackageVariable canned Variable type. """ +import os +from typing import List import TestSCons @@ -34,8 +36,9 @@ 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 @@ from SCons.Variables import PackageVariable 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 @@ check([space_subdir]) 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 ", + default='{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: -- 2.11.4.GIT