Move public headers to support-lib/include/djinni #281
Conversation
This separation of public and private headers allows users to specify "support-lib/include/" as obvious header search path and then use "#include <djinni/...>" as reliable include prefix. This also gives a sensible default for all the "--xxx-base-lib-include-prefix" values.
|
Automated message from Dropbox CLA bot @mknejp, it looks like you've already signed the Dropbox CLA. Thanks! |
|
Do you know when this plans to be merged in Thanks! |
|
I ran into the same problem as @bvirlet. I also believe that this PR should fix it. Can anyone review/merge this into master please? 🙏 |
There was a problem hiding this comment.
Sorry for the long delay, but I'm back to trying to catch up on Djinni PRs.
Also sorry to re-open old discussions, but I think we may've had different ideas of where things ended up after discussion in the prior version of this PR. I'm on board with the new file layout which separates contract from impl, and lays out the headers with a clear include-path root and prefix for those who want one. I'm also on-board with the use of the pre-existing prefix arguments (with new defaults) to change the include lines in generated code for the example and test-suite.
What I'd prefer to see changed (and I thought was the agreed outcome of the discussion on the former PR) is the way that the support-lib headers include each other internally. I'd like them to use relative paths which will work with or without the configured include path, rather than the current state which uses paths like "djinni/jni/whatever.hpp" and will break use cases which don't set that include path.
At Dropbox we prefer to avoid include paths in favor of an "absolute" path relative to our main repo, and I'll deal with tweaking the prefix arguments to match that. But if the support-lib headers include each other in a way which uses the "djinni/" prefix, then we don't really have the flexibility to do that.
I'm on the fence about the question of how the hand-written non-header files in the support-lib include those headers. I'd probably use a relative path there too, and just assume the fact that the "src" and "include" directories are siblings in the repo. However I can see the argument for not wanting to hard-code that for build systems which put headers in different places. I don't see that as a blocker since it can be addressed in a localized way with per-project includes (like the entries you've already included in the gyp file) rather than requiring global changes to include paths in end-user code.
TL;DR Change the headers in the support-lib to include each other using relative paths, and I'll merge. Other changes are optional, but you have my thoughts here. If you have other concerns to raise that I've forgotten about, go ahead and I'll try to be more responsive than I have been in the past few months.
This is the continuation of #247. I had to recreate the PR.
I finally got around to update this. The changes are as agreed upon to separate the public and private files into
support-lib/include/djinniandsupport-lib/src. Tests and examples are adjusted accordingly.