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

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

Merged
merged 6 commits into from
Sep 4, 2022

Conversation

tdm4
Copy link

@tdm4 tdm4 commented Aug 22, 2022

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

@tdm4 tdm4 mentioned this pull request Aug 22, 2022
@namtsui
Copy link

namtsui commented Aug 23, 2022

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
Copy link

namtsui commented Aug 23, 2022

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

  • use PCRE_FOUND and target_include_directories instead of include_directories to make it more like the existing CMakeLists curl bits
  • no need for mark_as_advanced
  • only compile bundled pcre (get.c and pcre.c) if system pcre isn't found. it is based on the logic here: Use pkg-config to detect system-provided PCRE library #41
  • fix comment: # Check for PCRE. if found, include it; if not found, use bundled PCRE.
  • print helpful messages
===>  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
Copy link
Author

tdm4 commented Aug 23, 2022

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

@tdm4
Copy link
Author

tdm4 commented Aug 23, 2022

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
Copy link

namtsui commented Aug 23, 2022

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

@namtsui
Copy link

namtsui commented Aug 24, 2022

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

@tcsabina
Copy link
Collaborator

tcsabina commented Aug 24, 2022

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

Toma

@namtsui
Copy link

namtsui commented Aug 24, 2022

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
Copy link
Author

tdm4 commented Aug 25, 2022

@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
Copy link

namtsui commented Aug 25, 2022

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@tdm4
Copy link
Author

tdm4 commented Aug 30, 2022

@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
Copy link
Collaborator

tcsabina commented Sep 4, 2022

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!

@tcsabina tcsabina merged commit b5da7c4 into QW-Group:master Sep 4, 2022
@tdm4 tdm4 deleted the cmake-link-pcre branch September 6, 2022 20:00
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.

3 participants