Skip to content

common/log.h:290:61: error: expected primary-expression before ',' token #2898

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

Closed
munitioner opened this issue Aug 30, 2023 · 13 comments · Fixed by #2911
Closed

common/log.h:290:61: error: expected primary-expression before ',' token #2898

munitioner opened this issue Aug 30, 2023 · 13 comments · Fixed by #2911

Comments

@munitioner
Copy link

Running environment: Windows

Compilation method: BLAS Build

When I open w64devkit.exe and CD it to the llama.cpp directory, enter the command make LLAMA_ OPENBLAS=1 encountered the following error

image

@ghost
Copy link

ghost commented Aug 30, 2023

Hi, the log's new, but I had no problems building in Linux with OpenBlas, so I think this is a Windows specific issue. You might try without OpenBlas for now.

@staviq Do you recognize this compile issue?

@ggerganov
Copy link
Member

If you change ##__VA_ARGS__ to __VA_ARGS__ in common/log.h does it compile?

@staviq
Copy link
Contributor

staviq commented Aug 30, 2023

This is likely Windows stray comma shenanigans, builds were fully passing yesterday, so that has to be some quirk of w64devkit

If you can tell me how did you set up the compilation environment I would be able to recreate it

In the mean time

Can you edit log.h and at the top, after includes and before the rest add

#undef _WIN32

and at the very bottom add

#define _WIN32

And see if it fixes it ?

Edit: This w64devkit ? As in, mingw thing ? https://www.mingw-w64.org/downloads/
Because in that case, I think the problem is with the use of _WIN32 flag, I would have to find something else to detect windows msvc specifically

@LostRuins
Copy link
Collaborator

I am using w64devkit and am encountering the same issue.

@staviq
Copy link
Contributor

staviq commented Aug 30, 2023

I'm stuck at work for couple more hours, but i have a general idea as to what is going on.

@staviq
Copy link
Contributor

staviq commented Aug 30, 2023

Ok, this is 100% because _WIN32 is defined on w64devkit, yet it compiles the "unix" way with gcc.

Fix is already on the way, just testing if it doesn't break other builds.

I used ifdef _WIN32 because other parts of the project are using it, but it turns out to be a bad idea, and now i can see lots of warning from other source files related to _WIN32

Proper way of detecting MSVC seems to be checking for _WIN32 and __MINGW32__ too.

This appears to be specific to w64devkit/msys/mingw

@Coloris
Copy link

Coloris commented Aug 30, 2023

I have the exact same issue using the latest version of w64devkit (https://github.com/skeeto/w64devkit/releases/tag/v1.20.0)

Just doing this :

Run w64devkit.exe
cd llama.cpp
make

@slaren
Copy link
Member

slaren commented Aug 30, 2023

@staviq I think you can use _MSC_VER to detect MSVC.

@staviq
Copy link
Contributor

staviq commented Aug 30, 2023

@slaren Take a look at the fix PR, and if you think _MSC_VER is more suitable, I'll do it.

Edit: I'm trying to figure out if there is a need for detecting Linux build, Linux-like builds on Windows and Windows builds on Windows, separately

Done.

@munitioner
Copy link
Author

If you change to in does it compile?##__VA_ARGS__``__VA_ARGS__``common/log.h
According to your method, if the compilation error is not resolved, the same compilation error will still occur

@munitioner
Copy link
Author

After recompiling, there was no error: expected primary expression before ',' token error, but a new error occurred

This is likely Windows stray comma shenanigans, builds were fully passing yesterday, so that has to be some quirk of w64devkit

If you can tell me how did you set up the compilation environment I would be able to recreate it

In the mean time

Can you edit log.h and at the top, after includes and before the rest add

#undef _WIN32

and at the very bottom add

#define _WIN32

And see if it fixes it ?

Edit: This w64devkit ? As in, mingw thing ? https://www.mingw-w64.org/downloads/ Because in that case, I think the problem is with the use of _WIN32 flag, I would have to find something else to detect windows msvc specifically

After recompiling, there was no error: expected primary expression before ',' token error, but a new error occurred
image

@staviq
Copy link
Contributor

staviq commented Aug 31, 2023

Fix for this particular issue is already prepared to be merged, but we noticed other problems with w64devkit specifically, and we aren't sure yet, but those other problems might be w64devkit fault, so I would try something other than w64devkit untill we determine the source cause.

@munitioner
Copy link
Author

Fix for this particular issue is already prepared to be merged, but we noticed other problems with w64devkit specifically, and we aren't sure yet, but those other problems might be w64devkit fault, so I would try something other than w64devkit untill we determine the source cause.

I compiled successfully with cmake and did not encounter any errors

ggerganov pushed a commit that referenced this issue Sep 1, 2023
* fix mingw-like builds

* formatting

* make LOG_COMPAT easier to override and extend

* simplify win detection

* fix for #2940
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 a pull request may close this issue.

6 participants