Skip to content
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 restinio package #503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add restinio package #503

wants to merge 1 commit into from

Conversation

ned14
Copy link

@ned14 ned14 commented Jan 4, 2022

  • I've followed this guide
    step by step carefully. Yes

Note that everything fails due to:

-- Configuring incomplete, errors occurred!
[hunter ** INTERNAL **] HUNTER_<name>_VERSION can't be empty
See also "D:/a/hunter/hunter/_testing/_builds/vs-16-2019-win64-sdk-10-0-18362-0-cxx17/CMakeFiles/CMakeOutput.log".
[hunter ** INTERNAL **] (foo)
[hunter ** INTERNAL **] (probably hunter_default_version/hunter_config call is missing)
[hunter ** INTERNAL **] [Directory:D:/a/hunter/hunter/_testing/Hunter/_Base/Download/Hunter/unknown/4f3536d/Unpacked/cmake/projects/foo]

Yet I am setting hunter_default_version, so I am officially stumped. Help would be appreciated.

Copy link

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

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

Please follow the guide on cmake packages with dependencies and hunterize restinio

https://hunter.readthedocs.io/en/latest/creating-new/create/cmake-dependencies.html

cmake/projects/restinio/hunter.cmake Outdated Show resolved Hide resolved
find_package(restinio CONFIG REQUIRED)
target_link_libraries(... restinio::restinio)

The reason you must manually bring in restinio's dependencies is because

Choose a reason for hiding this comment

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

dependencies are normally done by the hunterized package. The user of the package shouldn't need to specify dependencies by hand. Please hunterize restinio package instead following the documentation

https://hunter.readthedocs.io/en/latest/creating-new/create/cmake-dependencies.html

Copy link
Author

Choose a reason for hiding this comment

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

As my comment in the docs notes, restinio is an INTERFACE library. Therefore, its cmake dependencies get exported with the target.

restinio is fully hunterised, see https://github.com/ned14/restinio/blob/master/CMakeLists.txt. This isn't the problem here.

Copy link
Member

Choose a reason for hiding this comment

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

You can still add dependencies through the Config.cmake.in template methods that we generally use, no? https://github.com/cpp-pm/basis_universal/blob/hunter-1.15-c4c0db7/cmake/Config.cmake.in for example

find_package(http-parser CONFIG REQUIRED)

hunter_add_package(fmt)
find_package(fmt CONFIG REQUIRED)

Choose a reason for hiding this comment

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

see remark above about hunter dependencies, please hnunterize the restinio package

Copy link
Author

Choose a reason for hiding this comment

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

It is fully hunterised.

@@ -0,0 +1,21 @@
[

Choose a reason for hiding this comment

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

I don't know if this file is needed. I thought there is a default matrix.json and this override is only needed if you need to change toolchains

Copy link
Author

Choose a reason for hiding this comment

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

restinio is not compatible with VS2015, hence me commenting it out. If there is a better way to do this, I'd be glad to hear.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it should not be needed

@ned14
Copy link
Author

ned14 commented Jan 6, 2022

What's weird here is that all the Windows CI targets work just fine, this issue only affects the POSIX CI targets. Weird.

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.

3 participants