Custom ListVariable cause error with invalid input

[Protofall]

The basic issue is that for invalid input, the converter is run before my validation function and because of that it throws an error. Here is the example code:

def get_builds():
    return ['a', 'b', 'c', 'all']

def valid_build(key, val, env):
    print('--ENTERED THE VALIDATOR--')

    # Catches BUILDS=none
    if val == "":
        print("Please give a value for BUILDS. Type \"scons --help\" for more information")
        Exit(1)

    # Split val so we can check all arguments
    for v in val.split():
        if not v in get_builds():
            print("Please give a value for BUILDS. Type \"scons --help\" for more information")
            Exit(1)

vars = Variables('scons_args.py', ARGUMENTS)

(key, help, default, _, converter) = ListVariable('BUILDS',
    help = "The builds we want to create",
    default = 'none',
    names = get_builds()
)
vars.Add((key, help, default, valid_build, converter))

processing_env = Environment(tools = [], variables = vars)    # Crashes here

Here are some test inputs and their outputs

scons BUILDS=a   # Works fine, same for any combination of the get_builds() list
scons BUILDS=none   # Gets caught by the validator
scons   # Unless scons_args.py sets BUILDS, then this is the same as above

scons BUILDS=anything_else
scons: Reading SConscript files ...

scons: *** Error converting option: BUILDS
Invalid value(s) for option: anything_else
File "SConstruct", line 27, in <module>

I would expect it to run the validator first so I can catch this input and handle it myself.

[mwichmann]

To record here (this was mentioned on Discord), that's what scons does: SCons/Variables/__init__.py in the Update method - this is just the flow of comments, no code:

        # first set the defaults:
...
        # next set the value specified in the options file
...
        # set the values specified on the command line
...
        # put the variables in the environment:
        # (don't copy over variables that are not declared as options)
...
        # Call the convert functions:
...
        # Finally validate the values:

So just in terms of implementation, we'd have to work out some way to either change the order, or have the converter fail in a different way since bailing out with an exception means we don't go on to call the validator.

[Protofall]

~~I've also realised a related problem that might influence how this is fixed. Because I'm manually validating and exiting early when invalid input is detected (None, anything_else) I have to do something like scons BUILDS=a --help in order to get the full help output. This is because scons --help has the BUILDS list set to none and hence triggers one of my outputs and exits on lines 8-10.~~

~~However I might be able to patch my own code if there's a way to force the help message to appear when the --help flag is enabled.~~

EDIT:

Yes, its possible. Please ignore this comment.

[mwichmann]

Yes, this is a normal trick that's needed: there are often cases of "don't do this if user asked for help" (or "... asked for clean"). Described here, which you probably already found:

https://scons.org/doc/4.1.0/HTML/scons-user.html#idp140637539207872

[mwichmann]

also noticed from your code snip here - perhaps this is already done in your more real code - that you usually need to call GenerateHelpText to get the help text from your variables into the actual SCons help message. Be aware that --help and -h are different: --help always prints the original built-in help message, -h prints the one you have set up (or the original if you've not set one up).

[Protofall]

also noticed from your code snip here - perhaps this is already done in your more real code - that you usually need to call GenerateHelpText to get the help text from your variables into the actual SCons help message. Be aware that --help and -h are different: --help always prints the original built-in help message, -h prints the one you have set up (or the original if you've not set one up).

Yeah, my real code is a bit more complex. Its in multiple files, but this file is the one that handles the input (input_logic() specifically). It feels a little hack-ish, but scons -h works fine again. For me both "help"s do the same thing. Must be because I'm manually detecting help.

[mwichmann]

So years later... been staring at Variables, and I'm now of the opinion that there's a mis-implementation. The ListVariable converter effectively serve two purposes and there's no separate validator (it's a TODO), but the converter really shouldn't fail unless it hits some real error, it shouldn't raise because of validation stuff, it should convert anything convertible, leave the rest, and let the validator deal with the rejections. However, they made this one convoluted - the converter instantiates a (private) class, so the flow is kind of hinky.

We could also argue whether converter or validator actually should be called first by the Variables class. I think I see justification for convert first, but it would have been nice if the author left an explanation in the code. Sigh.

Thirdly, one might consider that there should be a way of specifying a custom converter and/or validator for at least some of these pre-defined variable types. The snip from the original posting is a workable way to do that, but should that be necessary?

(key, help, default, _, converter) = ListVariable(...args...)
vars.Add(key, help, default, my_validator, converter)

why not allow

vars.Add(ListVariable(...args..., validator=my_validator))

[mwichmann]

Updated: ignore this, I had missed that the stored value is passed through env.subst() before the validator is called, so the validator gets a string.

~~Implementation question: splitting the validation part into a separate validator callback is easy enough, but comes with a caveat: the core Variables code will call such a function with the arguments key, value, env, but at this point value is useless, as the converter callback produces an instance of the private (and undocumented) _ListVariable class. So the validator needs to do something conceptually like this to fetch the stuff it needs to work with for validation:~~

value = env[key]
values = value.data
allowed_values = value.allowedElems

~~That works okay, but makes it tricky for a user to provide a custom validator function, as they need to have knowledge of this private class. Of course, just staring at the code in ListVariable.py will provide that information, but it still looks a little hokey.~~

~~Other Variable types get around the awkwardness of wanting non-standard calling by instead using a lambda, for example from PackageVariable:~~

    return (key, help, default,                                                
            lambda k, v, e: _validator(k, v, e, searchfunc),                   
            _converter) 

~~But that makes it even more awkward to replace the default validator in your SConscript file. Only PathVariable provides a way in the documented API to supply your own validator. The trick in the example at the top works "in general" (unpack the tuple, then replace the validator element with your own when calling Add) but is harder if we end up doing "funny stuff" in the provided default validator.~~

~~So: does it matter that a custom validator becomes harder? Should there maybe be a validator kwarg to ListVariable?~~

Alternatively: the ListVariable implementation always allows the special argument none, meaning none from the list were selected. The example in this issue seems to want to error out if no value is supplied for the list variable, by making the default none and then erroring out if the value is empty - which it will be after the converter sees none and turns it into an empty list. Perhaps a flag argument could be added to the ListVariable call that says to error out if the converted value ends up empty to cover that specific case (although in general, Variables don't provide a way to set up a Required Build Variable).