getml / reflect-cpp

A C++20 library for fast serialization, deserialization and validation using reflection. Supports JSON, BSON, CBOR, flexbuffers, msgpack, TOML, XML, YAML / msgpack.org[C++20]
https://getml.github.io/reflect-cpp/
MIT License
901 stars 76 forks source link

Change project includes to relative (related to #30) #38

Closed ChemiCalChems closed 8 months ago

ChemiCalChems commented 8 months ago

After some consideration as to the true meaning of issue #30, I think we misunderstood what the author of said issue, @vaijns, actually meant. In their original post they said

Most includes seem to use an absolute path (starting with "rfl/...") with some exceptions I found:

from_named_tuple.hpp
fields.hpp
Validator.hpp
Timestamp.hpp
PatternValidator.hpp
Literal.hpp

which have some relative paths defined as well (might wanna unify that).

However I think this makes it a bit harder to include the library just for some quick testing as you have to specify an include directory (which you'd probably do for production anyways though ofc).

They are absolutely right that if an end-user of the library is to use it header-only or otherwise, they shouldn't need to accommodate to our internal practices of how we include files by setting an include directory in their build system. Our library should be build-system-agnostic in the sense that if the user include one of our files correctly, we should internally resolve that file without depending on any of their settings (ie. using relative includes).

As I said in my comment in #30

The more time passes, the more I think we should've gone the opposite way on this issue, as @vaijns originally suggested.

The rationale is we shouldn't expect users to have to set any extra include directories when using our header-only library as a drop-in. The headers should include each other in a relative manner so no matter where our library is, no matter what the user's build system looks like, if he manages to include one of our files, then it can find others because they are all packaged together anyway.

I've been looking around some header-only libraries and this seems to be done quite often, Hanickadot's CTRE is a good example of this practice.

I hereby request this issue be reopen, that all includes be changed to relative paths, and that we add somewhere in the README that developers should strictly adhere to this for the stability of the library. I'm currently looking for a clang-format / clang-tidy flag to could help us with this, maybe now that we have Github Actions a custom action could be written, I'll look into that too.

This will become even more of a problem the moment we start adding more dependencies. We should solve this now.

In the meantime, I'll get a PR going.

This is said PR.

To ease the task, I wrote this little Bash script (to be run from the include directory) which I will leave here for future documentation purposes (in case we have to do it again some time, though I hope not...):

#!/bin/bash

for i in $(seq 1 5); do
    for dir in $(find . -mindepth $i -maxdepth $i -type d | sed 's$^./$$'); do
        sed -i "s|^#include \"$dir/|#include \"|" $dir/*.hpp
        sed -i "s|^#include \"rfl/|#include \"$(for x in $(seq 1 $((i-1))); do printf '../'; done)|" $dir/*.hpp
    done
done

I ran grep -R '#include "rfl' from within include and the only results that came up after my changes are in include/rfl.hpp as it should be.

I'm creating this as a draft since I want to add some documentation regarding this change in a new How to contribute section in the README, and I'd like to study whether I can write a Github Actions check for this.

ChemiCalChems commented 8 months ago

Alright, so I finally managed to get a check going for non-relative includes. As you can see, in fact, on commit 564aa19 the check failed because of a commit push to main a couple of days ago that used the old include status quo in rfl/to_view.hpp (this also serves on an example as to how the errors as shown in the workflow, so that's quite convenient) . I cherry-picked the commit and changed the include to be relative and now that check should pass.

This introduces a trivial merge condition, though, but oh well.

I'll add another commit with a section in the README stating includes should be relative for the well-being of the project and set the PR as ready for review.

liuzicheng1987 commented 8 months ago

I‘ll take a look later and then merge.

ChemiCalChems commented 8 months ago

Decided to clean the history up a bit. Waiting for checks to pass, but everything should be fine.

liuzicheng1987 commented 8 months ago

@ChemiCalChems , is this ready to be merged?

ChemiCalChems commented 8 months ago

Yeah, all ready to be merged. In the future, I'll try change this for a clang-tidy check of some sort, but for now it'll do.