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 static linking for tests and extras, remove unneeded -static flag #98

Merged
merged 4 commits into from
Jun 14, 2023

Conversation

schamane
Copy link
Contributor

Hi,

on macosx you cant staticaly link with "-static" flag for binaries. So it should be removed.
Please review my pool request for this :D

extras/CMakeLists.txt Outdated Show resolved Hide resolved
@saharNooby
Copy link
Collaborator

Do I understand correctly that the added code just removes "-static" option from compiler options when compiling extras and tests?

CC @LoganDark just in case -- he is more familliar with C/C++ :)

@schamane
Copy link
Contributor Author

Yes, at least on macosx you dont need "-static" flag to build binaries, it event worse. It break your build.

https://leanprover-community.github.io/archive/stream/113488-general/topic/libgmp.20dependency.html

image

Co-authored-by: Alex <saharNooby@users.noreply.github.com>
@LoganDark
Copy link
Contributor

Yeah no, this is a grave misunderstanding of the screenshotted article. You only can't statically link libSystem.dylib. You can however statically link with individual libraries. There should be no problem here

@schamane
Copy link
Contributor Author

schamane commented Jun 14, 2023

Thats true @LoganDark, but on mac build process crash if you try to build statically tests and extras...

tests and extras binaries will be successful statically build with librwkv.a even without "-static" flag.

Here is the way to reproduce:

mkdir build && cd build
cmake -DRWKV_BUILD_SHARED_LIBRARY=OFF -DRWKV_STATIC=ON ..
cmake --build . --config Release

Output:

4 warnings generated.
[ 23%] Linking CXX static library librwkv.a
[ 23%] Built target rwkv
[ 30%] Building C object tests/CMakeFiles/test_ggml_basics.dir/test_ggml_basics.c.o
[ 38%] Linking CXX executable ../bin/test_ggml_basics
ld: library not found for -lcrt0.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [bin/test_ggml_basics] Error 1
make[1]: *** [tests/CMakeFiles/test_ggml_basics.dir/all] Error 2

@LoganDark
Copy link
Contributor

Thats true @LoganDark, but on mac build process crash if you try to build statically tests and extras...

does removing -static still allow you to statically link with rwkv.cpp?

@schamane
Copy link
Contributor Author

Thats true @LoganDark, but on mac build process crash if you try to build statically tests and extras...

does removing -static still allow you to statically link with rwkv.cpp?

Yes, here is build without -static flag in tests and extras:

❯ otool -L bin/rwkv_cpu_info 
bin/rwkv_cpu_info:
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)
        /System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate (compatibility version 1.0.0, current version 4.0.0)
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1500.65.0)

extras/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@LoganDark LoganDark left a comment

Choose a reason for hiding this comment

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

LGTM, doesn't look like these need to be statically linked on any platform

@schamane
Copy link
Contributor Author

Done

@schamane schamane requested a review from saharNooby June 14, 2023 14:37
@saharNooby saharNooby merged commit 5316068 into RWKV:master Jun 14, 2023
24 checks passed
@schamane schamane deleted the fix/static_linking branch June 15, 2023 11:32
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.

None yet

3 participants