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 cmake nghttp2 support #922

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@remoe
Contributor

remoe commented Jul 21, 2016

No description provided.

@mention-bot

This comment has been minimized.

mention-bot commented Jul 21, 2016

@remoe, thanks for your PR! By analyzing the annotation information on this pull request, we identified @Sukender, @jzakrzewski and @billhoffman to be potential reviewers

@bagder bagder added the cmake label Jul 21, 2016

find_path(NGHTTP2_INCLUDE_DIR "nghttp2/nghttp2.h"
HINTS ${NGHTTP2_DIR}/include ${NGHTTP2_DIR}/inc)
find_library(NGHTTP2_LIBRARY_RELEASE NAMES NGHTTP2LIB

This comment has been minimized.

@Lekensteyn

Lekensteyn Aug 10, 2016

Member

Name should be lowercase to work on case-sensitive filesystems (Linux).

@bradking is this appropriate use of select_library_configurations in a FindXXX.cmake file?

This comment has been minimized.

@bradking

bradking Aug 10, 2016

Contributor

Yes.

@@ -322,6 +322,14 @@ if(CMAKE_USE_OPENSSL)
endif()
endif()
if(USE_NGHTTP2)

This comment has been minimized.

@Lekensteyn

Lekensteyn Aug 10, 2016

Member

Missing option(USE_NGHTTP2 ...)

@bradking

This comment has been minimized.

Contributor

bradking commented Aug 10, 2016

Please see cmake-developer(7) for documentation of find module variable names. The NGHTTP2_INCLUDE_DIR value should not be used by clients directly. It is only a cache entry to hold the location of a specific include directory. Instead a NGHTTP2_INCLUDE_DIRS variable should provide the result (in case additional directories are needed later).

@bagder

This comment has been minimized.

Member

bagder commented Sep 4, 2016

@remoe, can we hope for an update to this in line with @bradking's comments?

@remoe

This comment has been minimized.

Contributor

remoe commented Sep 4, 2016

@bagder , ready to merge ?

@bagder

This comment has been minimized.

Member

bagder commented Sep 4, 2016

I'll await a 👍 or two from some of the more cmake oriented people first.

@Lekensteyn

This comment has been minimized.

Member

Lekensteyn commented Sep 4, 2016

👎 FindNGHTTP2.cmake is not good:

  • The release/debug libs are looked up in the same dir. Why bother then with calling select_library_configurations?
  • The nghttp2 names are non-standard.

nghttp2 (in lib/Makefile.msvc) uses the name nghttp2.lib. On Linux the library is called libnghttp2.so. Then why not replace the two find_library and single select_library_configurations by:

find_library(NGHTTP2_LIBRARY NAMES nghttp2)

Note the lower-case nghttp2. NGHTTP2_DIR is not needed if you set CMAKE_LIBRARY_PATH appropriately. (Similarly for the find_path, the NGHTTP2_DIR can be removed in favor of setting CMAKE_INCLUDE_PATH).

@remoe

This comment has been minimized.

Contributor

remoe commented Sep 4, 2016

I've tested it on windows. I prefer not to use global settings like CMAKE_* for paths on windows. About the names: you're right, it could this doesn't find the library on linux.

@Lekensteyn

This comment has been minimized.

Member

Lekensteyn commented Sep 4, 2016

How did you build nghttp2 on Windows? Using its cmake system, the lib/Makefile.msvc or something else?

CMAKE_LIBRARY_PATH and friends are supposed to be set by the user (e.g. using cmake-gui), are you avoiding them because you find it more convenient to set a single NGHTTP2_DIR variable?

@remoe

This comment has been minimized.

Contributor

remoe commented Sep 5, 2016

I've build nghttp2 1.12.0 with CMake on Windows.
CMake has pre and post suffixes for library names, so it could work without any changes on linux ( @bradking ? ).

@bradking

This comment has been minimized.

Contributor

bradking commented Sep 5, 2016

@Lekensteyn's comments still need to be addressed:

  • The find_library call should specify the library file name in lower case.
  • The use of select_library_configurations is not necessary unless we expect the library file to have different names per-configuration.
  • CMake currently discourages use of per-package root variables as NGHTTP2_DIR is used here. Instead one can set CMAKE_PREFIX_PATH to get both find_library and find_path to look in appropriate locations (and it can be a list to work for multiple packages). If one does use a per-package root variable then the name should be something other than NGHTTP2_DIR because that would be reserved for use if upstream every distributes a CMake package configuration file.
@remoe

This comment has been minimized.

Contributor

remoe commented Sep 5, 2016

find_path(NGHTTP2_INCLUDE_DIR "nghttp2/nghttp2.h")
find_library(NGHTTP2_LIBRARY NAMES nghttp2 libnghttp2)

This comment has been minimized.

@bradking

bradking Sep 5, 2016

Contributor

CMake does automatically add a lib prefix when searching on platforms that use it. This can be just

find_library(NGHTTP2_LIBRARY NAMES nghttp2)
find_package_handle_standard_args(NGHTTP2
FOUND_VAR
NGHTTP2_FOUND

This comment has been minimized.

@Lekensteyn

Lekensteyn Sep 6, 2016

Member

FOUND_VAR NGHTTP2_FOUND is the default so you could omit that

@Lekensteyn

This comment has been minimized.

Member

Lekensteyn commented Sep 6, 2016

Change looks good, I'll give this a test on Linux.

Oh, and please use Unix line endings in FindNGHTTP2.cmake

@Lekensteyn

This comment has been minimized.

Member

Lekensteyn commented Sep 6, 2016

This fails to build if nghttp2 is not installed. Maybe use find_package(NGHTTP2 REQUIRED) or rename the option?

@remoe

This comment has been minimized.

Contributor

remoe commented Oct 10, 2016

@Lekensteyn @bagder I've tested it now with USE_NGHTTP2 ON/OFF.

Windows (win64 VS2015) curl binary version dump with OFF:

curl 7.51.0-DEV (Windows) libcurl/7.51.0-DEV OpenSSL/1.0.2h zlib/1.2.8
Protocols: http https smb smbs
Features: AsynchDNS IPv6 Largefile NTLM SSL libz

Version dump with ON:

curl 7.51.0-DEV (Windows) libcurl/7.51.0-DEV OpenSSL/1.0.2h zlib/1.2.8 nghttp2/1.12.0
Protocols: http https smb smbs
Features: AsynchDNS IPv6 Largefile NTLM SSL libz HTTP2
@bagder

This comment has been minimized.

Member

bagder commented Oct 10, 2016

And does it now also work fine to build even if nghttp2 is not installed/found? @Lekensteyn remarked on that on Sep 6.

@remoe

This comment has been minimized.

Contributor

remoe commented Oct 10, 2016

yes

@Lekensteyn

The current patch looks good for merging.

option(USE_NGHTTP2 "Use Nghttp2 library" OFF)
if(USE_NGHTTP2)
find_package(NGHTTP2 REQUIRED)
if(NGHTTP2_FOUND)

This comment has been minimized.

@Lekensteyn

Lekensteyn Oct 10, 2016

Member

Note that this is strictly not necessary since you have REQUIRED for find_package.

@bagder

This comment has been minimized.

Member

bagder commented Oct 10, 2016

Thanks a lot @remoe for your hard work on this. Squashed and merged now!

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 10, 2016

I tested this PR, works like a charm.

However, of the latest stable release, the provided source code zip file is incomplete. It does not contain all CMake Modules required and available in git. Seems to be a simple packaging error..
After adding the modules from this repo, it works like a charm.

Thanks for the good job!

@jay

This comment has been minimized.

Member

jay commented Oct 10, 2016

However, of the latest stable release, the provided source code zip file is incomplete. It does not contain all CMake Modules required and available in git. Seems to be a simple packaging error..

@TDannhauer What's missing? I see FindCARES.cmake and FindLibSSH2.cmake are not in the release and should be added to CMAKE_DIST. Is there anything else? Do you want to take a shot at a PR?

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 10, 2016

The package as defined in CMAKE_DIST contains

CMake/CMakeConfigurableFile.in
CMake/CurlTests.c
CMake/FindGSS.cmake
CMake/OtherTests.cmake
CMake/Platforms/WindowsCache.cmake
CMake/Utilities.cmake
include/curl/curlbuild.h.cmake
CMake/Macros.cmake
CMake/CurlSymbolHiding.cmake

However, the package did not contain:
CMake/CurlSymbolHiding.cmake
--> I don't know what's wrong there.

Additionally we have two missing files:

CMake/FindCARES.cmake
CMake/FindLibSSH2.cmake

Finally, we need also the brand new CMake/FindNGHTTP2.cmake, which was added with the PR of this thread.

@jay

This comment has been minimized.

Member

jay commented Oct 10, 2016

CurlSymbolHiding issue was fixed in 82279c8 which was after 7.50.3 was released. The three missing files I agree. Do you want to open a PR with those changes?

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 10, 2016

Ah thanks.

I will try, I'm not used to the workflow, so it will be a fight against gihub, give me some minutes. :)

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 10, 2016

Do I need to make the PR in master branch oder another branch? sorry, I'm not used to it....

@bagder

This comment has been minimized.

Member

bagder commented Oct 10, 2016

The easiest way is if you make a new branch dedicated for that fix only and you make a PR from that. That way you can update that individually if needed and you can delete it once the PR gets merged.

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 10, 2016

Hm thanks.. Way back to the basics: What should I use? GitHub desktop, or Web interface?
I tried Github desktop:

  1. cloned github repo
  2. Create my branch
  3. Edited Makefile.am
  4. Commited changes to my branch
  5. Tried to create a PR from my branch to master -> failed: "Sync failed to push local changes" (Missing Permission)

What might be the mistake?

Hmpf. the Github doc is very confusing for SVN guys without git experience...

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 10, 2016

Is this repos a shared model approach, or do I need to fork?

Thanks for the baby sitting ;)

@jay

This comment has been minimized.

Member

jay commented Oct 10, 2016

Basically when you are working in git branches are cheap so you can just create a bunch of them. Your failure message may be because you are attempting to push directly upstream (here) and not to your origin (your fork of curl). The way you submit changes is push them to a branch in your fork and then when you go to your curl fork in github you will see the changes you have just pushed with a button "create pull request".

So first you need to go to the top of our project page and click the fork button and that will put a fork of the curl repo on your account, and then in the local repo:

# The canonical repo (us) should be 'upstream'
git remote rename origin upstream
git branch --set-upstream-to=upstream/master master

# Your fork should be 'origin'
git remote add -f origin git@github.com:TDannhauer/curl.git
git config branch.master.pushremote origin

# Ensure you're up-to-date
git checkout master
git fetch upstream && git merge --ff-only upstream/master

# Advice: don't work on master, start a new branch off of it
git checkout -b fix_cmake_dist

# Read https://github.com/curl/curl/blob/master/docs/CONTRIBUTE.md
# commit your changes

# push branch upstream
git push --set-upstream origin fix_cmake_dist

What gets kind of confusing is though 'upstream' is a typical name for the canonical repo you set a branch "upstream" meaning where you want it to land, which in this case is your fork 'origin'.

Also read https://github.com/curl/curl/blob/master/docs/CONTRIBUTE.md

@TDannhauer

This comment has been minimized.

Contributor

TDannhauer commented Oct 11, 2016

Hm, I hope I did it right: #1070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment