Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Feb 28, 2019

Using the uriparser C library.

@pitrou pitrou force-pushed the ARROW-4697-cpp-uri-parsing branch 2 times, most recently from 1ccd56b to 685782b Compare February 28, 2019 15:41
@pitrou
Copy link
Member Author

pitrou commented Feb 28, 2019

I can reproduce the manylinux1 failure locally but I don't understand what's happening. CMake does find out the full path to the uriparser library:

-- Added static library dependency uriparser_static: /tmp/build-PY3.7-32/uriparser_ep-install/lib/liburiparser.a

However, for some reason, the linker command then uses relative paths and fails:

c++: error: uriparser_ep-install/lib/liburiparser.a: No such file or directory

@xhochy @kszucs Any idea?

@pitrou pitrou force-pushed the ARROW-4697-cpp-uri-parsing branch 2 times, most recently from 599f194 to abf1904 Compare February 28, 2019 17:54
@pitrou
Copy link
Member Author

pitrou commented Feb 28, 2019

MinGW build failure here. It seems the uriparser library doesn't build on MinGW:
https://ci.appveyor.com/project/pitrou/arrow/builds/22726490

I don't really know what to do with this.

@kou

@kou
Copy link
Member

kou commented Mar 1, 2019

There is a build error in tool/uriparse.c:

https://ci.appveyor.com/project/kou/arrow#L387

C:/projects/arrow/cpp/build/uriparser_ep-prefix/src/uriparser_ep/tool/uriparse.c:47:39: error: conflicting types for 'inet_ntop'
 WINSOCK_API_LINKAGE const char WSAAPI inet_ntop(
                                       ^~~~~~~~~
In file included from C:/projects/arrow/cpp/build/uriparser_ep-prefix/src/uriparser_ep/tool/uriparse.c:45:
C:/msys64/mingw64/x86_64-w64-mingw32/include/ws2tcpip.h:447:35: note: previous declaration of 'inet_ntop' was here
 WINSOCK_API_LINKAGE LPCSTR WSAAPI InetNtopA(INT Family, PVOID pAddr, LPSTR pStringBuf, size_t StringBufSize);
                                   ^~~~~~~~~
make[5]: *** [CMakeFiles/uriparse.dir/build.make:63: CMakeFiles/uriparse.dir/tool/uriparse.c.obj] Error 1
make[4]: *** [CMakeFiles/Makefile2:110: CMakeFiles/uriparse.dir/all] Error 2

It doesn't need to be built with -DURIPARSER_BUILD_TOOLS=OFF.
I sent a pull request to uriparser: uriparser/uriparser#56

@pitrou
Copy link
Member Author

pitrou commented Mar 1, 2019

Good catch. Apparently URIPARSER_BUILD_TOOLS was ignored.

@hartwork
Copy link

hartwork commented Mar 1, 2019

Thanks to @kou for the pull request regarding URIPARSER_BUILD_TOOLS!

I welcome help understanding how to fix the build for MinGW before I time for an in-depth analysis myself. Making Travis or AppVeyor cover MinGW for uriparser is yet to be done as of today.

@hartwork
Copy link

hartwork commented Mar 1, 2019

PS: Existing commit uriparser/uriparser@8bdd07e and the existing prototype might be of interest.

@kou
Copy link
Member

kou commented Mar 1, 2019

@hartwork Thanks for merging the pull request! I'll send another pull request for the build error.

@pitrou The pull request has been merged. Could you use the latest revision?

@pitrou
Copy link
Member Author

pitrou commented Mar 2, 2019

@kou i'll be away until March 15th. There should be no urgency here, but otherwise another core developer could still push updates to this PR ;-)

@xhochy
Copy link
Member

xhochy commented Mar 3, 2019

I can take of this PR once the CMake refactor is merged.

@pitrou pitrou force-pushed the ARROW-4697-cpp-uri-parsing branch from abf1904 to ea34e87 Compare March 18, 2019 13:59
@pitrou
Copy link
Member Author

pitrou commented Mar 18, 2019

I rebased on the latest CMake changes.
However there is a problem: by default, the CMake configuration tries to pull uriparser from Conda and fails. It looks like I have to pass -Duriparser_SOURCE=AUTO explicitly. This will not be practical: one has to add it to all CI and build scripts around. @xhochy What is your advice?

@pitrou pitrou force-pushed the ARROW-4697-cpp-uri-parsing branch from 6909dac to 37235ed Compare March 18, 2019 15:45
@pitrou pitrou force-pushed the ARROW-4697-cpp-uri-parsing branch from 6fe4024 to bca49e1 Compare March 18, 2019 17:40
@pitrou pitrou force-pushed the ARROW-4697-cpp-uri-parsing branch from bca49e1 to ff41cef Compare March 18, 2019 17:56
@pitrou
Copy link
Member Author

pitrou commented Mar 18, 2019

The manylinux1 issue is still strinking, as described in #3779 (comment)

Suggested by @xhochy.

Co-Authored-By: pitrou <pitrou@free.fr>
@pitrou
Copy link
Member Author

pitrou commented Mar 19, 2019

Successful AppVeyor build here: https://ci.appveyor.com/project/pitrou/arrow/builds/23190635

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@xhochy xhochy closed this in ca23513 Mar 20, 2019
@pitrou pitrou deleted the ARROW-4697-cpp-uri-parsing branch March 20, 2019 08:33
hartwork added a commit to uriparser/uriparser.github.io that referenced this pull request Mar 22, 2019
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.

4 participants