[Cython] remaining open issues for 0.17

[Robert Bradshaw]

On Fri, Aug 10, 2012 at 9:25 AM, Robert Bradshaw <robertwb at gmail.com> wrote:
> On Thu, Aug 9, 2012 at 10:04 AM, Stefan Behnel <stefan_ml at behnel.de> wrote:
>> mark florisson, 09.08.2012 18:51:
>>> On 9 August 2012 16:36, Stefan Behnel wrote:
>>>> Stefan Behnel, 09.08.2012 14:31:
>>>>> mark florisson, 07.08.2012 11:09:
>>>>>> I thought the 32 bit issue was resolved? You pushed a fix and I fixed
>>>>>> some tests, so it passed for me. I can run it again to check...
>>>>>
>>>>> I don't know. Yaroslav replied to your mail saying that your fixes didn't
>>>>> change anything for the Debian builds. Let's see what he has to say about
>>>>> the second beta.
>>>>
>>>> He ran them through the build servers. Here are the latest results:
>>>>
>>>> https://buildd.debian.org/status/package.php?p=cython&suite=experimental
>>>>
>>>> (hint: click on "all" in the Logs column, then click on the build result to
>>>> see the log output)
>>>>
>>>> The i386 tests show 4 errors related to fused types in the NumPy tests:
>>>>
>>>> https://buildd.debian.org/status/fetch.php?pkg=cython&arch=i386&ver=0.17~beta1-2&stamp=1343428255
>>>>
>>>> >From a quick look, they might really be problems in the tests rather than
>>>> the code we generate.
>>>>
>>>> It also looks like my signed char fix was not sufficient, as you can see in
>>>> the memslice tests here, e.g. in the
>>>> "memslice.__test__.test_memslice_struct_with_arrays" test:
>>>>
>>>> https://buildd.debian.org/status/fetch.php?pkg=cython&arch=s390&ver=0.17~beta1-2&stamp=1343418358
>>>>
>>>> It now prints
>>>>
>>>> """
>>>> ValueError: Buffer dtype mismatch, expected 'char' but got 'unsigned char'
>>>> in 'ArrayStruct.chars'
>>>> """
>>>>
>>>> Well, "char" and "unsigned char" are supposed to be the same on this
>>>> platform, but I guess it can't know that. I wonder if "char" and "signed
>>>> char" are considered identical on other platforms ...
>>>>
>>>> To fix the tests, it might be enough to use an explicit "unsigned char"
>>>> instead of plain "char" for the types. Not sure if that's a good idea or if
>>>> there actually is a bug hidden below this test failure. At least, it smells
>>>> like an inconvenience for users to get that error.
>>>
>>> I think these tests are fine (unlike the fused test I fixed), it's a
>>> bug somewhere. I'm not sure where though, because the code that
>>> creates the format string uses the type info struct (with the C
>>> compile-time unsigned check), and so does the buffer format parser.
>>> The code creating the type strings are in the MemoryView.pyx and
>>> Buffer.c utility files at the bottom. It does
>>>
>>> if (type->is_unsigned)
>>>     *buf = toupper(*buf);
>>>
>>> So whatever I feed it on my system (char, signed char or unsigned
>>> char), it generates the right format string for me.
>>
>> I think this needs some more investigation directly on the failing systems.
>> It'll be much easier to figure this out with a debugger.
>
> Strangely enough, I wasn't even able to reproduce this passing
> "-funsigned-char" so yes, I think it may require access to one of
> these systems. (Perhaps I'd need to compile Python and/or NumPy with
> this option as well...)
>
>>> Perhaps we can just ignore these test failures, I don't think many
>>> people are using memoryviews with characters. Changing them to signed
>>> or unsigned char may fix the problem for the tests, though.
>>
>> If we can't find a way to fix this in time, I'm leaning towards that, too.
>> However, working around a bug in a test is a sure way to forget about the
>> bug and consider everything working. Could you copy the failing tests into
>> a separate test case, disable that in bugs.txt and then change the types in
>> the original test modules to whatever you think would make the tests work?
>
> I'll take a stab at this.

OK, the problem boiled down to using 'b' or 'B' for the format string
for char rather than 'c'. See, e.g.
http://docs.python.org/library/array.html . I've pushed a fix at
https://github.com/robertwb/cython/commit/b0539cbc32c200a09b1fbddf2d6943e92aec2f3e
, but it'd be good to have another set of eyes with someone more
familiar with this code to be sure I didn't miss anything. It also
involved a lot of changes to the tests
https://github.com/robertwb/cython/commit/6bcc8fd8419e6e4079344788023029736610d5aa
so is not exactly backwards compatible, though perhaps more correct.

It'd be good to see if this actually fixes the issues.

- Robert