build/CMake: find_package(… REQUIRED) #8464
Conversation
|
@jszakmeister |
CMakeLists.txt
Outdated
| find_package(LibIntl) | ||
| if(LibIntl_FOUND) | ||
| if(ENABLE_LIBINTL) | ||
| find_package(Intl REQUIRED) |
There was a problem hiding this comment.
@jamessan @jszakmeister Is find_package(LibIntl) needed instead of find_package(Intl) for some reason?
There was a problem hiding this comment.
Yes, because find_package(LibIntl) is actually delegating to our cmake/FindLibIntl.cmake file. If we provide the wrong name, then CMake can't locate it.
There was a problem hiding this comment.
BTW, I'm not sure why find_package(Intl) seems to work and find_package(LibIntl) doesn't. :-/ Maybe it was partially related to gettext missing? But I don't really see how that would happen.
There was a problem hiding this comment.
Upstream's is FindIntl:
[jamessan@freya] 0 % dpkg -S FindLibIntl
dpkg-query: no path found matching pattern *FindLibIntl*
[jamessan@freya] 1 % dpkg -S FindIntl
cmake-data: /usr/share/cmake-3.11/Modules/FindIntl.cmake
cmake-data: /usr/share/cmake-3.11/Help/module/FindIntl.rst
Is there a reason we don't use that or build our own on top of that?
There was a problem hiding this comment.
Oh, and one other thing. The FindIntl.cmake that comes with CMake doesn't do the work of finding it via Homebrew--which is why we don't use the standard one.
There was a problem hiding this comment.
@jamessan Yes, it doesn't find it when Homebrew is involved on macOS.
There was a problem hiding this comment.
Isn't there a way we can instruct FindIntl to consider homebrew's paths too?
7fd9b05 to
04e93e7
Compare
.travis.yml
Outdated
| @@ -53,7 +53,7 @@ jobs: | |||
| compiler: clang | |||
| env: > | |||
| CLANG_SANITIZER=ASAN_UBSAN | |||
| CMAKE_FLAGS="$CMAKE_FLAGS -DPREFER_LUA=ON" | |||
| CMAKE_FLAGS="$CMAKE_FLAGS -DPREFER_LUA=OFF" | |||
There was a problem hiding this comment.
This is intentionally using Lua (c.f. 8fec4d5) because we build Lua with ASAN to be able to test our embedded Lua support.
Hmm, I don't depend on that for Debian's official packaging either and the build works... Ah, because it's pulled in by other packages: I should make that explicit. |
|
Updated the wiki. |
|
If |
04e93e7 to
dc3124f
Compare
I did some more digging, turns out that was by accident: it only fails if I tried to better encapsulate this stuff in our |
|
Probably need this to fix the ASAN build. Not sure why it worked before. diff --git i/ci/common/build.sh w/ci/common/build.sh
index a3cf64d47..dfd96051a 100644
--- i/ci/common/build.sh
+++ w/ci/common/build.sh
@@ -23,7 +23,7 @@ build_deps() {
fi
if test "${FUNCTIONALTEST}" = "functionaltest-lua" \
|| test "${CLANG_SANITIZER}" = "ASAN_UBSAN" ; then
- DEPS_CMAKE_FLAGS="${DEPS_CMAKE_FLAGS} -DUSE_BUNDLED_LUA=ON"
+ DEPS_CMAKE_FLAGS="${DEPS_CMAKE_FLAGS} -DUSE_BUNDLED_LUA=ON -DUSE_BUNDLED_LUAJIT=OFF"
fi
mkdir -p "${DEPS_BUILD_DIR}" |
Although, the main cmake should be preferring Lua even if LuaJIT is present. Why isn't that happening... |
Good question. If you're looking at master, it looks like this: if(PREFER_LUA)
find_package(Lua REQUIRED)
set(LUA_PREFERRED_INCLUDE_DIRS ${LUA_INCLUDE_DIR})
set(LUA_PREFERRED_LIBRARIES ${LUA_LIBRARIES})
find_package(LuaJit)
else()
find_package(LuaJit REQUIRED)
set(LUA_PREFERRED_INCLUDE_DIRS ${LUAJIT_INCLUDE_DIRS})
set(LUA_PREFERRED_LIBRARIES ${LUAJIT_LIBRARIES})
endif()It looks like the |
|
Actually, that looks intentional: 7383274 |
|
Yeah, the fallback is intentional since we need a Lua system. The fallbacks should change to only be used if needed, though. |
|
@jamessan I think it's a little more than that reading the comments. It looks like we link nvim with Lua and nvim-test with LuaJit, since it's required (at least according to that commit). So it seems like we need to find both when |
|
AFAIR the main problem was libnvim-test being built without ASAN, but trying to use library linked with ASAN leading to lots of liker errors. Not sure what were/are the troubles of building luajit, lua and libnvim-test all with ASAN, but the second main problem is that lua does not have ffi. So whether you build lua and libnvim-test with ASAN or not you still have to build luajit, just when you build libnvim-test with ASAN you also need to build luajit with ASAN: I am not sure what ASAN would do when e.g. checking for out-of-bounds array access if said array comes from luajit in this case, but there really are only two options: crash or be useless, and neither option is particularly good. |
1669a5d to
2961bda
Compare
|
@b-r-o-c-k The MSVC build fails when I add |
e0d1540 to
e71b336
Compare
|
@justinmk The gettext library is split into two parts: runtime (libintl) and tools. #8163 only adds the runtime part to the bundled dependencies. Building the gettext tools requires a lot of dependencies, one of which is libiconv. I will try to add libiconv and a CMake config for the tools to the bundled dependencies. |
|
Thanks, let's not bother building the tools then. We can use Iconv maybe we can find a binary somewhere. |
|
Edit: no, we do |
1705dfa to
3cc1ee5
Compare
|
@justinmk What's the use-case for |
|
@janlazo will that do the right thing if a developer double-clicks on it or just executes it? If so then we can get rid of builddeps.bat (renamed to makedeps.bat). |
a55550f to
7376ac3
Compare
"Always use `find_package` with `REQUIRED`." - We make an exception for LuaJit (not REQUIRED): the `nvim-test` target is included only if we can find LuaJit. This is partially a cargo-cult (reference below), but it uncovered at least one problem: `find_package(LibIntl REQUIRED)` fails on my vanilla ubuntu 16.04 system. ref: https://schneide.blog/2017/11/06/4-tips-for-better-cmake/ > optional dependencies is nice, but skipping on REQUIRED is not the way > you want to do it. In the worst case, some of your features will just > not work if those packages are not found, with no explanation > whatsoever. Instead, use explicit feature-toggles (e.g. using option()) > that either skip the find_package call or use it with REQUIRED, so the > user will know that another lib is needed for this feature.
If check_c_source_compiles() succeeded (HAVE_WORKING_LIBINTL is set) then the result of find_xxx() doesn't matter. This happens on systems (linux+glibc) where libintl is available passively. This allows `find_package(LibIntl REQUIRED)` to work and will still correctly fail (REQUIRED) on systems lacking libintl.
- We need the gettext tools (msgmerge.exe) because these aren't built when we build from source (not trivial). - We can use the pre-built libiconv-2.dll for DYNAMIC_ICONV_DLL.
ssize_t is already typedef's by libuv:uv-win.h
7376ac3 to
ec29eee
Compare
|
on master the MSVC_64 build fails with this mysterious message: |
|
I can't reproduce the error on my machine using #8405 |
| set(GETTEXT_X86_URL https://github.com/mlocati/gettext-iconv-windows/releases/download/v0.19.8.1-v1.15/gettext0.19.8.1-iconv1.15-shared-32.zip) | ||
| set(GETTEXT_X86_SHA256 b7d8fe2d038950bc0447d664db614ebfc3100a1ba962a959d78e41bc708a2140) | ||
| set(GETTEXT_X86_64_URL https://github.com/mlocati/gettext-iconv-windows/releases/download/v0.19.8.1-v1.15/gettext0.19.8.1-iconv1.15-shared-64.zip) | ||
| set(GETTEXT_X86_64_SHA256 c8ed2897438efc0a511892c2b38b623ef0c9092aced2acbd3f3daf2f12ba70b4) |
There was a problem hiding this comment.
Are these used in any builds?
"Always use
find_packagewithREQUIRED."This is partially a cargo-cult (reference below), but it uncovered at
least one problem:
find_package(LibIntl REQUIRED)fails on my vanillaubuntu 16.04 system, whereas
find_package(Intl REQUIRED)succeeds.ref: https://schneide.blog/2017/11/06/4-tips-for-better-cmake/
FindLibIntl.cmakehandleREQUIREDDYNAMIC_ICONVmakedeps.batuntilbuild.ps1can replace it for developers using VS.I didn't enable any new tests related to iconv, but I tested it locally with:
Nvim appveyor build log shows
iconv/dynfeature is enabled: