Make snowhouse a CMake library #38

HauserV · 2018-10-05T11:40:24Z

Adds CMake library target for snowhouse enabling importing the library to user CMake targets. Example follows.

shell$ git clone https://github.com/banditcpp/snowhouse.git

# CMakeLists.txt
add_subdirectory(snowhouse)
add_executable(app main.cpp)
target_link_libraries(app snowhouse)

Notes:

the header files were moved to include/ dictionary to enable including the headers only
building&running of tests is disabled by default to avoid cluttering users' target list

sbeyer

Thank you for your contribution.

If I get your point right, your changes make it easier for users to use Snowhouse as a library: although it is already easy because it's header only; user's shouldn't even care if it's header-only or not, they should just use target_link_libraries and all the magic is done... Is that the point?

[Monologue start] I think the CMake configuration was basically intended for the tests (thus the defaults) and not for potential Snowhouse users. I have never considered before to change this... But if it simplifies things for users that way, let's do it, and I'd also be okay with that default changes. On the other hand, I know that CMake offers installation magic to use with find_library()... wouldn't that even be easier for the user? I am not sure. One is for having a system-wide installation and the other is for shipping snowhouse with your code... Probably your change is a start and the installation stuff is a nice-to-have feature. Both ways could be used by users complementarily... [Monologue end]

Besides my review commits in the diff, your pull request is missing documentation changes. If it is now easy to use snowhouse as a library, the users should get to know it.

Edit: I am very sorry, I was only looking at the diff of the first commit, not at the diff of the whole pull request. I am very tired at the moment and should have deferred the comments.

sbeyer · 2018-10-05T18:52:54Z

include/snowhouse/fluent/operators/collections/collectionoperator.h

@@ -6,7 +6,7 @@
 #ifndef SNOWHOUSE_COLLECTIONOPERATOR_H
 #define SNOWHOUSE_COLLECTIONOPERATOR_H

-#include "../constraintoperator.h"
+#include "../../operators/constraintoperator.h"


Hu? What has happened here?

Looks like the refactoring tool had a slight hiccup there. Both of those paths are equivalent. Sorry.

CMakeLists.txt

sbeyer · 2018-10-05T18:58:55Z

CMakeLists.txt

@@ -55,3 +62,4 @@ if (SNOWHOUSE_BUILD_TESTS AND SNOWHOUSE_RUN_TESTS)
 elseif (SNOWHOUSE_RUN_TESTS)
    message(WARNING "Unable to run snowhouse tests - set:\n  option(SNOWHOUSE_BUILD_TESTS, \"Build the Snowhouse tests\" ON)\nand clear your CMakeCache.txt")
 endif()
+


Please don't.

I am not completely sure what am I supposed not to do.

Adding an empty line :-)

CMakeLists.txt

README.md

sbeyer · 2018-10-05T19:50:28Z

By the way I'm wondering if the move of the files is a breaking change (because existing build configurations (i.e., include paths) must be changed)... That would mean Snowhouse has to advance to version 4.0.0. This is funny because it rather feels like a minor change leading to 3.1.0... What do you think? ;-)

HauserV · 2018-10-05T20:18:17Z

Sorry for the late replay, you've caught me on the way home from work...

It depends on the way people actually include the library... I think that there is a way how to avoid bumping the version: since all paths in the #includes are relative to the snowhouse folder (now include/snowhouse) we could simply add a symlink pointing to that directory.

HauserV · 2018-10-05T20:39:31Z

