numba / llvmlite

A lightweight LLVM python binding for writing JIT compilers
https://llvmlite.pydata.org/
BSD 2-Clause "Simplified" License
1.94k stars 322 forks source link

Test failure in 0.40.0 on 64-bit PowerPC: test_get_process_triple #941

Closed musicinmybrain closed 1 year ago

musicinmybrain commented 1 year ago

Reporting a bug

I just tested updating the python-llvmlite package in Fedora Linux to 0.40.0 (in Rawhide, the development version of Fedora).

I got exactly one test failure—only on ppc64le—as follows:

======================================================================
FAIL: test_get_process_triple (llvmlite.tests.test_binding.TestMisc.test_get_process_triple)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/llvmlite-0.40.0/llvmlite/tests/test_binding.py", line 587, in test_get_process_triple
    self.assertEqual(default_parts[0], triple_parts[0])
AssertionError: 'ppc64le' != 'powerpc64le'
- ppc64le
+ powerpc64le
?  ++++

What do you think? Thanks!

gmarkall commented 1 year ago

Is there anything that might have recently changed the triple in Fedora or its build infrastructure lately?

esc commented 1 year ago

This was discussed in the Numba meeting yesterday, here is a link to the notes:

https://github.com/numba/numba/wiki/Minutes_2023_05_09

Unfortunately, the details have not been recorded, but IIRC this was a somewhat confusing issue, since in many places, the two strings are used synonymously. I think @sklam can recall the specifics?

The bug question for this one is, does this warrant an llvmlite point release? @musicinmybrain is this something you could work around during package building?

musicinmybrain commented 1 year ago

Unfortunately, the details have not been recorded, but IIRC this was a somewhat confusing issue, since in many places, the two strings are used synonymously.

https://github.com/pypa/manylinux/issues/687 does not exactly apply here, but it gives a starting point for digging into the history of these names and their handling.

The bug question for this one is, does this warrant an llvmlite point release? @musicinmybrain is this something you could work around during package building?

Was there a conclusion regarding what should be done about it? If the discrepancy is deemed harmless, I can easily skip the single affected test on ppc64le.

sklam commented 1 year ago

From a quick look into LLVM source code (e.g. llvm/lib/TargetParser/Triple.cpp#L475), this seems harmless. LLVM seems to use both ppc64le and powerpc64le internally.

The test is comparing the result of getDefaultTargetTriple (llvm/lib/TargetParser/Unix/Host.inc#L75-L86) against the result of getProcessTriple(llvm/lib/TargetParser/Host.cpp#L1880-L1890). Both rely on CMake build time config:

getProcessTriple also calls Triple::normalize before returning the result. Maybe that is what causing the difference.

The test failure is harmless. I don't think we need a bugfix release. But, we should change the test test_get_process_triple to account for the difference between getDefaultTargetTriple and getProcessTriple.

apmasell commented 1 year ago

I don't know how to test this, but I added normalization to the default triple: https://github.com/numba/llvmlite/pull/948

musicinmybrain commented 1 year ago

I don't know how to test this, but I added normalization to the default triple: #948

When that PR is applied as a patch to the Fedora Linux package, the tests still fail in the same way on ppc64le:

======================================================================
FAIL: test_get_process_triple (llvmlite.tests.test_binding.TestMisc.test_get_process_triple)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/llvmlite-0.40.0/llvmlite/tests/test_binding.py", line 587, in test_get_process_triple
    self.assertEqual(default_parts[0], triple_parts[0])
AssertionError: 'ppc64le' != 'powerpc64le'
- ppc64le
+ powerpc64le
?  ++++
----------------------------------------------------------------------
apmasell commented 1 year ago

Ah. That's unfortunate. I was hoping it would be a uniform way to handle this. I'll modify the test to handle this case specially.

musicinmybrain commented 1 year ago

Ah. That's unfortunate. I was hoping it would be a uniform way to handle this. I'll modify the test to handle this case specially.

It did sound like a reasonable idea! Thanks for working on this.

apmasell commented 1 year ago

Okay, let's try brute force.

esc commented 1 year ago

@musicinmybrain :: will https://github.com/numba/llvmlite/issues/941 suffice to fix the issue you are seeing?

musicinmybrain commented 1 year ago

@musicinmybrain :: will #941 suffice to fix the issue you are seeing?

I think you meant https://github.com/numba/llvmlite/pull/949. I commented there; it is not quite correct yet.