Repository navigation
Allow Windows with WinHTTP to use external http-parser - #4015
ethomson merged 2 commits into
Conversation
Sorry, something went wrong.
| ADD_DEFINITIONS(-DGIT_WINHTTP) | ||
|
|
||
| FIND_PACKAGE(HTTP_Parser) | ||
| IF (HTTP_PARSER_FOUND AND HTTP_PARSER_VERSION_MAJOR EQUAL 2) |
There was a problem hiding this comment.
I'd say this check HTTP_PARSER_VERSION_MAJOR EQUAL 2 doesn't quite match up with the message for when it fails. http-parser was not found or is too old; using bundled 3rd-party sources. since the failure could (in the future) be due to a version 3 being released?
But this was the case in the old version too, so maybe it doesn't matter.
Sorry, something went wrong.
There was a problem hiding this comment.
I'd be happy to bundle in a change to the error message as well if the maintainers think that's a good idea.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, I think that would be delightful. Something like http-parser version 2 not found... would be preferable.
Sorry, something went wrong.
|
I'm assuming the win64 failures are unrelated because they both fail during tests and correctly identify that there is no third-party http-parser library available, so their behavior should be unchanged with respect to this change. |
Sorry, something went wrong.
| IF (HTTP_PARSER_FOUND AND HTTP_PARSER_VERSION_MAJOR EQUAL 2) | ||
| INCLUDE_DIRECTORIES(${HTTP_PARSER_INCLUDE_DIRS}) | ||
| LINK_LIBRARIES(${HTTP_PARSER_LIBRARIES}) | ||
| LIST(APPEND LIBGIT2_PC_LIBS "-lhttp_parser") |
There was a problem hiding this comment.
does it link correctly under this name when the windows dll usually has the soname in it?
Sorry, something went wrong.
|
What's the goal here? Do you have a newer http-parser that you want to use? Independent of this, does it make sense to also upgrade the http-parser that we're bundling? |
Sorry, something went wrong.
|
Why do we use http-parser on Windows? If it's because of trying to parse out the URL, we should make it ask WinHTTP instead and just not use http-parser. We're certainly not trying to parse any HTTP with it. This is also not how you link to libraries on Windows, but if you expect to find something with pkg-config then I suppose you're not all that in Windows. |
Sorry, something went wrong.
We use libgit2 across a variety of platforms, and we like to make things as homogenous across platforms as possible. We recently started building http-parser as a shared library (for use with other software) and thought it was a shame to have the same code built and linked in twice.
I'm open to corrections. :) |
Sorry, something went wrong.
| IF (WIN32 AND WINHTTP) | ||
| ADD_DEFINITIONS(-DGIT_WINHTTP) | ||
|
|
||
| FIND_PACKAGE(HTTP_Parser) |
There was a problem hiding this comment.
It probably makes sense to move this block down toward the zlib block; these are quite similar, they both decide whether to load external dependencies, or to use the bundled third party libraries. Also, this looks like it adds some extra whitespace, which would be nice to ⚡️ in that move.
Sorry, something went wrong.
|
Addressed @ethomson's comments. |
Sorry, something went wrong.
|
Thanks @staticfloat , this looks good to me. Enjoy your external httpparser goodness! |
Sorry, something went wrong.
Currently, when building libgit2 on windows, if building against WinHTTP, there is no option to use an external http-parser library, we are locked into using the builtin http-parser library. I'd guess this was done because there were purportedly some differences between the third-party http-parser library on windows and the builtin http-parser library. However, I cannot find any special treatment of the windows build of the internal http-parser, so it stands to reason that we should be able to use the same external http-parser library on windows as we do on Linux or OSX.
This patch allows searching for http-parser libraries on any platform.