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

CFLAGS c11 #262

Closed
jwijffels opened this issue Dec 11, 2022 · 8 comments
Closed

CFLAGS c11 #262

jwijffels opened this issue Dec 11, 2022 · 8 comments
Labels
build Build related issues

Comments

@jwijffels
Copy link
Contributor

Hello,

Many thanks for the code!
I'm writing an R wrapper around this C++ code at https://github.com/bnosac/audio.whisper
I'm now compiling the C code using -std=c11 -O3 as in https://github.com/bnosac/audio.whisper/blob/master/src/Makevars
But then I'm getting (e.g. at https://github.com/bnosac/audio.whisper/actions/runs/3671368911/jobs/6206523324)

Non-portable flags in variable 'PKG_CFLAGS':
  -std=c11 -O3

I'm trying to work around this by compiling with -O3 -D_POSIX_C_SOURCE=199309L but that gives me the following.
Any suggestions on how to fix this?

E> gcc -std=gnu99 -I"/usr/share/R/include" -DNDEBUG -pthread -DSTRICT_R_HEADERS  -I"/home/jwijffels/R/x86_64-pc-linux-gnu-library/3.6/Rcpp/include"  -O3 -D_POSIX_C_SOURCE=199309L -fpic  -g -O2 -fdebug-prefix-map=/build/r-base-jbaK_j/r-base-3.6.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -UNDEBUG -Wall -pedantic -g -O0 -fdiagnostics-color=always -c ggml.c -o ggml.o
E> In file included from ggml.c:1
E> ggml.h:711:6:warning: 99 doesn’t support unnamed structs/unions [-Wpedantic
E>   711 |     };
E>       |      ^
E> ggml.c:1287:15:error: pected declaration specifiers or ‘...before ‘sizeof
E>  1287 | static_assert(sizeofct ggml_object)%GGML_MEM_ALIGN == 0, "ggml_object size must be a multiple of GGML_MEM_ALIGN");
E>       |               ^~~~~~
E> ggml.c:1287:63:error: pected declaration specifiers or ‘...ore string constant
E>  1287 | static_assert(sizeof(struct ggml_object)%GGML_MEM_ALIGN == 0, "ggml_object size must be a multiple of GGML_MEM_ALIGN"
E>       |                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E> ggml.c:1288:15:error: pected declaration specifiers or ‘...before ‘sizeof
E>  1288 | static_assert(sizeofct ggml_tensor)%GGML_MEM_ALIGN == 0, "ggml_tensor size must be a multiple of GGML_MEM_ALIGN");
E>       |               ^~~~~~
E> ggml.c:1288:63:error: pected declaration specifiers or ‘...ore string constant
E>  1288 | static_assert(sizeof(struct ggml_tensor)%GGML_MEM_ALIGN == 0, "ggml_tensor size must be a multiple of GGML_MEM_ALIGN"
E>       |                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E> ggml.c:n function ‘ggml_nelements
E> ggml.c:1364:5:warning: plicit declaration of function ‘static_assert-Wimplicit-function-declaration
E>  1364 |     static_assert_MAX_DIMS == 4, "GGML_MAX_DIMS is not 4 - update this function");
E>       |     ^~~~~~~~~~~~~
E> make: *** [/usr/lib/R/etc/Makeconf:168: ggml.o] Error 1
@ggerganov ggerganov added the build Build related issues label Dec 12, 2022
@ggerganov
Copy link
Owner

Not familiar with R make files. Maybe try passing CFLAGS=-std=c11
The ggml depends on C11, so you must enable it somehow or it won't build.

@jwijffels
Copy link
Contributor Author

jwijffels commented Dec 12, 2022

Thanks for the feedback, I'm compiling it with CFLAGS=-std=c11, unfortunately although it compiles R does not allow that as indicated here as quoting from that email those flags are not portable, hence the R package will never make it to CRAN.
Hence my question if it would make sense to make it C99 compatible :) or is this a no-can-do due to the multithreading?

@ggerganov
Copy link
Owner

Should support C99 now - give it a try with latest master

@jwijffels
Copy link
Contributor Author

thanks! will try it out

@ggerganov
Copy link
Owner

Ah wait, <stdatomic.h> might still be a problem for C99. So it might not be that easy, sorry :)

@jwijffels
Copy link
Contributor Author

jwijffels commented Dec 13, 2022

That seems to be fixing it. Due to this change the R package now builds on Windows as well next to Ubuntu. I'll close this.
Many thanks for the intervention and most of all your time.

@jwijffels
Copy link
Contributor Author

jwijffels commented Dec 13, 2022

I've compiled the code on a Windows CRAN machine (winbuilder) which still complains on a few things, namely

gcc  -I"D:/RCompile/recent/R/include" -DNDEBUG -pthread -DSTRICT_R_HEADERS -I./dr_libs -I./whisper.cpp -I'D:/RCompile/CRANpkg/lib/4.3/Rcpp/include'   -I"d:/rtools42/x86_64-w64-mingw32.static.posix/include"     -pedantic -O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign  -c whisper.cpp/ggml.c -o whisper.cpp/ggml.o
In file included from whisper.cpp/ggml.c:2:
whisper.cpp/ggml.h:712:6: warning: ISO C99 doesn't support unnamed structs/unions [-Wpedantic]
  712 |     };
      |      ^
whisper.cpp/ggml.c:1294:119: warning: ISO C does not allow extra ';' outside of a function [-Wpedantic]
 1294 | static_assert(sizeof(struct ggml_object)%GGML_MEM_ALIGN == 0, "ggml_object size must be a multiple of GGML_MEM_ALIGN");
      |                                                                                                                       ^
whisper.cpp/ggml.c:1295:119: warning: ISO C does not allow extra ';' outside of a function [-Wpedantic]
 1295 | static_assert(sizeof(struct ggml_tensor)%GGML_MEM_ALIGN == 0, "ggml_tensor size must be a multiple of GGML_MEM_ALIGN");

I'm not sure if this is an easy fix or not.

@jwijffels
Copy link
Contributor Author

jwijffels commented Dec 19, 2022

I've incorporated these fixes and I'm now using version 1.0.4 in the R package. This seems now solved. Thanks!

rock3125 pushed a commit to rock3125/whisper.cpp that referenced this issue Feb 21, 2023
rock3125 pushed a commit to rock3125/whisper.cpp that referenced this issue Feb 21, 2023
anandijain pushed a commit to anandijain/whisper.cpp that referenced this issue Apr 28, 2023
anandijain pushed a commit to anandijain/whisper.cpp that referenced this issue Apr 28, 2023
jacobwu-b pushed a commit to jacobwu-b/Transcriptify-by-whisper.cpp that referenced this issue Oct 24, 2023
jacobwu-b pushed a commit to jacobwu-b/Transcriptify-by-whisper.cpp that referenced this issue Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build related issues
Projects
None yet
Development

No branches or pull requests

2 participants