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

Installing Crow #160

Closed
The-EDev opened this issue Jun 29, 2021 · 21 comments · Fixed by #209 or #228
Closed

Installing Crow #160

The-EDev opened this issue Jun 29, 2021 · 21 comments · Fixed by #209 or #228
Assignees
Labels
task Non code related issue
Milestone

Comments

@The-EDev
Copy link
Member

With the work on vcpkg and different Linux package managers, Crow can now be installed rather than just included in the downstream project.

The idea has often been to just use crow_all.h which probably works for small projects, but I'm not sure about larger ones, since a larger project might want to use certain components in one source file and not others. (I might be wrong here and a single header is all that is actually needed, please correct me if so)

So the first question is: Should crow by default be installed as a set of header files or just the single header file?

This will shape how the documentation and releases are made moving forward.

@The-EDev The-EDev added the task Non code related issue label Jun 29, 2021
@The-EDev The-EDev added this to the v0.4 (v1.0 possibly) milestone Jul 2, 2021
@beached
Copy link
Contributor

beached commented Jul 20, 2021

I think that there is merit to multiple headers, but it is difficult right now. As is, getting updates is a non-automated process in many projects. Making Crow work with FetchContent would help a lot too.

@The-EDev
Copy link
Member Author

well the idea behind this issue is whether single or multi header, the user either manually or automatically (through a package manager) installs crow on their system and just references it without having a to download the headers for each project.

@luca-schlecker
Copy link
Collaborator

I guess having multiple headers should be no problem. Vcpkg doesn't use crow_all.h and that's fine because with a package manager, the user doesn't really care about easy distribution.

Maybe the amalgamation should be made accessible via an option. nlohmann/json is doing a similar thing.

I could look into providing FetchContent support and a non-amalgamated target.
What do you think, @The-EDev?

@The-EDev
Copy link
Member Author

The-EDev commented Aug 18, 2021

@luca-schlecker Sorry for the late response, I was doing some research. I think a non-amalgamated target for install is a really good idea. FetchContent isn't a bad addition either.

So by the time this issue closes, we should have the following:

@luca-schlecker
Copy link
Collaborator

luca-schlecker commented Aug 18, 2021

Exactly what I've though. 👍 I'll have a look at the first three points. 🚀

@The-EDev
Copy link
Member Author

I appreciate the help :)

@luca-schlecker
Copy link
Collaborator

I guess installing the amalgamated crow_all.h has no use after having a non-amalgamated crow target as crow_all.h would only be used to manually copy it inside the consuming project. Do you agree on omitting the install-step for crow_all.h, @The-EDev?

@The-EDev
Copy link
Member Author

That's sort of what I had in mind, any install, through PM or CMake would install all headers, and keep crow_all for manual installation (small projects and such)

@The-EDev The-EDev assigned luca-schlecker and unassigned mrozigor Aug 20, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 23, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 23, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 23, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 24, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 24, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 24, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 24, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 25, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 26, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 29, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 30, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 30, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Aug 31, 2021
luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Sep 1, 2021
@Leon0402
Copy link
Contributor

Leon0402 commented Sep 9, 2021

To make this support even more Fetch Content ready, could sensible default values for the configuration be used? In particular tests and stuff disabled by default?

This doesn't mean that Tests have to be OFF by default, one can just build a protection, so that test get disabled if it's not the top level source project:
https://github.com/CrowCpp/Crow/blob/revamped_setup/CMakeLists.txt#L83

Something like if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND CROW_BUILD_TESTS)

Furthermore, I would also suggest using BUILD_TESTING instead of CROW_BUILD_TESTS. First is similar to BUILD_SHARED_LIBS a standard name for tests.
It's actually set automatically if you use include(CTest) (enable_testing is also set automatically)

@luca-schlecker
Copy link
Collaborator

luca-schlecker commented Sep 9, 2021

That's a good suggestion.
I will spin up a PR implementing the behavior you talked about.
I guess comparing CMAKE_CURRENT_SOURCE_DIR to CMAKE_SOURCE_DIR also works for this purpose.

I'd like to keep CROW_BUILD_TESTS though, but that's not up to me.

@The-EDev
Copy link
Member Author

