mheily / libkqueue

kqueue(2) compatibility library
Other
236 stars 77 forks source link

CMake warnings when building as submodule to another project #133

Closed timwoj closed 2 years ago

timwoj commented 2 years ago

This came up after upgrading Zeek's libkqueue submodule to 2.6.0:

********** Begin libkqueue External Project CMake Output ************

CMake Deprecation Warning at CMakeLists.txt:26 (cmake_policy):
  The OLD behavior for policy CMP0063 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.

CMake Warning (dev) at CMakeLists.txt:56 (MATH):
  Unexpected character in expression at position 1: d

  Unexpected character in expression at position 2: e

  Unexpected character in expression at position 3: v

This warning is for project developers.  Use -Wno-dev to suppress it.

Version 2.6.0-1 (13150b03)
-- Configuring done
-- Generating done

We build libkqueue as a submodule and link it as a static library using CMake's ExternalProject module. One of my teammates took a look at why we get this warning, and came up with this:

The policy is about how broadly symbol visibility constraints apply: https://cmake.org/cmake/help/latest/policy/CMP0063.html. From looking at our FindKqueue.cmake it doesn't look like we're doing anything special regarding these. libkqueue itself declares set(CMAKE_C_VISIBILITY_PRESET hidden). I built and tested with the policy directive commented out, and it ran through fine. If I read the policy correctly then the new policy means the visibility applies more widely, and so if everything still works, seems we're fine.

The second warning is because libkqueue's CMakeLists.txt has a few execute_process() instances that dig into git and don't run from libkqueue's source, but Zeek's. Looks like we're specifying Zeek's build dir as working directory in FindKqueue.cmake, for good reason. If we add WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}) in libkqueue's CMakeLists.txt, it works fine.

So, all in all:

diff --git i/CMakeLists.txt w/CMakeLists.txt
index e82f24c..60bf075 100644
--- i/CMakeLists.txt
+++ w/CMakeLists.txt
@@ -23,14 +23,15 @@ else()
   # For RPM packaging to work correctly version >= 3.8 is required
   cmake_minimum_required(VERSION 3.8.0)
 endif()
-cmake_policy(SET CMP0063 OLD)
+#cmake_policy(SET CMP0063 OLD)

 # Both variables used in src/version.h.in
 string(TIMESTAMP PROJECT_VERSION_DATE "%b %d %Y at %H:%M:%S")

 execute_process(COMMAND git rev-parse --short=8 HEAD
                 OUTPUT_VARIABLE LIBKQUEUE_VERSION_COMMIT
-                OUTPUT_STRIP_TRAILING_WHITESPACE)
+                OUTPUT_STRIP_TRAILING_WHITESPACE
+                WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})

 if(LIBKQUEUE_VERSION_COMMIT STREQUAL "")
        unset(LIBKQUEUE_VERSION_COMMIT)
@@ -46,7 +47,8 @@ if(VERSION_OVERRIDE)
 else()
   execute_process(COMMAND sh -c "git describe --tags --match 'v*' | grep '-' | cut -d- -f2"
                   OUTPUT_VARIABLE PROJECT_VERSION_RELEASE
-                  OUTPUT_STRIP_TRAILING_WHITESPACE)
+                  OUTPUT_STRIP_TRAILING_WHITESPACE
+                  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})

   if(PROJECT_VERSION_RELEASE STREQUAL "")
     MESSAGE("Version ${PROJECT_VERSION} (${LIBKQUEUE_VERSION_COMMIT})")

Do you want me to put up a PR for this?

arr2036 commented 2 years ago

Yes, go ahead and raise a PR.