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

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).

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

@bagder , ready to merge ?

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

👎 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).

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.

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?

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 ? ).

@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.

@Lekensteyn , @bradking thanks.

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

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

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

@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

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

yes

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

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!

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?

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.

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?

Ah thanks.

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

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

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.

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...

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

Thanks for the baby sitting ;)

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

Hm, I hope I did it right: #1070