Since CMake generates find_package data now, it may be wise to include a compiled archive (include directory + CMake files) with every release, this way a package manager doesn't have to use CMake to generate that data, something like that would be very easy to include in the release script.

@luca-schlecker
Copy link
Collaborator

Well, to properly consume the compiled archive (include directory + CMake files), one would have to have CMake installed anyway. One could ignore the CMake files and only use the header files, but I guess this isn't the priority. CMake is widely used (JetBrains 2021 C++ Ecosystem Survey) and I think it is most likely already installed on most systems.

@The-EDev
Copy link
Member Author

I understand, my concern is someone installing Crow through AUR or manually then later on wanting to use CMake, specifically for the AUR install, they would need to uninstall and reinstall the package, which is less than ideal.

@luca-schlecker
Copy link
Collaborator

I am sorry, I don't quite understand. Could you please elaborate?

@The-EDev
Copy link
Member Author

The way AUR (or pacman) works is that it downloads a package from source then runs 1 or more shell scripts, since the cmake files (Config and Targets) need to be generated using CMake, one would need to have CMake installed in order to get these files. The simple solution is to only attempt to use CMake if it is already installed on the system, but this means that a user installing Crow before CMake would not have these files. And would only have them if they uninstalled and reinstalled the package.

Providing a package with the CMake files fixes this problem by not relying on CMake at all during installation, instead just copying the files to where they need to be (usr/local/include and usr/local/lib). That way the CMake files will be available regardless.

@The-EDev
Copy link
Member Author

Update on this matter, I added the tar.gz package generation to the release script, used it as the basis for a manual md5 and sha256 checksum calculation for AUR, made AUR more robust by using a PKGBUILD file inside the scripts directory, and added a command to update AUR's SRCINFO (making it so that the user only needs to git commit the changes to update Crow on AUR)

@The-EDev
Copy link
Member Author

The-EDev commented Sep 27, 2021

@luca-schlecker adding -DCROW_AMALGAMATE to cmake gives a compiler error where example_with_all can't find crow_all.h, I checked the cmake files (main one and examples one) and everything seems to be in order.. any ideas?

Update: I got it working by adding include_directories(${CMAKE_CURRENT_BINARY_DIR}) below the custom command for amalgamate, but I'm guessing this isn't the right approach..

Update 2: I also needed to add target_link_libraries(example_with_all PUBLIC Boost::boost Boost::system Boost::date_time Threads::Threads) to the CMakeLists.txt in examples in order to get it compiling.

@luca-schlecker
Copy link
Collaborator

I had a short look and I know where the problem is coming from. I'll fix it in a PR hopefully this afternoon/evening. 👍

@luca-schlecker
Copy link
Collaborator

@The-EDev The fix is pretty simple.
It had the wrong include directory added and it also needs the libraries you've already mentioned.
I think it would be easiest to just apply this patch:

diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
index 643e2f3e..e7bc0e61 100644
--- a/examples/CMakeLists.txt
+++ b/examples/CMakeLists.txt
@@ -49,7 +49,8 @@ if(CROW_AMALGAMATE)
   add_executable(example_with_all example_with_all.cpp)
   add_dependencies(example_with_all crow_amalgamated)
   add_warnings_optimizations(example_with_all)
-  target_include_directories(example_with_all PUBLIC ${CMAKE_CURRENT_BINARY_DIR})
+  target_link_libraries(example_with_all PUBLIC Crow::Crow)
+  target_include_directories(example_with_all PUBLIC ${CMAKE_BINARY_DIR})
 endif()
 
 add_executable(example_chat example_chat.cpp)

Linking against Crow would automatically lead to the right libraries also being linked to.

@The-EDev
Copy link
Member Author

The-EDev commented Sep 28, 2021

Solution works, Thanks a lot! I'll merge ssl_test then commit your changes. This branch is getting a little too big with SSL, docs, and now the coveralls issue. I think I won't work on docs beyond the setup ones, I'll leave the extra tutorials for another PR.

@luca-schlecker
Copy link
Collaborator

Seems like a good idea. 👍

luca-schlecker added a commit to luca-schlecker/crow that referenced this issue Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Non code related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants