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

Do not use _GNU_SOURCE gratuitously. #2035

Merged
merged 6 commits into from
Sep 8, 2023

Conversation

przemoc
Copy link
Contributor

@przemoc przemoc commented Jun 28, 2023

What is needed to build llama.cpp and examples is availability of stuff defined in The Open Group Base Specifications Issue 6 (https://pubs.opengroup.org/onlinepubs/009695399/) known also as Single Unix Specification v3 (SUSv3) or POSIX.1-2001 + XSI extensions, plus some stuff from BSD that is not specified in POSIX.1.

Well, that was true until NUMA support was added recently, so enable GNU libc extensions for Linux builds to cover that.

Not having feature test macros in source code gives greater flexibility to those wanting to reuse it in 3rd party app, as they can build it with FTMs set by Makefile here or other FTMs depending on their needs.

It builds without issues in Alpine (musl libc), Ubuntu (glibc), MSYS2.


llama : use posix_madvise() instead of madvise() derived from BSD

sed -i 's,<madvise>,posix_&,g;s,<MADV_,POSIX_&,g' llama-util.h


make : enable Darwin extensions for macOS builds

This is an attempt at fixing macOS build error coming from the fact that RLIMIT_MEMLOCK define is not available there without Darwin extensions.


cmake : follow recent FTM improvements from Makefile

@przemoc
Copy link
Contributor Author

przemoc commented Jun 28, 2023

macOS builds are working, that's good, but I have not tested cmake-driven builds, sorry for that, and of course they have to fail. I will have limited time for rest of the work week, so it's possible I will look at it only during weekend. I will have to check what are the best ways to provide FTMs in cmake, as I am not familiar with this build system.

@ggerganov
Copy link
Owner

No worries - whenever you get the time

@howard0su
Copy link
Collaborator

Check this for cmake fix:
howard0su@5b3d570

@przemoc
Copy link
Contributor Author

przemoc commented Jul 15, 2023

Thanks, @howard0su. I haven't spent much time with CMake (whenever I looked at any CMakeLists.txt files in the past over years, they repelled me for some reason), so I'm not able to judge atm if what you've done is proper (idiomatic) way of handling FTMs in CMake, but at least it looks like viable way way of doing that, so I may end up adding your commit to PR.

For now I've noticed that make in llama.cpp in MSYS2 (I mean UCRT64/CLANG64) was failing on some stuff, so I had to fix that first in #2235.

@przemoc
Copy link
Contributor Author

przemoc commented Jul 16, 2023

I rebased it (and did one new removal in one commit) on top of #2235 (fix-make-on-msys2), so do not merge current #2035 before merging #2235 first (which atm does not fix CMake on MSYS2, as I already commented there). I added modified commit from howard0su here. Was hoping CI checks would trigger automatically, as they were already enabled in the past for this PR, but it's not the case.

@przemoc
Copy link
Contributor Author

przemoc commented Jul 16, 2023

I rebased it on top of current #2235 (fix-make-on-msys2), where make and CMake work in MSYS2.
Do not merge current #2035 (remove-gnu-source-ftm) before merging #2235 first.
I'll rebase it to master when #2235 will be merged.

@przemoc
Copy link
Contributor Author

przemoc commented Jul 21, 2023

FreeBSD is not covered in llama.cpp checks, but if it would, it could potentially fail atm as it did in ggerganov/whisper.cpp#1129. Mentioning for the record.

@przemoc
Copy link
Contributor Author

przemoc commented Sep 3, 2023

I finally updated ggerganov/whisper.cpp#1129 today, so I will have to update this one too, but llama.cpp and whisper.cpp Makefiles diverge more and more.

@przemoc
Copy link
Contributor Author

przemoc commented Sep 5, 2023

I updated the branch, but it's kind of untested locally yet, so theoretically it may be not clean.

@przemoc
Copy link
Contributor Author

przemoc commented Sep 5, 2023

Portable applications should employ sysconf(_SC_PAGESIZE) instead of getpagesize(), so I will create separate PR for llama.cpp and whisper.cpp to do that, to be merged before this PR will be merged. But tbh, it's unclear to me if macOS supports sysconf(_SC_PAGESIZE) (as I don't see it in apple-oss-distributions/xnu, but maybe that's wrong place to look at). I guess checks will tell us the truth...

@przemoc
Copy link
Contributor Author

przemoc commented Sep 5, 2023

#3037 has been created. I added there madvise change too and will remove it from here (#2035) after #3037 will be merged.

What is needed to build llama.cpp and examples is availability of
stuff defined in The Open Group Base Specifications Issue 6
(https://pubs.opengroup.org/onlinepubs/009695399/) known also as
Single Unix Specification v3 (SUSv3) or POSIX.1-2001 + XSI extensions,
plus some stuff from BSD that is not specified in POSIX.1.

Well, that was true until NUMA support was added recently,
so enable GNU libc extensions for Linux builds to cover that.

Not having feature test macros in source code gives greater flexibility
to those wanting to reuse it in 3rd party app, as they can build it with
FTMs set by Makefile here or other FTMs depending on their needs.

It builds without issues in Alpine (musl libc), Ubuntu (glibc), MSYS2.
@przemoc
Copy link
Contributor Author

przemoc commented Sep 7, 2023

Rebased after #3037 got merged. Hopefully there won't be any problems in checks anymore.

Copy link
Owner

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @przemoc

Let's wait for the CI and merge if everything is OK

@ggerganov ggerganov merged commit cb6c44c into ggerganov:master Sep 8, 2023
26 checks passed
@przemoc przemoc deleted the remove-gnu-source-ftm branch September 8, 2023 22:07
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 this pull request may close these issues.

3 participants