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 compilation with musl libc (fixes #164) + make CI cover compilation with musl libc + bump version to 1.6.2 #165

Merged
merged 4 commits into from
Jan 20, 2024

Conversation

hartwork
Copy link
Collaborator

Fixes #164

Co-authored-by: Edgar Bonet <linux@edgar-bonet.org>
@hartwork hartwork added the bug Something isn't working label Jan 18, 2024
@hartwork hartwork added this to the 1.6.2 milestone Jan 18, 2024
Copy link
Collaborator

@edgar-bonet edgar-bonet left a 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.

@hartwork
Copy link
Collaborator Author

Didn't know you could do a Musl build without a dedicated VM or container. This is cool!

@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)…

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.

I'm with you there, I would not have touched the file unless needed. With the patch in ttyplot.c reverted, we would get this compile error that I'll also inline here so that we'll have a copy when that run log is long gone:

Compile errors log here, please click to expand
+ bmake
musl-gcc -std=c99 -pedantic -Werror  -O1 -Wall -Wextra `pkg-config --cflags ncursesw`  ttyplot.c `pkg-config --libs ncursesw` -lm -o ttyplot
ttyplot.c:68:8: error: unknown type name ‘cchar_t’
   68 | static cchar_t plotchar, max_errchar, min_errchar;
      |        ^~~~~~~
ttyplot.c:182:54: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  182 | static void draw_line(int x, int ph, int l1, int l2, cchar_t *c1, cchar_t *c2,
      |                                                      ^~~~~~~
      |                                                      wchar_t
ttyplot.c:182:67: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  182 | static void draw_line(int x, int ph, int l1, int l2, cchar_t *c1, cchar_t *c2,
      |                                                                   ^~~~~~~
      |                                                                   wchar_t
ttyplot.c:183:23: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  183 |                       cchar_t *hce, cchar_t *lce) {
      |                       ^~~~~~~
      |                       wchar_t
ttyplot.c:183:37: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  183 |                       cchar_t *hce, cchar_t *lce) {
      |                                     ^~~~~~~
      |                                     wchar_t
ttyplot.c:200:32: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  200 |                         int n, cchar_t *pc, cchar_t *hce, cchar_t *lce, double hm) {
      |                                ^~~~~~~
      |                                wchar_t
ttyplot.c:200:45: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  200 |                         int n, cchar_t *pc, cchar_t *hce, cchar_t *lce, double hm) {
      |                                             ^~~~~~~
      |                                             wchar_t
ttyplot.c:200:59: error: unknown type name ‘cchar_t’; did you mean ‘wchar_t’?
  200 |                         int n, cchar_t *pc, cchar_t *hce, cchar_t *lce, double hm) {
      |                                                           ^~~~~~~
      |                                                           wchar_t
ttyplot.c: In function ‘paint_plot’:
ttyplot.c:267:9: error: implicit declaration of function ‘asctime_r’; did you mean ‘asctime’? [-Werror=implicit-function-declaration]
  267 |         asctime_r(lt, ls);
      |         ^~~~~~~~~
      |         asctime
ttyplot.c:272:5: error: implicit declaration of function ‘mvvline_set’; did you mean ‘mvvline’? [-Werror=implicit-function-declaration]
  272 |     mvvline_set(height - 2, 5, &plotchar, 1);
      |     ^~~~~~~~~~~
      |     mvvline
ttyplot.c:287:5: error: implicit declaration of function ‘plot_values’ [-Werror=implicit-function-declaration]
  287 |     plot_values(plotheight, plotwidth, values1, values2, max, hardmin, n, &plotchar,
      |     ^~~~~~~~~~~
ttyplot.c: In function ‘main’:
ttyplot.c:535:17: error: request for member ‘chars’ in something not a structure or union
  535 |         plotchar.chars[0] = 0x2502;  // U+2502 box drawings light vertical
      |                 ^
ttyplot.c:537:17: error: request for member ‘chars’ in something not a structure or union
  537 |         plotchar.chars[0] = '|';  // U+007C vertical line
      |                 ^
ttyplot.c:538:16: error: request for member ‘chars’ in something not a structure or union
  538 |     max_errchar.chars[0] = 'e';
      |                ^
ttyplot.c:539:16: error: request for member ‘chars’ in something not a structure or union
  539 |     min_errchar.chars[0] = 'v';
      |                ^
ttyplot.c:592:33: error: request for member ‘chars’ in something not a structure or union
  592 |                 mbtowc(&plotchar.chars[0], optarg, MB_CUR_MAX);
      |                                 ^
ttyplot.c:595:36: error: request for member ‘chars’ in something not a structure or union
  595 |                 mbtowc(&max_errchar.chars[0], optarg, MB_CUR_MAX);
      |                                    ^
ttyplot.c:598:36: error: request for member ‘chars’ in something not a structure or union
  598 |                 mbtowc(&min_errchar.chars[0], optarg, MB_CUR_MAX);
      |                                    ^
cc1: all warnings being treated as errors
*** Error code 1

Stop.
bmake: stopped in /home/runner/work/ttyplot/ttyplot

The addition is only in the #else part of Apple because otherwise it broke compilation on macOS.

What do you think?

Copy link
Collaborator

@edgar-bonet edgar-bonet left a 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.

@hartwork
Copy link
Collaborator Author

@edgar-bonet cool!

@tenox7
Copy link
Owner

tenox7 commented Jan 20, 2024

LGTM, please let me know if you want me to merge it

@hartwork hartwork force-pushed the issue-164-fix-compilation-with-musl branch from 7bd8271 to f187abf Compare January 20, 2024 14:52
@hartwork hartwork merged commit 035ff43 into tenox7:master Jan 20, 2024
9 checks passed
@hartwork
Copy link
Collaborator Author

LGTM, please let me know if you want me to merge it

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!

@hartwork hartwork deleted the issue-164-fix-compilation-with-musl branch January 20, 2024 15:43
@hartwork
Copy link
Collaborator Author

hartwork commented Jan 23, 2024

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!

@hartwork
Copy link
Collaborator Author

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 🙏

@tenox7
Copy link
Owner

tenox7 commented Jan 28, 2024

Sincerest apologies for the delay. Tag 1.6.2 is pushed.

@tenox7
Copy link
Owner

tenox7 commented Jan 28, 2024

For the release could you post high level change log between 1.6 and 1.6.2? Just major stuff. Thanks.

@hartwork
Copy link
Collaborator Author

hartwork commented Jan 28, 2024

Sincerest apologies for the delay. Tag 1.6.2 is pushed.

@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.

For the release could you post high level change log between 1.6 and 1.6.2? Just major stuff. Thanks.

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of M_PI in stresstest.c is not C99 and unfortunately breaks compilation with musl libc
3 participants