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

rework cmake configs #209

Merged
merged 7 commits into from
Sep 4, 2021
Merged

Conversation

luca-schlecker
Copy link
Collaborator

@luca-schlecker luca-schlecker commented Aug 23, 2021

This PR partly fixes #160

It restructures the CMake system quite a bit.

It is adding:

  • FetchContent support
  • A proper CMake target
  • FindPackage support
  • Almost all examples on machines compiling with MSVC (+2 fixes)
  • Submodule support
  • Unittest now compiles without compression support. (Instead gives a status message)
  • Optional installation step

It is removing:

  • The need for #define CROW_ENABLE_SSL and #define CROW_ENABLE_COMPRESSION (Except if the amalgam is used)

Left to do:

  • Get the documentation up to date

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@The-EDev The-EDev added the feature Code based project improvement label Aug 24, 2021
@luca-schlecker luca-schlecker changed the title rework cmake configs [WIP] rework cmake configs Aug 24, 2021
@The-EDev
Copy link
Member

The-EDev commented Aug 25, 2021

Now that the checks are passing I think this PR might be ready to merge. On a side note, I mean no offense with the next part, but you probably know this PR clashes with #147, so what I was wondering is if there's anything that PR has or does that this one doesn't. I'm asking because I'm not that experienced with CMake.

@luca-schlecker
Copy link
Collaborator Author

This PR wasn't even supposed to be done at the moment.
But I see what you mean. Then I'll leave that part out because he already did it.
I'll change this PR out of draft state as soon as I get a last look at it and am happy with it. 👍

@The-EDev
Copy link
Member

The-EDev commented Aug 25, 2021

But I see what you mean. Then I'll leave that part out because he already did it.

That wasn't what I meant, You seemed to take a different approach to get FindPackage working, and I was wondering which way was better.. Sorry for the misunderstanding

@luca-schlecker
Copy link
Collaborator Author

luca-schlecker commented Aug 25, 2021

I have no FindPackage code in this PR. Mine would have looked almost like his, from what I've seen.

@The-EDev
Copy link
Member

Hmmm, I understand, thanks for clarifying

@The-EDev The-EDev mentioned this pull request Aug 25, 2021
CMakeLists.txt Outdated Show resolved Hide resolved
@The-EDev
Copy link
Member

CMake Error at /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the
system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY
OPENSSL_INCLUDE_DIR)

@luca-schlecker luca-schlecker changed the title [WIP] rework cmake configs rework cmake configs Aug 29, 2021
@luca-schlecker luca-schlecker marked this pull request as ready for review August 29, 2021 16:37
@luca-schlecker luca-schlecker marked this pull request as draft August 29, 2021 17:08
@luca-schlecker luca-schlecker marked this pull request as ready for review August 29, 2021 18:02
@The-EDev
Copy link
Member

The-EDev commented Aug 31, 2021

Coverage here is incorrect, it's being pulled from when some tests were disabled (I believe), the newer tests give a 500 error, I was informed by Coveralls that this is due to some downtime they've been having and should be fixed in time.

@The-EDev
Copy link
Member

The-EDev commented Sep 1, 2021

okay for some reason coverage went down to 66%...

@luca-schlecker
Copy link
Collaborator Author

luca-schlecker commented Sep 1, 2021

I have a hunch where it might be coming from.

diff --git a/.travis.yml b/.travis.yml
index eb59f5b7c8..7c49755b7b 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -33,6 +33,7 @@ addons:
       - mkdocs
       - graphviz
       - zlib1g-dev
+      - libssl-dev
 
 before_install:
   - if [ "$TRAVIS_COMPILER" == "gcc" -a "$TRAVIS_CPU_ARCH" == "amd64" ]; then export PUSH_COVERAGE=ON; fi
@@ -45,7 +46,7 @@ before_script:
   - mkdir build
   - cd build
   - cmake --version
-  - cmake ..
+  - cmake .. -DCROW_ENABLE_COMPRESSION=ON -DCROW_ENABLE_SSL=ON

I've enabled SSL in the CI build, leading to CROW_ENABLE_SSL being defined, and thus SSL code is enabled. Maybe Coverall didn't care before because the code was disabled by:

#ifdef CROW_ENABLE_SSL
...
#endif

Now though, it is enabled but no tests exist for the now accessible code.

@The-EDev
Copy link
Member

The-EDev commented Sep 1, 2021

hmmm, we could undefine CROW_ENABLE_SSL at the unittest, or get #130 working. what do you suggest?

@luca-schlecker
Copy link
Collaborator Author

I think getting some SSL tests up and running is the proper way to up the coverage again.

@The-EDev
Copy link
Member

The-EDev commented Sep 4, 2021

I'll merge this into a separate branch, Add documentation and SSL tests, and merge everything into master. Thanks again for all your help.

@The-EDev The-EDev changed the base branch from master to revamped_setup September 4, 2021 13:25
@The-EDev The-EDev merged commit 300e5bf into CrowCpp:revamped_setup Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Code based project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installing Crow
2 participants