build: fix some build issues encountered on a musl libc system #4903
Conversation
src/include/OpenImageIO/typedesc.h
Outdated
| template<> struct BaseTypeFromC<int64_t> { static constexpr TypeDesc::BASETYPE value = TypeDesc::INT64; }; | ||
| template<> struct BaseTypeFromC<const int64_t> { static constexpr TypeDesc::BASETYPE value = TypeDesc::INT64; }; | ||
| #if defined(__GNUC__) && __WORDSIZE == 64 && !(defined(__APPLE__) && defined(__MACH__)) || defined(__NetBSD__) | ||
| #if defined(__GNUC__) && IS_64_BIT && !(defined(__APPLE__) && defined(__MACH__)) || defined(__NetBSD__) |
There was a problem hiding this comment.
Since this is the only place it's used, how about just changing to
| #if defined(__GNUC__) && IS_64_BIT && !(defined(__APPLE__) && defined(__MACH__)) || defined(__NetBSD__) | |
| #if defined(__GNUC__) && (ULONG_MAX == 0xffffffffffffffff) && !(defined(__APPLE__) && defined(__MACH__)) || defined(__NetBSD__) |
and then you don't actually need to define IS_64_BIT?
There was a problem hiding this comment.
I'm okay with this change.
I was worried that the word size might be something like 128 bits in the future (hence the explicit check for both 64 bit and 32 bit, then erroring out otherwise), but now that I think about it, the compile will fail anyway in that case.
|
Is this intended as a DevDays project? |
No. By the way, I am not sure how to respond on suggested changes. I just looked through the contributing guidelines and did not see anything about it. Should I just use the GitHub UI to "sign off and commit suggestion"? Rebase? Add another commit manually to incorporate the change? Do I need to add a "Co-Author" tag? Anyway, I have allowed maintainers to edit this PR, so feel free to make any edits you see fit. |
No problem either way, I just was trying to figure out whether or not to tag it as such.
Any of the above. You can click and sign off on the change. Or you can do it yourself and push the additional commit. (Just make sure EVERY commit is done with You do not need a co-author tag for something this trivial. |
Use standard defines to determine if the system is 64 bits. Despite __WORDSIZE not being standard, musl libc also has it defined in another header (either bits/reg.h or bits/user.h), but it is probably a good idea to avoid using it anyway for standard compliance. Signed-off-by: omcaif <[email protected]>
feenableexcept is a GNU extention and is not part of the standard. For example, musl libc does not implement it. Signed-off-by: omcaif <[email protected]>
|
Okay, thanks. I think I updated the commit now to address your comment. |
lgritz
left a comment
lgritz
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fixes
…mySoftwareFoundation#4903) Two small fixes to compile on musl libc systems. The first commit is about the use of `__WORDSIZE`, which is not a standard define and is defined another header in musl libc. I decided to fix this by following what was done in `libdnf` in [this commit](rpm-software-management/libdnf@82a07db). However, there are other ways, such as including `bits/reg.h` or `bits/user.h`, which define `__WORDSIZE` in musl libc, or using `__LP64__` to check for 64 bit instead (inspired by [Alpine Linux's patch](https://gitlab.alpinelinux.org/alpine/aports/-/blob/0f71e9504b59049e0ec1abb486ba455ab2650c83/community/openimageio/0001-fix-compile-error.patch)): ```c #if defined(__GNUC__) && (__WORDSIZE == 64 || (defined(__LP64__) && __LP64__)) && !(defined(__APPLE__) && defined(__MACH__)) || defined(__NetBSD__) ``` The second commit is about the use of the `feenableexcept` function. This function is only defined by glibc. The original code assumed all Linux systems use glibc (checking `__linux__` define), so I just changed it to check for glibc directly (by checking the `__GLIBC__` define). I have only tested it on my musl libc system. I think just compiling it on different systems is enough of a test in this case... --------- Signed-off-by: omcaif <[email protected]>
lgritz
added
the
build / testing / port / CI
2 participants
Description
Two small fixes to compile on musl libc systems.
The first commit is about the use of
__WORDSIZE, which is not a standard define and is defined another header in musl libc. I decided to fix this by following what was done inlibdnfin this commit. However, there are other ways, such as includingbits/reg.horbits/user.h, which define__WORDSIZEin musl libc, or using__LP64__to check for 64 bit instead (inspired by Alpine Linux's patch):The second commit is about the use of the
feenableexceptfunction. This function is only defined by glibc. The original code assumed all Linux systems use glibc (checking__linux__define), so I just changed it to check for glibc directly (by checking the__GLIBC__define).Tests
I have only tested it on my musl libc system. I think just compiling it on different systems is enough of a test in this case...
Checklist:
need to update the documentation, for example if this is a bug fix that
doesn't change the API.)
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.