feat: Use system libuv if available #1861
Conversation
| # Based on https://github.com/xenoscopic/libuv-cmake/blob/fc164590c6c723fd474e35b0b52022ebaecef745/Findlibuv.cmake | ||
| find_path (libuv_INCLUDE_DIR NAMES uv.h) | ||
| find_library (libuv_LIBRARY NAMES uv libuv) | ||
| find_package_handle_standard_args(libuv libuv_LIBRARY libuv_INCLUDE_DIR) |
There was a problem hiding this comment.
IIUC, L14 and L15 will search uv.h and libuv.so in the default path which usually is also the system path and doesn't include a directory of WAMR source tree. So I am not sure it will locate the libuv downloaded by CMake.
There was a problem hiding this comment.
This is not designed to load libuv downloaded by cmake. It is only downloaded below if there is no system installed libuv
There was a problem hiding this comment.
I've updated the original description for clarity
There was a problem hiding this comment.
Got it. It would be helpful if using system providing libuv.so.
BTW, how about the version of system installed libuv? Should be worried about the compatibility problem?
There was a problem hiding this comment.
And how to install the system packages of libuv and uvwasi? Could you help update the document?
There was a problem hiding this comment.
And how to install the system packages of libuv and uvwasi? Could you help update the document?
@wenyongh this PR is not completed. Is this a good way to proceed? If so, I can make similar changes for uvwasi.
There was a problem hiding this comment.
It is good to me if it really resolves the issue that developers met, could you make the changes for uvwasi, and update the document? e.g. update the Note of Configure LIBC here:
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/doc/build_wamr.md#configure-libc
There was a problem hiding this comment.
I've been working on this, but when the uvwasi source isn't available, the cmake build process errors that it cannot find any of the symbols. Either I'm incorrectly linking uvwasi or maybe it doesn't support linking.
phated
force-pushed
the
phated/use-libuv-global
branch
from
cf8f356
to
8c5f4c6
Compare
|
I believe this is completed but it relies on nodejs/uvwasi#210 |
|
Looks like some of the linux tests flaked on trying to download an image |
| FetchContent_Populate(uvwasi) | ||
| include_directories("${uvwasi_SOURCE_DIR}/include") | ||
| add_subdirectory(${uvwasi_SOURCE_DIR} ${uvwasi_BINARY_DIR} EXCLUDE_FROM_ALL) | ||
| set (UVWASI_LIBRARIES uvwasi_a) |
There was a problem hiding this comment.
Hi, I tested the PR, it reported error in my machine (I didn't install uvwasi package):
/usr/bin/ld: _deps/libuv-build/libuv_a.a(core.c.o): relocation R_X86_64_PC32 against symbol `environ@@GLIBC_2.2.5' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit statusIt can be resoled by adding PIC propertie for uvwasi_a and uv_a here (like L32 above):
set_target_properties(uvwasi_a PROPERTIES POSITION_INDEPENDENT_CODE 1)
set_target_properties(uv_a PROPERTIES POSITION_INDEPENDENT_CODE 1)Could you help check and add the them?
There was a problem hiding this comment.
It looks like this was already added for uv_a so I just added it to uvwasi_a. Please let me know if that works.
There was a problem hiding this comment.
The issue in my machine is that package libuv1-dev is installed while no uvwasi related package is installed, and find_package(LIBUV QUIET) returns true but uvwasi must be compiled from source code. And when downloading and compiling uvwasi, it also downloads, compiles and links libuv (static lib) instead of linking the libuv.so from the installed package. And here the compiler reports error when linking libuv.a. I don't know how to let uvwasi use the installed libuv package directly, if we can, it is unnecessary to add the line. Otherwise, had better add the line. Any idea for it? Thanks.
There was a problem hiding this comment.
I don't know how to let uvwasi use the installed libuv package directly, if we can, it is unnecessary to add the line.
Sorry for the confusion. My PR at nodejs/uvwasi#210 was required to have uvwasi use the same global version. I've now merged the PR into the main branch so you should be able to build with the new source code and it will link the global libuv.
There was a problem hiding this comment.
OK, thanks a lot. I tested again, it links the correct libuv now. An issue is in Ubuntu-20.04, when package libuv1-dev is installed, compile warning and link error occur:
product-mini/platforms/linux/build/_deps/uvwasi-src/src/uvwasi.c:1743:7: warning: implicit declaration of function ‘uv_fs_lutime’; did you mean ‘uv_fs_futime’? [-Wimplicit-function-declaration]
1743 | r = uv_fs_lutime(NULL, &req, resolved_path, atim, mtim, NULL);[ 98%] Linking C executable iwasm
/usr/local/bin/ld: _deps/uvwasi-build/libuvwasi_a.a(uvwasi.c.o): in function `uvwasi_path_filestat_set_times':
uvwasi.c:(.text+0x293f): undefined reference to `uv_fs_lutime'Note that the warning/error isn't reported in Ubuntu-22.04, it works fine in Ubuntu-22.04.
I think it is due to the installed libuv.so doesn't have the uv_fs_lutime function. It is not the issue of this PR, let's merge it first. And it would be good if we can resolve the issue, e.g. modifying uvwasi by using uv_fs_futime?
There was a problem hiding this comment.
That sounds good. I don't have experience with the libuv function mentioned, but I can raise the issue for the other members of the uvwasi team with more libuv experience.
There was a problem hiding this comment.
That issue has been raised at nodejs/uvwasi#213
This PR attempts to search for the system libuv and use it if found instead of downloading it. As reported in bytecodealliance#1831, this is needed because some tools build in a sandbox and clear the extra sources.
This is my attempt at searching for the system libuv
beforeinstead of downloading it, if libuv exists on the system already. As I mentioned in #1831, this is needed because some tools build in a sandbox and clear the extra sources.I didn't implement this for uvwasi because I wanted to get feedback first.