Fix ListVariable with a space-containing value #4589
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix ListVariable handling of a quoted variable value containing spaces.
As a side effect of splitting the former monolithic converter/validator
for ListVariable into separate callbacks, it became possible for
subst to be called twice. The ListVariable converter produces an
instance of a _ListVariable container, and running subst on that
result ends up losing information, so avoid doing so.
While developing a test for this, it turned out the test framework
also didn't handle a quoted argument containing a space, so that
a test case passing arguments to scons via "run(arguments='...')"
could end up with scons seeing a different (broken) command line
than scons invoked with the same arguments typing to a shell prompt.
A regex is now used to more accurately split the "arguments" parameter,
and a unit test was added to the framework tests to validate.
The framework fix had a side effect - it was possible that when run
as part of the test suite, the Variables package could receive a value
still wrapped in quotes, leading to string mismatches ('"with space"'
is not equal to 'with space'), so ListVariable now strips wrapping
quote marks.
Also during testing it turned out that the earlier fix for SCons#4241,
allowing a Variable to declare the value should not be subst'd,
introduced problems for two types which assumed they would always
be passed a string. With subst=False, they could be passed a default
value that had been specified as a bool. Fixed to not fail on that.
Fixes SCons#4585
Signed-off-by: Mats Wichmann <mats@linux.com>
mwichmann
added
Variables
mwichmann
commented
Aug 16, 2024
| @@ -287,7 +290,13 @@ def Update(self, env, args: Optional[dict] = None) -> None: | |||
| for option in self.options: | |||
| if option.validator and option.key in values: | |||
| if option.do_subst: | |||
| value = env.subst('${%s}' % option.key) | |||
| val = env[option.key] | |||
| if not SCons.Util.is_String(val): | |||
There was a problem hiding this comment.
This could be more specific and check for an instance of _ListVariable, but that's supposed to be private to the ListVariable module and the main Variables package isn't supposed to have to know details...
mwichmann
commented
Aug 16, 2024
| @@ -70,7 +77,7 @@ def check(expected): | |||
| else: | |||
| print('0') | |||
|
|
|||
| print(" ".join(env['shared'])) | |||
| print(",".join(env['shared'])) | |||
There was a problem hiding this comment.
Here we print things so that the test "expect" can look for matches - if a space separator is used, we can't distinguish a space-containing value, so had to rework to use a comma.
The previous commit introduced a change to how the framework handled arguments, which necessitated some changes in the variables code. It got too complicated, too many places would need too much logic. Just accept that the test.run(arguments="...") will never be quite like the same arguments on the CLI, and just use lists to avoid things being broken on embedded spaces - those won't be split. Many tests arleady do this, so it's nothing new. Added a comment in TestCmd to make it more clear. Signed-off-by: Mats Wichmann <mats@linux.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A call like:
scons SKIPUTILS="NSIS Menu"forListVariableSKIPUTILS, with a space embedded in the value and thus given quoted, did not work, as the splitting ofListVariableinto separate converter / validator routines leftsubst()running twice. The call before the converter is invoked was fine; but the converter turns the value into an instance of the internal list-variable class, which is then stored, and runningsubst()on that before the validator was invoked undid that work and turned it back into a string - which the validator then split incorrectly because of the lost context. There's no completely clean fix (perhaps other than going back to a combined converter/validator like before 4.8.0). The approach chosen was to not run thesubstbefore the validator if the stored value doesn't look like a string - indicating it's probably already in the form we wanted.(Edited) Testing this was complicated by the test framework it splits the
arguments=parameter, and so breaks with embedded spaces liketest.run(arguments='TEST="a b"'). Attempts to fix this up just created more problems, so the test additions now use a list to supply strings that should not be split, likearguments=['TEST="a', 'b"']. There's plenty of precedent for this, many tests are run this way without necessarily even needing to be. Added some doc notes on this. The various variables are now tested for space-included strings, which can be values in an Enum or List type, or path strings (think Windows paths...) in Path and Package types.Although not directly related to the space problem, during the investigation it also became clear that the new option to avoid calling
subst()before the converter (the earlier fix for #4241) could have side effects - it is now possible for a variable's converter to be called with a type that is not an expected user input type. This could happen withBoolVariableandPathVariable: here the inputs were all expected to be strings, but the default to use if the variable is not specified could be in a "final" type - a bool (forPathVariableit's bool-or-str). Ifsubstis used, those will arrive as stringified versions of the bool, but ifsubstis suppressed they will actually be bool, so those converters needed to check for a bool type before doing astring.lower().Signed-off-by: Mats Wichmann mats@linux.com
Contributor Checklist:
CHANGES.txt(and read theREADME.rst)