Skip to content

Feature/adapted struct jsonizer#1

Merged
daminetreg merged 47 commits intomasterfrom
feature/adapted_struct_jsonizer
Nov 28, 2015
Merged

Feature/adapted struct jsonizer#1
daminetreg merged 47 commits intomasterfrom
feature/adapted_struct_jsonizer

Conversation

@daminetreg
Copy link
Owner

@linkineo and @dan-42 I would like to invite you to review my code here.

Please note that I've put the jsonizer in boost::fusion namespace, but this should be changed, I imagine to pre::jsonize / pre::dejsonize or something similar, please feel-free to propose.

The boost::fusion::for_each_member is something that I should take time to provide to boost officilially but for the moment lives in pre.

Thanks for your help.

Please also review :

@linkineo
Copy link

linkineo commented Oct 4, 2015

DOC: you could add a reference to the Hunter cmake dependency, as this is might be required by any project using your lib, and hunter, while cool, is far from being mainstream yet.

@daminetreg
Copy link
Owner Author

Hunter shouldn't need any special treatment, but sure it makes sense to informe about it.

@linkineo
Copy link

linkineo commented Oct 4, 2015

Hunter seems to need a special treatment in as much as I need to conform to hunter's expectations (huntergate stuff etc) whenever I include the library in a project that is not using hunter yet, or am I wrong ?

@daminetreg
Copy link
Owner Author

Actually no the gate is taken for the build of pre and only the good things get installed on your system but you don't need to do anything specific. But I'll write some docs to explain how disabling it in case one don't want hunter to download the dependencies.

@linkineo
Copy link

linkineo commented Oct 4, 2015

It's not about downloading dependencies, which is cool, but about requiring people to adapt their cmakelist to use hunter, which could be a barrier to adoption.

@daminetreg
Copy link
Owner Author

there is no need for them to adapt their cmakelists.

@linkineo
Copy link

linkineo commented Oct 4, 2015

so I can add lib-pre as a submodule to my project, add it as a subdirectory in my cmakelist and just expect it to compile with no mention to hunter whatsoever ?. I tried, could not make it work. You got my git bundle by mail, maybe you'll see what I'm doing wrong.

@daminetreg
Copy link
Owner Author

Actually the project is thought to be added via find_package or via my patched hunter but not as git submodule via add_subdirectory. That's possibly why you are expecting errors as I didn't test the inclusion of the project this way and it's not the way normally one should use if he don't control the subproject. That's why hunter and cmake externalproject was invented.

@linkineo
Copy link

linkineo commented Oct 4, 2015

add_subdirectory may not be the best way to include dependencies, but it usually works without a hitch. anyway, would be cool to tell in the doc which ways you expect people to use your lib.

@daminetreg
Copy link
Owner Author

Yes I agree 😄 I'll anyway look that add_subdirectory works, but it looks like the problem you had is not because of this, it's because I wrongly referenced hunter that I in fact override locally in my build with -DHUNTER_SOURCE_DIR.

I did test it now that I fixed this, tell me if it works. Sorry for the problems.

@linkineo
Copy link

linkineo commented Oct 8, 2015

ok, so this time I went for the sysroot-style solution.

  1. I compiled lib-pre, and installed it to a test syroot
  2. I made a test app including your headers
    -> Does not compile, as nlohmann-json headers are missing in the sysroot, when including adapted_struct_jsonize.hpp.
    Your cmakelist does not seem to install the headers required, and nlohmann-json's cmakelist does not support make install.

@daminetreg daminetreg merged commit cba1615 into master Nov 28, 2015
@daminetreg daminetreg deleted the feature/adapted_struct_jsonizer branch March 6, 2016 21:48
@daminetreg daminetreg restored the feature/adapted_struct_jsonizer branch September 9, 2016 14:24
linkineo pushed a commit to linkineo/express-cpp that referenced this pull request Mar 2, 2017
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