To answer the first set of questions. Yep, making it easier to use the library in CMake projects was exactly my goal. I had to do these changes manually to make the library work with my current build setup seamlessly and so I thought that others might benefit from it as well. Find_library seems a little bit too complex because you have to deal with the paths to the library and since there is no make install or similar step, it might be quite difficult to write one that would be universal. Investing time into writing proper exported targets and CMake install steps would be more useful (but I am not sure whether it's actually worth it).

sbeyer · 2018-10-05T21:07:19Z

To answer the first set of questions. Yep, making it easier to use the library in CMake projects was exactly my goal. I had to do these changes manually to make the library work with my current build setup seamlessly and so I thought that others might benefit from it as well.

The way I had thought about it before is that you do not add it to the CMake setup at all, you simply use the headers-only branch version as a submodule in the right location and hence you don't need any additional include paths.

However, I can imagine that this is not the best choice for everyone. So it's nice to have this alternative.

You wrote on the breaking change issue:

It depends on the way people actually include the library... I think that there is a way how to avoid bumping the version: since all paths in the #includes are relative to the snowhouse folder (now include/snowhouse) we could simply add a symlink pointing to that directory.

That's a solution I have also considered, however, a symlink is not portable (please remove it again). I'd rather bump the version. (Btw you don't have to make any changes regarding version or anything; that's my task. I just wanted an opinion.)

You wrote on exported targets and find_library:

(but I am not sure whether it's actually worth it)

Yes, I also doubt that.

I once did that for another project, a more complex library, and it seems there is a lot that you can miss and do wrong... so let's keep it simple.

sbeyer · 2018-10-05T21:13:09Z

a symlink is not portable (please remove it again)

While at it, it would be nice if you do a git rebase -i master your changes and squash (fixup) the "Fix odd #include, remove dangling newline" commit into the first commit. And while at it, please also reword the "set RUN_TESTS[…]" commit title to "Run tests on Travis CI again". Both these changes make the history clearer for me ;-)

HauserV · 2018-10-05T21:21:49Z

The way I had thought about it before is that you do not add it to the CMake setup at all, you simply use the headers-only branch version as a submodule in the right location and hence you don't need any additional include paths.

Well, the difference here is that I acrtually want CMake to manage the paths for me so that I don't have to care where I had put the library: in your case I would have to include the snowhouse which is located in ../../third_party_libs_dir/snowhouse or something like that while in this case I can just say that I want snowhouse and if the actual path changes because I move it elsewhere, CMake fixes that itself.

That's a solution I have also considered, however, a symlink is not portable (please remove it again). I'd rather bump the version. (Btw you don't have to make any changes regarding version or anything; that's my task. I just wanted an opinion.)

Really? I use them in git projects regularly. I have actually created the link in Windows and pushed the repo from Linux (WSL) and it worked well in both but hey, you're the maintainer. Will remove it.

sbeyer · 2018-10-06T20:31:13Z

README.md

+target_link_libraries(app snowhouse)
+```
+
+


Again, I have very strong relational issues with empty lines. ;-) Please remove one of them.

This moves headers into a new include/ directory and turns off default running/building of tests. Document the CMake-based installation. Support for CMake < 3.0.0 is retained.

HauserV closed this Oct 5, 2018

HauserV reopened this Oct 5, 2018

HauserV force-pushed the master branch from 505ea99 to 2f47e38 Compare October 5, 2018 12:57

sbeyer requested changes Oct 5, 2018

View reviewed changes

sbeyer reviewed Oct 5, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

sbeyer reviewed Oct 5, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

HauserV force-pushed the master branch 2 times, most recently from cab1ce9 to 8f23480 Compare October 5, 2018 20:45

HauserV force-pushed the master branch from 0641093 to 9c57f3e Compare October 5, 2018 21:32

sbeyer reviewed Oct 6, 2018

View reviewed changes

Make snowhouse a CMake library

5d08afa

This moves headers into a new include/ directory and turns off default running/building of tests. Document the CMake-based installation. Support for CMake < 3.0.0 is retained.

sbeyer force-pushed the master branch from 9c57f3e to 5d08afa Compare January 9, 2019 21:19

sbeyer approved these changes Jan 9, 2019

View reviewed changes

sbeyer merged commit b25c70f into banditcpp:master Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make snowhouse a CMake library #38

Make snowhouse a CMake library #38

HauserV commented Oct 5, 2018

sbeyer left a comment •

edited

Loading

sbeyer Oct 5, 2018

HauserV Oct 5, 2018

sbeyer Oct 5, 2018

HauserV Oct 5, 2018

sbeyer Oct 5, 2018

sbeyer commented Oct 5, 2018

HauserV commented Oct 5, 2018

HauserV commented Oct 5, 2018

sbeyer commented Oct 5, 2018

sbeyer commented Oct 5, 2018

HauserV commented Oct 5, 2018

sbeyer Oct 6, 2018

Make snowhouse a CMake library #38

Make snowhouse a CMake library #38

Conversation

HauserV commented Oct 5, 2018

sbeyer left a comment • edited Loading

Choose a reason for hiding this comment

sbeyer Oct 5, 2018

Choose a reason for hiding this comment

HauserV Oct 5, 2018

Choose a reason for hiding this comment

sbeyer Oct 5, 2018

Choose a reason for hiding this comment

HauserV Oct 5, 2018

Choose a reason for hiding this comment

sbeyer Oct 5, 2018

Choose a reason for hiding this comment

sbeyer commented Oct 5, 2018

HauserV commented Oct 5, 2018

HauserV commented Oct 5, 2018

sbeyer commented Oct 5, 2018

sbeyer commented Oct 5, 2018

HauserV commented Oct 5, 2018

sbeyer Oct 6, 2018

Choose a reason for hiding this comment

sbeyer left a comment •

edited

Loading