Skip to content

Bringing Cmake back #470

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

Merged
merged 4 commits into from
May 19, 2020
Merged

Bringing Cmake back #470

merged 4 commits into from
May 19, 2020

Conversation

sum01
Copy link
Contributor

@sum01 sum01 commented May 10, 2020

I find having Cmake support very useful, so I reverted the deletion, and fixed the issue someone had with it in #421

Note: This PR namespaces the Cmake target, so it's now httplib::httplib

I also took the time to improve it in a couple ways.

  • Added some Cmake options to allow for strict or relaxed dependency requirements.[1]
  • Added in a way for it to properly find its (optional) dependencies for the user when find_package(httplib) is called so they don't have to remember to do that.
  • Added a way for users to require a version in the find_pacakge call.[2]
  • Fixed the wrong install path that was causing issue using httplib cmake target #421
  • Changed the install path to allow for end-users to change it as desired.[3]
  • Added an alias target to httplib::httplib so people building in-tree can link against the same target as users who installed.
  • Added additional compiler feature detection. This is pretty minor, just might catch a few rare failed cmake builds on old compiler versions.
  • Added some documentation on all this to the README

[1]. Specifically the following:

  • HTTPLIB_REQUIRE_OPENSSL (default off)
  • HTTPLIB_REQUIRE_ZLIB (default off)
  • HTTPLIB_USE_OPENSSL_IF_AVAILABLE (default on)
  • HTTPLIB_USE_ZLIB_IF_AVAILABLE (default on)

[2]. I was unsure if you follow SemVer or not, so I made the versioning allow anything >= request version.

[3]. See GNUInstallDirs, specifically CMAKE_INSTALL_INCLUDEDIR

@sum01
Copy link
Contributor Author

sum01 commented May 18, 2020

Anything you want me to improve upon with this PR?

I think it doesn't really have any more to add to it, but I'm open to suggestions.

@yhirose
Copy link
Owner

yhirose commented May 19, 2020

@sum01, thank you very much for spending time for CMake support. I don't come up with any suggestion since I personally don't know much about CMake. But it looks good and complete to me.

I would like to ask you to handle the following two things before merge the PR:

  1. Remove the version description (VERSION 0.6.3) from CMakeLIsts.txt
  2. Revert the change in README

The reason why I am asking these things is to reduce necessary maintenance work as much as possible because I don't personally use it and I don't have time to support users about CMake. (That's why I removed the previous CMake support.)

When it comes to README, I would like to put only information about features and examples in README. (I have actually received similar pull requests such as #416, #408, #195. But I didn't accepted any of them with the same reason.)

I appreciate your understanding and thank you very much for your great job!

@sum01
Copy link
Contributor Author

sum01 commented May 19, 2020

I'm a little bummed that you don't want the version string, as it allows specific version requirements in a find_package call, but very well.

As for the README, would it make sense to put that info somewhere else?
It seems odd to leave no info at all.

EDIT: Never mind, I'll just put the info into the CMakeLists.txt

sum01 added 4 commits May 19, 2020 13:11
Previous behaviour is just a warning.
Adds automatic dependency finding (if they were used).
Adds a way to require a specific version in the find_package(httplib) call.

You should link against the httplib::httplib IMPORTED target, which is
created automatically.

Add options to allow for strictly requiring OpenSSL/ZLIB

HTTPLIB_REQUIRE_OPENSSL & HTTPLIB_REQUIRE_ZLIB require the libs be found, or the build fails.

HTTPLIB_USE_OPENSSL_IF_AVAILABLE & HTTPLIB_USE_ZLIB_IF_AVAILABLE silently search for the libs.
If they aren't found, the build still continues, but shuts off support for those features.
Has info on all the available options and what targets are produced.

Also put some things about installation on certain platforms.
@sum01 sum01 force-pushed the cmake_improvements branch from c7d18ff to 216862c Compare May 19, 2020 18:47
@sum01
Copy link
Contributor Author

sum01 commented May 19, 2020

Ok I removed the version file & README changes.

The only thing that might ever need changing is the OpenSSL minimum version string, which I doubt you'll be doing often anyways.

@yhirose yhirose merged commit 9505a76 into yhirose:master May 19, 2020
@yhirose
Copy link
Owner

yhirose commented May 19, 2020

@sum01, thank you for the adjustments even if you don't fully agree with.

The OpenSSL minimum version string (1.1.1) is good with me. Also, it's actually a good idea to include the installation guide in the CMakeLists.txt file itself, so that users don't need to look around in README to find the info.

Hope it helps many users who depend on cmake. Thank you so much for the fine contribution!

@sum01 sum01 deleted the cmake_improvements branch May 20, 2020 02:35
ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
* Revert "Removed CMakeLists.txt. (Fix yhirose#421)"

This reverts commit e59ffaa.

* Fail if cmake version too old

Previous behaviour is just a warning.

* Improve CMakeLists

Adds automatic dependency finding (if they were used).
Adds a way to require a specific version in the find_package(httplib) call.

You should link against the httplib::httplib IMPORTED target, which is
created automatically.

Add options to allow for strictly requiring OpenSSL/ZLIB

HTTPLIB_REQUIRE_OPENSSL & HTTPLIB_REQUIRE_ZLIB require the libs be found, or the build fails.

HTTPLIB_USE_OPENSSL_IF_AVAILABLE & HTTPLIB_USE_ZLIB_IF_AVAILABLE silently search for the libs.
If they aren't found, the build still continues, but shuts off support for those features.

* Add documentation to CMakeLists.txt

Has info on all the available options and what targets are produced.

Also put some things about installation on certain platforms.
SC-One pushed a commit to SC-One/cpp-httplib that referenced this pull request Jan 15, 2025
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.

2 participants