ARROW-5370: [C++] Use system uriparser if available #4525

wesm · 2019-06-12T02:03:41Z

This change will use uriparser >= 0.9.0 (found via pkgconfig) if it is available in the system toolchain, otherwise it will build from source. Using the SYSTEM method it will not fail for now if the version is not new enough given that the version we require is too new for many Linux distributions. We can look into supporting older uriparser some other time

kou · 2019-06-12T07:47:58Z

I tried this on Debian strecth. Debian stretch ships uriparser 0.8.4 https://packages.debian.org/search?suite=default&section=all&arch=any&searchon=names&keywords=uriparser

We use uriparser 0.9.0 API, uriParseSingleUriExA, here: https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/uri.cc#L131
So we can't use uriparser 0.8.4.

I need the following patch to confirm this change:

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 0841767e..ee0e24d4 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -56,6 +56,7 @@ set(ARROW_THIRDPARTY_DEPENDENCIES
     RapidJSON
     Snappy
     Thrift
+    uriparser
     ZLIB
     ZSTD)
 
@@ -597,7 +598,7 @@ if(ARROW_WITH_URIPARSER)
   endmacro()
 
   if("${uriparser_SOURCE}" STREQUAL "AUTO")
-    pkg_check_modules(URIPARSER_PC liburiparser)
+    pkg_check_modules(URIPARSER_PC "liburiparser >= 0.9.0")
     if(URIPARSER_PC_FOUND)
       set(URIPARSER_INCLUDE_DIR "${URIPARSER_PC_INCLUDEDIR}")
       list(APPEND URIPARSER_PC_LIBRARY_DIRS "${URIPARSER_PC_LIBDIR}")

+1 with this patch.

wesm · 2019-06-12T13:59:45Z

OK, thanks, I'll incorporate this change.

wesm · 2019-06-12T20:37:19Z

@kou done. I'll merge once the build look ok

kou · 2019-06-12T20:45:59Z

Ah, sorry. Please wait.
I'm trying this with MinGW. It seems that liburiparser.pc in MSYS2 is broken. There is no version information:

prefix=/mingw64
exec_prefix=/mingw64
libdir=/mingw64/lib
includedir=/mingw64/include

Name: liburiparser
Description: URI parsing and handling library

Version: 
URL: https://uriparser.github.io/
Libs: -L${libdir} -luriparser
Cflags: -I${includedir}

uriparser package in MSYS2 provides files for CMake:

ls /mingw64/lib/cmake/uriparser-0.9.2/
uriparser-release.cmake
uriparser.cmake

So the following logic is better like we did for other packages:

find_package(uriparser)
if(NOT uriparser_FOUND)
  pkg_check_modules(...)
  ...
endif()

Note that uriparser package in Debian doesn't provide files for CMake yet.

kou · 2019-06-12T20:46:50Z

Wow! We already have cpp/cmake_modules/Finduriparser.cmake. We should use this.

kou · 2019-06-12T20:58:08Z

I didn't try yet but something the following will be better. (Finduriparser.cmake is renamed to FinduriparserAlt.cmake)

diff --git a/cpp/cmake_modules/Finduriparser.cmake b/cpp/cmake_modules/FinduriparserAlt.cmake
similarity index 85%
rename from cpp/cmake_modules/Finduriparser.cmake
rename to cpp/cmake_modules/FinduriparserAlt.cmake
index a24cca47..95a9a7fe 100644
--- a/cpp/cmake_modules/Finduriparser.cmake
+++ b/cpp/cmake_modules/FinduriparserAlt.cmake
@@ -15,7 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
-pkg_check_modules(uriparser_PC liburiparser)
+pkg_check_modules(uriparser_PC "liburiparser >= 0.9.0")
 if(uriparser_PC_FOUND)
   set(uriparser_INCLUDE_DIR "${uriparser_PC_INCLUDEDIR}")
 
@@ -43,11 +43,13 @@ else()
   find_path(uriparser_INCLUDE_DIR NAMES uriparser/Uri.h PATH_SUFFIXES ${INCLUDE_PATH_SUFFIXES})
 endif()
 
