-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Crowcpp 0.2 Fork, support for FreeBSD #4085
Conversation
Failure in build 1 (
|
see #3064 |
Failure in build 2 (
|
Some configurations of 'crowcpp-crow/0.2' failed in build 3 (
|
All green in build 4 (
|
include_directories("${PROJECT_SOURCE_DIR}") | ||
|
||
-add_subdirectory(examples) | ||
+#add_subdirectory(examples) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read it correctly, then you are part of this project right?
Could you please consider adding a CMake option like BUILD_EXAMPLES
so that we don't need to patch it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not part of the project. The patch addresses an issue in the library that has been solved after the release tag v0.2. Until there's no new release of the library, I'm skipping the build of the examples, leaving only the tests active.
EDIT: This is the fix and it's in the master branch, not yet released: CrowCpp/Crow#63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix you're talking about is to be able to build examples in all environments.
But conan cci recipes prefer not to build examples at all.
To make it easy for the recipe, it would be great having this add_subdirectory(examples)
under if (BUILD_EXAMPLES)
and we would just BUILD_EXAMPLES
to False, instead of patching.
What I suggest:
- to make a PR in the crow project to add this variable (it may be ON by default, to not change the current behaviour)
- then ask the developer to release 0.2.1.
After that patch for examples will not be required at all.
It might be an overkill and/or too difficult, but it would make maintenance easier for the future versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I understand your point, which I totally share. I will try to do my best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've submitted the PR in the repository, the project maintainers plan to do a release soon. I will update the recipe once they'll release a new version!
include_directories("${PROJECT_SOURCE_DIR}") | ||
|
||
-add_subdirectory(examples) | ||
+#add_subdirectory(examples) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix you're talking about is to be able to build examples in all environments.
But conan cci recipes prefer not to build examples at all.
To make it easy for the recipe, it would be great having this add_subdirectory(examples)
under if (BUILD_EXAMPLES)
and we would just BUILD_EXAMPLES
to False, instead of patching.
What I suggest:
- to make a PR in the crow project to add this variable (it may be ON by default, to not change the current behaviour)
- then ask the developer to release 0.2.1.
After that patch for examples will not be required at all.
It might be an overkill and/or too difficult, but it would make maintenance easier for the future versions.
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -std=c++14 -pedantic -Wextra") | ||
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -std=c++11 -pedantic -Wextra") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to change the C++ version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen that no C++14 constructs are used, but C++11 is required by some boost header used in the project. Since the conan build system is using gcc 4.9, I've pushed it to C++11 instead of 14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to minimize the patches we apply, but this one follows the draft rules #3903 so okay!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the project https://github.com/CrowCpp/crow#attributions
There's a few missing external deps... I dont know where they come from though (not in the CMake)
The attributions are for libraries that are copied into the code, all the mentioned libraries' header files are inside |
Ahhh thank you for the clarification! I would discourage that since the consumer does not have any choice about which version is used... like the nodejs/http_parser is very common... I see it's wrapped with a the only blocker is my question #4085 (comment) I saw in my second pass it's a windows limitation for C++14 but that was a unix section... |
@@ -42,7 +42,7 @@ set(PROJECT_INCLUDE_DIR | ||
include_directories("${PROJECT_INCLUDE_DIR}") | ||
include_directories("${PROJECT_SOURCE_DIR}") | ||
|
||
-add_subdirectory(examples) | ||
+#add_subdirectory(examples) | ||
|
||
if (MSVC) | ||
else() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -42,7 +42,7 @@ set(PROJECT_INCLUDE_DIR | |
include_directories("${PROJECT_INCLUDE_DIR}") | |
include_directories("${PROJECT_SOURCE_DIR}") | |
-add_subdirectory(examples) | |
+#add_subdirectory(examples) | |
if (MSVC) | |
else() |
Not required if you only build amalgamation as target.
Co-authored-by: Uilian Ries <[email protected]>
All green in build 5 (
|
Now that you are building only the main target you should be able to remove the patches and openssl requirement you might need to explicitly copy ant not use the cmake.install in that case |
All green in build 7 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Extending OS recognition to FreeBSD Co-authored-by: Anonymous Maarten <[email protected]>
0a8b5bd
@ericLemanissier could you run freebsd build, please? |
All green in build 8 (
|
You need to add |
All green in build 9 (
|
it tries to invoke g++, even if the profile should use clang |
As far as I can tell, the problem is with boost, so I think this recipe is good to go. |
@mathbunnyru I remember FreeBSD switched to clang some time ago, that might be required to use |
The problem is with boost.build (b2), not boost itself. |
If my PR in the main project is approved, Boost and OpenSSL will be fully optional (only required by tests and examples), but the recipe for the "amalgamation" target will not require Boost and OpenSSL anymore. They will be required directly in the user's "conanfile.txt" or "conanfile.py" recipe in the project where crow is needed. |
Specify library name and version: crowcpp-crow/0.2
conan-center hook activated.