[Mesa-dev] [PATCH mesa 6/6] scons: use python3-compatible string-check

[Jose Fonseca]

On 26/09/17 12:20, Ilia Mirkin wrote:
> On Tue, Sep 26, 2017 at 7:07 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
>> On 25/09/17 14:30, Eric Engestrom wrote:
>>>
>>> I pushed the rest of the series.
>>> See below for discussion on this patch.
>>>
>>>
>>> On Wednesday, 2017-09-20 17:05:21 +0000, Jose Fonseca wrote:
>>>>
>>>> On 19/09/17 15:14, Eric Engestrom wrote:
>>>>>
>>>>> Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>
>>>>> ---
>>>>>     scons/custom.py | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/scons/custom.py b/scons/custom.py
>>>>> index 0767ba936d410167116d..978ee5f9ec7c23a74cb9 100644
>>>>> --- a/scons/custom.py
>>>>> +++ b/scons/custom.py
>>>>> @@ -257,7 +257,7 @@ def parse_source_list(env, filename, names=None):
>>>>>         sym_table = parser.parse(src.abspath)
>>>>>         if names:
>>>>> -        if isinstance(names, basestring):
>>>>> +        if isinstance(names, str):
>>>>>                 names = [names]
>>>>>             symbols = names
>>>>>
>>>>
>>>> I'm not sure if this won't give the wrong results for unicode strings,
>>>> but
>>>> at any rate, I don't think that should ever happen in practice.
>>>
>>>
>>> Are you replying to Ilia [1] here?
>>>
>>> He left a comment on this patch, saying:
>>>>
>>>> This might be python3-compatible, but it's not the same thing. str !=
>>>> unicode. Not sure where "names" can come from, but if it can come in
>>>> as a unicode string, this won't work.
>>>
>>>
>>> My knowledge of python is quite basic, so I'd rather you discuss between
>>> you two rather than me trying to forward a conversation with each of you
>>> :P
>>>
>>> [1]
>>> https://lists.freedesktop.org/archives/mesa-dev/2017-September/170130.html
>>>
>>
>> I just checked all uses of ParseSourceList and it's never used with unicode
>> strings.  It's always used with plain strings or lists of plain strings.
>>
>> So on 2nd thought, I think this patch should be safe.  Ilia, would you
>> agree?
> 
> So really this whole thing is about whether names is a list or not.
> You can't check if it's iterable since strings are also iterable. What
> lists are passed in here? If it's all list-type lists (and not
> tuples/etc), we can change this to be
> 
> if not isinstance(names, list)

In this case, the odds of a tuple to creep in are not smaller than an 
unicode to creep in.

> 
> Or we want to be cheeky,
> 
> if type(names) == type(names[0])

That's an interesting suggestion.  There's the issue on empty lists/strings.

> 
> For a list of strings, that won't be the case, but for a string, it
> should be, since a single char has the same type as a string. At least
> in python2.
> 
> I don't use scons, or know how all this stuff is used, so you guys can
> do whatever. In my experience, unicode strings spring up from the most
> unexpected places.
> 
>    -ilia

I think in the end I `isinstance(names, str)` really should be sufficient.

Jose

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>