OpenColorIO icon indicating copy to clipboard operation
OpenColorIO copied to clipboard

Fix build and errno check on musl libc

Open listout opened this issue 3 years ago • 3 comments

In the 'from_chars' function, it's first checked if errno != 0 and immediately returns with std::errc::result_out_of_range aka ERANGE. Please refer issue [1624] (https://github.com/AcademySoftwareFoundation/OpenColorIO/issues/1624)

The function 'strtol_l' is not available on non GLIBC systems hence on other libc use the alternative 'strtol' function.

Signed-off-by: listout [email protected]

listout avatar Aug 23 '22 07:08 listout

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: listout (0eac66e57f80701facfba1c6358639843568448e)

Thanks for the contribution @listout. Out of curiosity, and sorry for being dumb here, why is returning the wrong error code problematic? Using strtol() instead of strtol_l() looks ok, especially if it fixes a compilation error on musl, though I am also curious what is the benefit of the locale aware variant for integers in this context of LUT file parsing in the first place.

remia avatar Sep 05 '22 20:09 remia

why is returning the wrong error code problematic?

I'm not too sure either, in the linked issue, you see that someone mentioned that a test case was failing and some other detail

According to strtod(3p), if no conversion could be performed, errno may be set to EINVAL. Unlike glibc which doesn't do this, musl does. This leads to an incorrect return value (ERANGE != EINVAL).

I discovered this through failing tests on musl.

As for

Using strtol() instead of strtol_l() looks ok, especially if it fixes a compilation error on musl, though I am also curious what is the benefit of the locale aware variant for integers in this context of LUT file parsing in the first place.

It could be because of musl having only one locale

Sorry @remia for not having any concrete answers, I too don't know much about the issue itself. Picked it up from another issue and compiling opencolorio on musl

listout avatar Sep 05 '22 20:09 listout

Looks good to me too. I am going to approve it.

cedrik-fuoco-adsk avatar Oct 28 '22 12:10 cedrik-fuoco-adsk