[GH-ISSUE #407] Potential regression 1.1.4 -> 1.1.5: Test fails on big-endian architecture (Debian s390x) #320

Closed
opened 2026-05-05 12:08:57 -06:00 by gitea-mirror · 3 comments
Owner

Originally created by @hosiet on GitHub (Aug 3, 2023).
Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/407

The build of libxlsxwriter previously succeeded on Debian s390x architecture, but it started to fail with version 1.1.5:

For an overview of build history, please visit https://buildd.debian.org/status/logs.php?pkg=libxlsxwriter&arch=s390x .

Example of failed build log: https://buildd.debian.org/status/fetch.php?pkg=libxlsxwriter&arch=s390x&ver=1.1.5-1%7Eexp2&stamp=1691020110&raw=0

For other architetures with failed build, please visit https://buildd.debian.org/status/package.php?p=libxlsxwriter&suite=experimental . Obviously 1.1.5 fails on all big-endian architectures (powerpc, hp-ux, sparc).

Since this looks like a regression, bisecting the code base may help.

Originally created by @hosiet on GitHub (Aug 3, 2023). Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/407 The build of libxlsxwriter previously succeeded on Debian s390x architecture, but it started to fail with version 1.1.5: For an overview of build history, please visit https://buildd.debian.org/status/logs.php?pkg=libxlsxwriter&arch=s390x . Example of failed build log: https://buildd.debian.org/status/fetch.php?pkg=libxlsxwriter&arch=s390x&ver=1.1.5-1%7Eexp2&stamp=1691020110&raw=0 For other architetures with failed build, please visit https://buildd.debian.org/status/package.php?p=libxlsxwriter&suite=experimental . Obviously 1.1.5 fails on all big-endian architectures (powerpc, hp-ux, sparc). Since this looks like a regression, bisecting the code base may help.
Author
Owner

@jmcnamara commented on GitHub (Aug 3, 2023):

For a big endian system you will need to pass the USE_BIG_ENDIAN flag:

http://libxlsxwriter.github.io/getting_started.html#gsg_endian

I didn’t see that in the Cmake flags in the failed build (but I may have missed it because I’m traveling and reading this on my phone).

Could you try add that if it isn’t being used.

That doesn’t explain why it passed earlier but try that as a starting point.

Obviously 1.1.5 fails on all big-endian architectures (powerpc, hp-ux, sparc).

Since this looks like a regression, bisecting the code base may help.

Do l need a big endian system to reproduce this? As far as I remember you can’t use a big-endian VM guest on a little endian host. Is that correct or can you cross compile and run the tests on a little-endian system?

When I developed the USE_BIG_ENDIAN feature I used a ~20 year old Power PC mac Mini that I brought down from the attic. :-)

<!-- gh-comment-id:1664295828 --> @jmcnamara commented on GitHub (Aug 3, 2023): For a big endian system you will need to pass the `USE_BIG_ENDIAN` flag: http://libxlsxwriter.github.io/getting_started.html#gsg_endian I didn’t see that in the Cmake flags in the failed build (but I may have missed it because I’m traveling and reading this on my phone). Could you try add that if it isn’t being used. That doesn’t explain why it passed earlier but try that as a starting point. > Obviously 1.1.5 fails on all big-endian architectures (powerpc, hp-ux, sparc). > > Since this looks like a regression, bisecting the code base may help. Do l need a big endian system to reproduce this? As far as I remember you can’t use a big-endian VM guest on a little endian host. Is that correct or can you cross compile and run the tests on a little-endian system? When I developed the `USE_BIG_ENDIAN` feature I used a ~20 year old Power PC mac Mini that I brought down from the attic. :-)
Author
Owner

@hosiet commented on GitHub (Aug 3, 2023):

Thanks. However, USE_BIG_ENDIAN is only present in hand-written Makefile, and was never placed in the logic of CMake:

-> % git rev-parse --short HEAD
4aaca54c
-> % cat CMakeLists.txt | grep ENDIAN | wc -l
0

My thought is that the control of using big-endian or not should not be configurable, but rather depends on the target build platform. With my personal preference, I would like to suggest the following patch to completely get rid of manual defining a macro (I release this code snippet under the same license as libxlsxwriter, BSD-2-Clause):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fb5a2f5..8996c31 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -243,6 +243,13 @@ if(BUILD_TESTS)
     list(APPEND LXW_PRIVATE_COMPILE_DEFINITIONS TESTING)
 endif()
 
+# Define "LXW_BIG_ENDIAN" macro on big-endian architectures
+include(TestBigEndian)
+TEST_BIG_ENDIAN(LXW_TARGET_BIG_ENDIAN)
+if(LXW_TARGET_BIG_ENDIAN)
+    list(APPEND LXW_PRIVATE_COMPILE_DEFINITIONS LXW_BIG_ENDIAN)
+endif()
+
 file(GLOB LXW_SOURCES src/*.c)
 file(GLOB_RECURSE LXW_HEADERS RELATIVE include *.h)

Let me know whether that looks good to you. I am applying this patch for a Debian's trial build 1.1.5-1~exp3, and you will be able to see the build results within the next 24 hours at https://buildd.debian.org/status/package.php?p=libxlsxwriter&suite=experimental . If the patch is working, we should see that all big-endian builds pass again.

<!-- gh-comment-id:1664602543 --> @hosiet commented on GitHub (Aug 3, 2023): Thanks. However, `USE_BIG_ENDIAN` is only present in hand-written `Makefile`, and was never placed in the logic of CMake: ``` -> % git rev-parse --short HEAD 4aaca54c -> % cat CMakeLists.txt | grep ENDIAN | wc -l 0 ``` My thought is that the control of using big-endian or not should not be configurable, but rather depends on the target build platform. With my personal preference, I would like to suggest the following patch to completely get rid of manual defining a macro (I release this code snippet under the same license as libxlsxwriter, BSD-2-Clause): ```diff diff --git a/CMakeLists.txt b/CMakeLists.txt index fb5a2f5..8996c31 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -243,6 +243,13 @@ if(BUILD_TESTS) list(APPEND LXW_PRIVATE_COMPILE_DEFINITIONS TESTING) endif() +# Define "LXW_BIG_ENDIAN" macro on big-endian architectures +include(TestBigEndian) +TEST_BIG_ENDIAN(LXW_TARGET_BIG_ENDIAN) +if(LXW_TARGET_BIG_ENDIAN) + list(APPEND LXW_PRIVATE_COMPILE_DEFINITIONS LXW_BIG_ENDIAN) +endif() + file(GLOB LXW_SOURCES src/*.c) file(GLOB_RECURSE LXW_HEADERS RELATIVE include *.h) ``` Let me know whether that looks good to you. I am applying this patch for a Debian's trial build `1.1.5-1~exp3`, and you will be able to see the build results within the next 24 hours at https://buildd.debian.org/status/package.php?p=libxlsxwriter&suite=experimental . If the patch is working, we should see that all big-endian builds pass again.
Author
Owner

@jmcnamara commented on GitHub (Aug 12, 2023):

Thanks for the patch. That is a better solution.

I have applied it to main.

<!-- gh-comment-id:1676133253 --> @jmcnamara commented on GitHub (Aug 12, 2023): Thanks for the patch. That is a better solution. I have applied it to main.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/libxlsxwriter#320
No description provided.