-find_package_handle_standard_args(uriparser REQUIRED_VARS uriparser_LIB uriparser_INCLUDE_DIR)
+find_package_handle_standard_args(uriparserAlt REQUIRED_VARS uriparser_LIB uriparser_INCLUDE_DIR)
 
-if(uriparser_FOUND)
+if(uriparserAlt_FOUND)
   add_library(uriparser::uriparser UNKNOWN IMPORTED)
   set_target_properties(uriparser::uriparser
                         PROPERTIES IMPORTED_LOCATION "${uriparser_LIB}"
-                                   INTERFACE_INCLUDE_DIRECTORIES "${uriparser_INCLUDE_DIR}")
+                                   INTERFACE_INCLUDE_DIRECTORIES "${uriparser_INCLUDE_DIR}"
+				   # URI_STATIC_BUILD required on Windows
+                                   INTERFACE_COMPILE_DEFINITIONS "URI_NO_UNICODE")
 endif()
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 1fe04d79..8ad14b3c 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -590,9 +590,23 @@ macro(build_uriparser)
 endmacro()
 
 if(ARROW_WITH_URIPARSER)
-  # Unless the user overrides uriparser_SOURCE, build uriparser ourselves
-  if("${uriparser_SOURCE}" STREQUAL "")
-    set(uriparser_SOURCE "BUNDLED")
+  if(uriparser_SOURCE STREQUAL "AUTO")
+    find_package(uriparser QUIET)
+    if(NOT uriparser_FOUND)
+      # Ubuntu doesn't package the CMake config
+      find_package(uriparserAlt)
+    endif()
+    if(NOT uriparser_FOUND AND NOT uriparserAlt_FOUND)
+      build_uriparser()
+    endif()
+  elseif(uriparser_SOURCE STREQUAL "BUNDLED")
+    build_uriparser()
+  elseif(uriparser_SOURCE STREQUAL "SYSTEM")
+    find_package(uriparser QUIET)
+    if(NOT uriparser_FOUND)
+      # Ubuntu doesn't package the CMake config
+      find_package(uriparserAlt REQUIRED)
+    endif()
   endif()
 
   resolve_dependency(uriparser)

wesm · 2019-06-12T21:43:23Z

Oh okay. Do you want to take over this PR and push the commits here? I didn't realize we had the CMake file

kou · 2019-06-13T00:00:53Z

OK. I take over this.

Because uriparser isn't available on conda.

codecov-io · 2019-06-16T12:52:33Z

Codecov Report

Merging #4525 into master will increase coverage by 0.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4525      +/-   ##
==========================================
+ Coverage   88.16%   88.58%   +0.41%     
==========================================
  Files         852      860       +8     
  Lines      106398   108022    +1624     
  Branches     1253     1253              
==========================================
+ Hits        93806    95691    +1885     
+ Misses      12347    12052     -295     
- Partials      245      279      +34

Impacted Files	Coverage Δ
go/arrow/datatype_fixedwidth.go	`13.15% <0%> (-6.08%)`	⬇️
cpp/src/arrow/vendored/datetime/date.h	`26.35% <0%> (-4.28%)`	⬇️
go/arrow/array/boolean.go	`88.88% <0%> (-4.22%)`	⬇️
r/R/array.R	`72% <0%> (-4%)`	⬇️
python/pyarrow/lib.pyx	`97.82% <0%> (-2.18%)`	⬇️
go/arrow/array/binary.go	`86.2% <0%> (-2.03%)`	⬇️
cpp/src/arrow/json/chunked-builder.cc	`81.58% <0%> (-1.26%)`	⬇️
cpp/src/arrow/ipc/writer.cc	`93.04% <0%> (-1.15%)`	⬇️
python/pyarrow/tests/test_flight.py	`79.26% <0%> (-0.82%)`	⬇️
cpp/src/arrow/array.cc	`90.43% <0%> (-0.21%)`	⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ea86ff...5a06e06. Read the comment docs.

