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

Improving CMakeLists.txt #96

Merged
merged 8 commits into from
Jan 14, 2021
Merged

Improving CMakeLists.txt #96

merged 8 commits into from
Jan 14, 2021

Conversation

madduci
Copy link
Contributor

@madduci madduci commented Jan 6, 2021

Hello

as suggested by Conan team, I've made some changes to the CMakeLists to make it better supported by conan recipes (but also by other package managers as well), using only what's needed and enabling/disabling tests and examples.

Here's a short list of what I'm trying to change upstream:

  • Added options BUILD_EXAMPLES and BUILD_TESTING to switch examples on/off during compilation: examples are not built by default, Tests are
  • Using standard CMake variable CMAKE_CXX_STANDARD to set the C++ Standard, independently from the compiler
  • Using OpenSSL only if BUILD_EXAMPLES are activated, since it's required by Boost ASIO only at runtime as dynamic library

The Conan recipe is going under review here: conan-io/conan-center-index#4085

If this change is merged in the master branch, I'd kindly ask you to make a new Release tag, so it can be used in the Conan recipe as well.

- Added options BUILD_EXAMPLES and BUILD_TESTING to switch examples on/off during compilationusing standard CMake variable to set the C++ Standard, independently from the compiler
- Using OpenSSL only if BUILD_EXAMPLES are activated, since it's required by Boost ASIO only at runtime as dynamic library
@The-EDev
Copy link
Member

The-EDev commented Jan 6, 2021

Hi, Thanks a lot for the help.

Regarding the C++ version, while crow does support C++11 at the moment (by altering some functionality), the plan was to use C++14 features if needed, reading conan-io/conan-center-index#4085 (comment) I understand the reason. @mrozigor would this change cause problems?

As for the release tag, the plan is to finish #84 and #93 and release v0.3, by Monday at the absolute latest. Is that okay? or should we create an intermediate release and then another once the PRs are merged?

vexy
vexy previously approved these changes Jan 6, 2021
Copy link

@vexy vexy left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@madduci
Copy link
Contributor Author

madduci commented Jan 6, 2021

Hi @The-EDev
I'll wait then for the release 0.3 once you're done, please send me a ping when it's there, so I can update the conan recipe and point to the version.

Copy link

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

CMakeLists.txt Outdated Show resolved Hide resolved
@madduci
Copy link
Contributor Author

madduci commented Jan 6, 2021

Sure, but I didn't want to do massive changes all at once.

@mrozigor
Copy link
Member

mrozigor commented Jan 8, 2021

Regarding the C++ version, while crow does support C++11 at the moment (by altering some functionality), the plan was to use C++14 features if needed, reading conan-io/conan-center-index#4085 (comment) I understand the reason. @mrozigor would this change cause problems?

Probably not until we use something 14 dependent ;)

@mrozigor
Copy link
Member

mrozigor commented Jan 8, 2021

Sure, but I didn't want to do massive changes all at once.

I think it a simple change (removing that one line). If you improve CMakeLists.txt, then go for it ;)

@madduci
Copy link
Contributor Author

madduci commented Jan 8, 2021

Ok then I would do a couple of improvements

@madduci
Copy link
Contributor Author

madduci commented Jan 11, 2021

@mrozigor @The-EDev I've upgraded the CMake, now Examples and Tests are enabled by default again, but they can be turned off (cmake .. -DBUILD_EXAMPLES=OFF -DBUILD_TESTING=OFF), not requiring either Boost or OpenSSL, to generate only the amalgamate version of crow.

I've added the equivalent of -Wall for MSVC (/W4) and some optimization flags, for tests and examples.

The compiler flags and the dependencies are now grouped in two separated files, to have a better overview of what is required.

Tell me if the PR fits your project, feel free to give me the feedbacks on that and how can I improve it.

CMakeLists.txt Outdated Show resolved Hide resolved
cmake/compiler_options.cmake Outdated Show resolved Hide resolved
cmake/dependencies.cmake Outdated Show resolved Hide resolved
cmake/dependencies.cmake Outdated Show resolved Hide resolved
examples/CMakeLists.txt Show resolved Hide resolved
@madduci
Copy link
Contributor Author

madduci commented Jan 12, 2021

Hi @mrozigor thanks for the review. I've bumped Boost to version 1.64.0 since on some systems the latest versions could not be available, what do you think?

@mrozigor
Copy link
Member

Hi @mrozigor thanks for the review. I've bumped Boost to version 1.64.0 since on some systems the latest versions could not be available, what do you think?

Ok, I think that's reasonable ;) Good work.

example_ssl won't be built if OpenSSL is not found: a status message will be shown instead
@mrozigor mrozigor merged commit d347c09 into CrowCpp:master Jan 14, 2021
@The-EDev
Copy link
Member

The-EDev commented Jan 15, 2021

Hi @mrozigor thanks for the review. I've bumped Boost to version 1.64.0 since on some systems the latest versions could not be available, what do you think?

I know I'm very late to the party, but this could be a problem, we discussed it in #29, this version of crow (as far as i know) does not work with boost<1.70

@madduci
Copy link
Contributor Author

madduci commented Jan 15, 2021

@The-EDev could you please try to reproduce the #29? If the bug is confirmed, then the Boost version can be bumped to 1.70.0

@The-EDev The-EDev mentioned this pull request Aug 24, 2021
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.

5 participants