Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build and errno check on musl libc #1678

Merged
merged 3 commits into from
Oct 30, 2022

Conversation

listout
Copy link
Contributor

@listout listout commented Aug 23, 2022

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] (#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 listout@protonmail.com

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] (AcademySoftwareFoundation#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 <listout@protonmail.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 23, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@remia
Copy link
Collaborator

remia commented Sep 5, 2022

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.

@listout
Copy link
Contributor Author

listout commented Sep 5, 2022

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

Copy link
Collaborator

@remia remia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Docker alpine/latest and the patch seem to fix both build and unit test so I am going to approve, thanks.

@remia remia linked an issue Oct 25, 2022 that may be closed by this pull request
@cedrik-fuoco-adsk
Copy link
Contributor

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

@michdolan michdolan merged commit c897bd3 into AcademySoftwareFoundation:main Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

errno checks break musl compatibility
4 participants