-
Notifications
You must be signed in to change notification settings - Fork 47
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 compilation with musl libc (fixes #164) + make CI cover compilation with musl libc + bump version to 1.6.2 #165
Fix compilation with musl libc (fixes #164) + make CI cover compilation with musl libc + bump version to 1.6.2 #165
Conversation
Co-authored-by: Edgar Bonet <linux@edgar-bonet.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know you could do a Musl build without a dedicated VM or container. This is cool!
One question bout the change to ttyplot.c: is _XOPEN_SOURCE
needed here for the Musl build, even though M_PI
is not used? Maybe it's required by ncurses? If it's not required, I would rather leave that file alone.
@edgar-bonet it saves us rolling Alpine and Docker and also downloading a full toolchain from https://musl.cc/#binaries . I ran into it only yesterday, glad you like the idea too. On a side note, Alpine may not even have helped to see the error in practice, because Alpine's ttyplot 1.6.1 packaging does not apply any patches to ttyplot, so some other part of Alpine makes this work apparently (not sure which)…
I'm with you there, I would not have touched the file unless needed. With the patch in Compile errors log here, please click to expand
The addition is only in the What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the patch in
ttyplot.c
reverted, we would get this compile error
Oh, now I remember from PR #99. Looks like a bad ncursesw.pc file. I'm fine with the fix.
@edgar-bonet cool! |
LGTM, please let me know if you want me to merge it |
7bd8271
to
f187abf
Compare
I did two tiny fixes more now — bumping the date to today and fixing spelling "Musl" to lowercase "musl" in two places, see https://github.com/tenox7/ttyplot/compare/7bd8271130fbd40f1955e1a9b2925dbf1406386b..f187abf3001853c5739cc3c8278dca998856fe4d — then merged just now. If you could do the Git tag and release for 1.6.2 that would rock. Thanks! |
@tenox7 please do not forget the 1.6.2 Git tag at least so that I can fix https://bugs.gentoo.org/922285 without need to copy patches downstream. Thank you! |
@tenox7 please please 🙏 |
Sincerest apologies for the delay. Tag 1.6.2 is pushed. |
For the release could you post high level change log between 1.6 and 1.6.2? Just major stuff. Thanks. |
@tenox7 thanks, bumped in Gentoo — gentoo/gentoo@b21ac13 . @q66 if you wanted to package ttyplot for Chimera, 1.6.2 now has all the musl fixes you would need. Just an idea.
Arguably that would only be:
I'm not a big fan of grouping multiple releases into a common "1.6" roof. Why blur clear lines, what's in this reduced transparency for our users? For all pull requests per milestone if curious: |
Fixes #164