QW-Group / mvdsv

MVDSV: a QuakeWorld server
GNU General Public License v2.0
58 stars 56 forks source link

Add linking to pcre if it is installed on the system #101

Closed tdm4 closed 1 year ago

tdm4 commented 1 year ago

Detect PCRE library and if found, link to that instead of internal PCRE.

namtsui commented 1 year ago
Remove link to dl as it is in OpenBSD's libc
Have Cmake find and link to devel/pcre instead of the outdated bundled pcre
Index: CMakeLists.txt
--- CMakeLists.txt.orig
+++ CMakeLists.txt
@@ -59,8 +59,6 @@ set(SRC_COMMON
    "${DIR_SRC}/vfs_pak.c"
    "${DIR_SRC}/world.c"
    "${DIR_SRC}/zone.c"
-   "${DIR_SRC}/pcre/get.c"
-   "${DIR_SRC}/pcre/pcre.c"
    )

 # Check build target, and included sources
@@ -89,7 +87,25 @@ else()
    )
 endif()

+######################################################################################################

+# Check for PCRE. if found, include it; if not found, use bundled PCRE.
+find_library(PCRE_LIBRARIES pcre)
+if(PCRE_LIBRARIES)
+   set(PCRE_FOUND 1)
+   find_path(PCRE_INCLUDE_DIR pcre.h)
+endif(PCRE_LIBRARIES)
+
+if(NOT PCRE_FOUND)
+   message(STATUS "PCRE library not found. Using bundled PCRE intead.")
+   list(APPEND SRC_COMMON
+       "${DIR_SRC}/pcre/get.c"
+       "${DIR_SRC}/pcre/pcre.c"
+   )
+else()
+   message(STATUS "Found PCRE: ${PCRE_LIBRARIES}")
+endif()
+
 ######################################################################################################

 # Set base compiler flags
@@ -111,6 +127,7 @@ set_target_properties(${PROJECT_NAME}

 # Set include directories
 target_include_directories(${PROJECT_NAME} PRIVATE ${CURL_INCLUDE_DIRS})
+target_include_directories(${PROJECT_NAME} PRIVATE ${PCRE_INCLUDE_DIR})

 ######################################################################################################
@@ -118,7 +135,6 @@ target_include_directories(${PROJECT_NAME} PRIVATE ${C
 # Check build target, and included sources and libs
 if(UNIX)
    target_link_libraries(${PROJECT_NAME} m)
-   target_link_libraries(${PROJECT_NAME} dl)
 else()
    target_link_libraries(${PROJECT_NAME} ws2_32)
    target_link_libraries(${PROJECT_NAME} winmm)
@@ -147,6 +163,9 @@ if(CURL_FOUND)
    target_link_libraries(${PROJECT_NAME} ${CURL_LIBRARIES})
 endif()

+if(PCRE_FOUND)
+   target_link_libraries(${PROJECT_NAME} ${PCRE_LIBRARIES})
+endif()

 ######################################################################################################
namtsui commented 1 year ago

I tweaked your patch a bit. (ignore the removal of dl.)

===>  Configuring for mvdsv-0.35
-- The C compiler identification is Clang 13.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/obj/pobj/mvdsv-0.35/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Could NOT find CURL (missing: CURL_DIR)
-- Found PkgConfig: /usr/bin/pkg-config (found version "0.29.2")
-- Found CURL: /usr/local/lib/libcurl.so.26.15 (found version "7.84.0")
-- Found PCRE: /usr/local/lib/libpcre.so.3.0
-- LITTLE_ENDIAN
-- Configuring done
-- Generating done
tdm4 commented 1 year ago

@namtsui Yeah those look better. I've updated the branch to suit.

tdm4 commented 1 year ago

I've also added some logic so that the 'dl' library is only linked if the OS is UNIX but not OpenBSD (which has -ldl in base)

namtsui commented 1 year ago

oops I had a typo: Using bundled PCRE intead --> instead

namtsui commented 1 year ago

I don't think the !OpenBSD works. It should be like this: if(NOT CMAKE_SYSTEM_NAME MATCHES "OpenBSD")

tcsabina commented 1 year ago

@tdm4 , The Github Actions are failing on the linux builds with this. Do you see the build logs above (or below)?

Toma

namtsui commented 1 year ago

I don't think the !OpenBSD works. It should be like this: if(NOT CMAKE_SYSTEM_NAME MATCHES "OpenBSD")

Try running it again after tdm4 updates. It needs to be changed as mentioned above in order to link dl. As it currently is, !OpenBSD doesn't match Linux.

tdm4 commented 1 year ago

@tcsabina @namtsui I've updated the branch to fix the If statement logic. Do I need to squash my commits, or are they OK as is?

namtsui commented 1 year ago

oops I had a typo: Using bundled PCRE intead --> instead

fix this typo, too

Also, quote "OpenBSD" for the regex. Although, it seems to work fine without quotes. Examples of regex in the second link show quotes at least.

see: https://cmake.org/cmake/help/latest/command/if.html#matches https://cmake.org/cmake/help/latest/command/string.html#regex-specification

tcsabina commented 1 year ago

@tcsabina @namtsui I've updated the branch to fix the If statement logic. Do I need to squash my commits, or are they OK as is?

Hi! They are okay as is. And I don't mind having a couple of commits, instead of only one, even for a PR...

tcsabina commented 1 year ago

@tdm4 , Have you seen the request/discussion about adding quotes around OpenBSD?

tdm4 commented 1 year ago

@tdm4 , Have you seen the request/discussion about adding quotes around OpenBSD?

Yes, my apologies! I've updated the branch and put double quotes around OpenBSD. I think it should be good to go?

tcsabina commented 1 year ago

hi @tdm4 , I am sorry as well. Haven't seen somehow your reply on this, just now. Let's check the pipeline, and if it is green, I think we are good to go!