It works on local but doesn't work on AppVeyor.

kou · 2019-06-17T00:19:45Z

GLib CI failure on macOS is unrelated.
I'll merge this.

wesm · 2019-06-17T01:15:08Z

This patch is breaking my local builds (Ubuntu 19.04)

-- Performing Test DOUBLE_CONVERSION_HAS_CASE_INSENSIBILITY - Failed
CMake Error at /home/wesm/miniconda/envs/arrow-3.7/share/cmake-3.14/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find uriparserAlt (missing: uriparser_LIB uriparser_INCLUDE_DIR)
  (Required is at least version "0.9.0")
Call Stack (most recent call first):
  /home/wesm/miniconda/envs/arrow-3.7/share/cmake-3.14/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  cmake_modules/FinduriparserAlt.cmake:81 (find_package_handle_standard_args)
  cmake_modules/ThirdpartyToolchain.cmake:773 (find_package)
  CMakeLists.txt:374 (include)


-- Configuring incomplete, errors occurred!
See also "/home/wesm/code/arrow/cpp/build/CMakeFiles/CMakeOutput.log".
See also "/home/wesm/code/arrow/cpp/build/CMakeFiles/CMakeError.log".

wesm · 2019-06-17T01:18:39Z

The issue is that I'm using ARROW_DEPENDENCY_SOURCE=SYSTEM with a custom package prefix, and uriparser is not available in conda-forge. I'll have to pass -Duriparser_SOURCE=BUNDLED until this is remedied. I expect this is going to break our conda packages @kszucs

kou · 2019-06-17T01:30:46Z

OK. You can use ARROW_DEPENDENCY_SOURCE=CONDA for your use case. This patch includes workaround for ARROW_DEPENDENCY_SOURCE=CONDA: https://github.com/apache/arrow/pull/4525/files#diff-2420b0c5b6bdad921f1d538f92d64b59R81

wesm requested a review from kou June 12, 2019 02:03

wesm added 2 commits June 12, 2019 15:35

Use system liburiparser-dev if available

177fd95

Incorporate kou's suggestion

b7cec7c

wesm force-pushed the ARROW-5370 branch from 2cdfc06 to b7cec7c Compare June 12, 2019 20:35

wesm marked this pull request as ready for review June 12, 2019 20:35

fsaintjacques added the Component: C++ label Jun 14, 2019

kou added 5 commits June 16, 2019 18:28

Reuse existing Finduriparser.cmake

3caabe4

Use AUTO for uriparser even if ARROW_DEPENDENCY_SOURCE is CONDA

0464cf8

Because uriparser isn't available on conda.

Use JIRA issue number

b2bfcff

Need variable expansion for empty string

df2f76f

Use consistent order

5a06e06

kou added 3 commits June 17, 2019 05:29

Enable Flight on MinGW to use uriparser

8398c23

Install gRPC package for Flight

f26faaa

Disable Flight for now

e5e40cc

It works on local but doesn't work on AppVeyor.

kou closed this in e1aebf6 Jun 17, 2019

asfimport mentioned this pull request Jun 17, 2019

[C++] Detect system uriparser by default #21829

Closed

ARROW-5370: [C++] Use system uriparser if available #4525

ARROW-5370: [C++] Use system uriparser if available #4525

Uh oh!

Conversation

wesm commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Jun 12, 2019

Uh oh!

wesm commented Jun 12, 2019

Uh oh!

wesm commented Jun 12, 2019

Uh oh!

kou commented Jun 12, 2019

Uh oh!

kou commented Jun 12, 2019

Uh oh!

kou commented Jun 12, 2019

Uh oh!

wesm commented Jun 12, 2019

Uh oh!

kou commented Jun 13, 2019

Uh oh!

codecov-io commented Jun 16, 2019

Codecov Report

Uh oh!

kou commented Jun 17, 2019

Uh oh!

wesm commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Jun 17, 2019

Uh oh!

kou commented Jun 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wesm commented Jun 12, 2019 •

edited

Loading

wesm commented Jun 17, 2019 •

edited

Loading