tenox7 / ttyplot

a realtime plotting utility for terminal/console with data input from stdin
Apache License 2.0
956 stars 42 forks source link

Use of `M_PI` in `stresstest.c` is not C99 and unfortunately breaks compilation with musl libc #164

Closed hartwork closed 5 months ago

hartwork commented 5 months ago
# make -j16 
cc -O2 -march=x86-64 -pipe -pipe -frecord-gcc-switches -fno-diagnostics-color -fmessage-length=0 -Wall -Wextra `pkg-config --cflags ncursesw` -Wl,-O1 -Wl,--as-needed -Wl,--defsym=__gentoo_check_ldflags__=0 ttyplot.c `pkg-config --libs ncursesw` -lm -o ttyplot
cc -O2 -march=x86-64 -pipe -pipe -frecord-gcc-switches -fno-diagnostics-color -fmessage-length=0 -Wall -Wextra `pkg-config --cflags ncursesw` -Wl,-O1 -Wl,--as-needed -Wl,--defsym=__gentoo_check_ldflags__=0 stresstest.c `pkg-config --libs ncursesw` -lm -o stresstest
stresstest.c: In function 'main':
stresstest.c:74:69: error: 'M_PI' undeclared (first use in this function)
   74 | buffer_pos += sprintf(buffer + buffer_pos, "%.1f\n", (sin(n*M_PI/180)*5)+5);
      |                                                             ^~~~

stresstest.c:74:69: note: each undeclared identifier is reported only once for each function it appears in
stresstest.c:87:17: warning: implicit declaration of function 'usleep'; did you mean 'sleep'? [-Wimplicit-function-declaration[https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-function-declaration]]
   87 |                 usleep(50);  // let ttyplot read this before proceeding
      |                 ^~~~~~
      |                 sleep

Also note the usleep issue further down.

Origin: https://bugs.gentoo.org/922285

Besides a fix, we could add CI using Docker with an image of Alpine Linux to cover compilation with musl, if that sounds acceptable. It would help protect against regressions once this is fixed.

edgar-bonet commented 5 months ago

I suggest defining _XOPEN_SOURCE=500 in stresstest.c, as suggested by the glibc documentation. The Musl libc provides M_PI when this macro is defined, irrespective of its value.

hartwork commented 5 months ago

I suggest defining _XOPEN_SOURCE=500 in stresstest.c, as suggested by the glibc documentation. The Musl libc provides M_PI when this macro is defined, irrespective of its value.

@edgar-bonet that's an interesting find and idea. I would want to test out this precise addition to the very top of stresstest.c then:

#if !defined(_XOPEN_SOURCE) || (_XOPEN_SOURCE < 500)
# define _XOPEN_SOURCE 500
#endif

Besides a fix, we could add CI using Docker with an image of Alpine Linux to cover compilation with Musl, if that sounds acceptable. It would help protect against regressions once this is fixed.

@edgar-bonet @tenox7 how do you feel about this direction in general?

edgar-bonet commented 5 months ago

In order to avoid a “redefined” warning, I would undefine _XOPEN_SOURCE if it has an unsuitable value:

#if _XOPEN_SOURCE < 500
# undef _XOPEN_SOURCE
#endif
#ifndef _XOPEN_SOURCE
# define _XOPEN_SOURCE 500
#endif

Note that this is safe even if _XOPEN_SOURCE is initially undefined:

how do you feel about [a CI with Musl/Alpine/Docker] in general?

Great! I am not that comfortable with Docker, but I appreciate the great job you are doing on the CI front.

hartwork commented 5 months ago

In order to avoid a “redefined” warning, I would undefine _XOPEN_SOURCE if it has an unsuitable value:

#if _XOPEN_SOURCE < 500
# undef _XOPEN_SOURCE
#endif
#ifndef _XOPEN_SOURCE
# define _XOPEN_SOURCE 500
#endif

Note that this is safe even if _XOPEN_SOURCE is initially undefined:

  • an undefined macro evaluates as zero within a #if
  • undefining an undefined macro is a no-op.

@edgar-bonet would you be okay with this alternative?:

#if !defined(_XOPEN_SOURCE) || (_XOPEN_SOURCE < 500)
# undef _XOPEN_SOURCE  // to address warnings about re-definition
# define _XOPEN_SOURCE 500
#endif

It seems to play by the rules you described and I would find it more "contained" and self-explaining, a bit easier to look at, but that's potentially all subjective. What do you think?

how do you feel about [a CI with Musl/Alpine/Docker] in general?

Great! I am not that comfortable with Docker, but I appreciate the great job you are doing on the CI front.

Thanks!

edgar-bonet commented 5 months ago

Yes, that way of (re)defining _XOPEN_SOURCE looks good to me. I would even be happy with the “magic number” version:

#ifndef M_PI
# define M_PI 3.1415926535897932
#endif