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 CrowConfig.cmake and related files #147

Closed
wants to merge 2 commits into from

Conversation

AmateurECE
Copy link

This patch allows downstream projects to use Crow simply by
including the following lines in their CMakeLists.txt:

find_package(Crow CONFIG REQUIRED)
target_link_libraries(myProject Crow::crow)

All transitive dependencies for the project are then managed
internally, not requiring downstream projects to change their
source when upgrading to newer versions of Crow.

NOTE: This patch is a little rough still, and I'd prefer to make a few more changes to the CMakeLists and supporting files (e.g. refactoring tests to use exported configuration--which would simplify the build scripts further). I would be happy to implement any feedback from the maintainers!

This patch allows downstream projects to use Crow simply by
including the following lines in their CMakeLists.txt:

    find_package(Crow CONFIG REQUIRED)
    target_link_libraries(myProject Crow::crow)

All transitive dependencies for the project are then managed
internally, not requiring downstream projects to change their
source when upgrading to newer versions of Crow.
@The-EDev
Copy link
Member

Hi.
First off thanks a lot for the work you've done here. And sorry for responding so late. I initially wasn't going to since you mentioned that the PR was incomplete.

Being able to install and use Crow without copying header files around is a great addition! If you need any help ironing out any problems please let me know. Until then, would it be appropriate if I convert this to a draft? (I'm not sure if github gives everyone the option to do so or only people with write access)

Another thing on my mind (not making a suggestion, just inquiring) is whether or not it's better to include the individual header files as opposed to crow_all.h, since IMO the single header file was originally designed for simplicity.

I also noticed that you're using 0.2 as a version, and we just released 0.3 so that should probably change.

Aside from that I'll need to look over the changes a few times (and look up what everything means 😅) to give any meaningful suggestions.

@AmateurECE
Copy link
Author

Hey, @The-EDev! Yes, feel free to convert this to a draft! I would have done that myself, but this is my first ever PR on Github and I wasn't aware of etiquette/existing features around that. Thanks for pointing out the version string, that one slipped past me. As for your comment about whether it's better to install all the headers or just crow_all.h, that's a very good point. Looking at the documentation, that would probably be more user-friendly. It may even make the CMake a little simpler. So, here's what I've got for action items to clean up the PR:

  1. Change version string from 0.2 to 0.3
  2. Install all headers, as opposed to crow_all.h.
  3. I'll attempt to refactor the examples/tests CMakeLists to use the exported config, so that dependencies only need to be added in one place.

While you're testing and looking over the changes, feel free to make other suggestions or changes!

@The-EDev The-EDev marked this pull request as draft June 14, 2021 12:48
@The-EDev The-EDev added this to the v0.4 (v1.0 possibly) milestone Jul 30, 2021
@The-EDev The-EDev mentioned this pull request Aug 18, 2021
@The-EDev The-EDev mentioned this pull request Aug 25, 2021
@The-EDev The-EDev removed this from the v0.4 (v1.0 possibly) milestone Aug 25, 2021
@The-EDev
Copy link
Member

The-EDev commented Sep 4, 2021

Closing this in favor of #209. Since The code used in that one was in part taken from here, you will still be credited for your work. Thank you again.

@The-EDev The-EDev closed this Sep 4, 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.

2 participants