Move public headers to include/djinni #247
Conversation
This separation of public and private headers allows users to specify "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! |
|
I've given this a look, and there's nothing wrong with the implementation, but I still have some concerns about the overall goal/design. While I take your point that this use of global include dirs and prefixes might be quite wide-spread and well accepted, I'm not sure it's important enough to force it on existing users or users with a different preference as to how to manage their includes. Obviously I'm biased in that Dropbox is an existing user who would be broken by this, and I'd rather not have to add a new global include directory. As a compromise, I'd be okay with re-arranging the headers so as to add a parent directory "djinni" for people who prefer that mechanism. However I wonder what you see as the disadvantage of having the headers include each other using relative paths, which will work regardless of the configured include dirs, rather than enforcing a specific include path on users. I'm less concerned about the support-lib sources having such assumptions since they can be in their own project/target with its own dependencies described in gyp, cmake, etc. The headers can be included indirectly from most of a user's code, so there's no way to let them use prefixes without requiring a global configuration change. As a smaller nit-pick on the proposed layout, I wonder why there should be include/djinni/support-lib/whatever rather than support-lib/include/djinni (or include/djinni-support or include/djinni/support-lib or somesuch). My high-level point is that the support-lib is the only piece of code which people might link against (not Djinni's scala implementation) so it seems like it's the library which has an include directory, rather than Djinni as a whole having an include directory. The proposed layout seems to actually reduce clarity by putting the headers and sources farther away from each other. I'm not sure the interface vs. impl distinction is more meaningful here than just file extensions, given that there are "implementation" headers like proxy_cache_interface.hpp in the include dir in this proposal. The layout might make more sense if the normal way most developers interacted with the support-lib was as a binary library with a separate set of headers. However, we currently expect developers to interact with the support-lib as a set of source they include in their project, in which case having the source split in two places seems to make life harder on them. |
|
For me the biggest problem I had related to this, was with If the includes were not related to relative path, that would be a big help. |
I have been bitten by relative paths before when the compiler was including a file I didn't expect. Or when a file was specified only by name without a path prefix and the compiler again included the wrong file. For me this is a means of protecting against the implementation defined behavior of include searching or projects that may happen to have a file with the same name somewhere. It's also a means of preventing any kind of ambiguity that is outside your control. It makes a library more robust by relying on something that should be pretty much unique anywhere and where even the implementation defined nature of include searching won't include something that doesn't actually have the name you asked for.
I see the support library and the Scala implementation as two components of the same thing. The generated code cannot operate without the support library. They are both part of the "Djinni package". As for the order of directories: It is the support library of Djinni so in my mind it should be in a folder called
I sort of get this point. One could think about putting public and private files into the same folder and expect the resulting root as the header search path. What I don't like about this approach in general is that when I setup the project to build the library I don't know which headers are required by users and which aren't. Libraries where public and private files aren't separate make it more likely for users to include (and use) code they're not supposed to which can lead to all kinds of problems. One could argue that people aren't supposed to manually include any Djinni files as only autogenerated code does it. But what if at some point something is added to Djinni that exists besides the support library and people actually would have to include manually.
Considering there is a gyp project (and CMake under way) to make the support-lib available as some form of prepared "package" (as built binary or project to build yourself) maybe this expectation is misguided. I think part of the confusion comes from there being a top-level |
There still are relative paths in the private implementation of this PR, but I agree with Andrew that one probably builds the support library in a separate environment/project where no other include paths are specified except for those needed to build Djinni. Even if that was still a problem one could pick some path in the djinni repo that is pretty much unique (or put the support lib inside |
|
@4brunu can you describe the issue you had with parent directories? Are you sure it was with the file you indicate? We used to use "../" by default for extended records, when including a hand-written header from generated code. That was definitely an invalid assumption, and something which has been fixed (made configurable, defaulting to no prefix). However I haven't heard anything about issues with parent-dir includes within the support-lib directory, so I'd like to know more about what you encountered. I just ran a quick experiment with clang++ on OSX, and it has no problem with including "../test.hpp" either based on the location of the current file, or based on a directory configured with -I. And when I create duplicate headers which could be found in both ways, the file-relative one is preferred, as I'd expect. I also confirmed that preference is relative to the file containing the #include, not the top-level file of the compilation unit. |
|
Responding to @mknejp, sorry this is getting so long.
Can you explain how you've been bitten by this before? I understand that include searches are implementation-defined, but in reality I haven't ever encountered a compiler which doesn't support relative path lookup, and doesn't prefer it to include-dir based lookup. I could imagine scenarios where a new incorrect #include ends up silently picking up the wrong header (and causing other problems down the road), but I think any #include with a correct relative path should continue to work regardless of what conflicting files may exist in other include-dirs. (Note that I'm against duplicate header names in general, but that's not something which can be enforced across multiple libraries.) If there were no issues on the other side here, I'd agree with you that a guaranteed unique include path would be a win. However I think the fact that it imposes a global choice of compiler flags on a user's codebase is an argument against it, as is backward-compatibility (though that's less my focus here). I think that internal use of relative paths is safe enough to be the best compromise. Given that most people never hand-write includes of the support-lib anyway, I don't want them to have to think about setting up compiler flags for #includes they didn't write. They should be able to think about this once when they set up the command-line flags to Djinni, then never again. When they do that, it's totally valid for them to decide to use a global include path and unique prefix as you propose. However it should also be just as valid for them to use some other approach. For instance, at Dropbox, we always use an "absolute" path relative to the root of our main repo.
I never proposed putting support-lib in front of djinni, so we agree there. If you want a unique prefix it could be djinni/support-lib or djinni-support but not support-lib/djinni. I don't see the tool and the support lib as part of the same "package". If we're looking ahead to pre-packaged binaries, then the tool is something you install with apt-get or homebrew, then run from the command-line. It has nothing to do with your source-code. On the other hand the support-lib is something you need to link into your code. Responding to your later comment, perhaps it's the "src" directory which is misnamed here (I've always thought it unclear). I think if we were doing it from scratch I might have top-level directories "generator" and "support-lib".
I agree on not liking the mix of impl files in your include directories. There aren't any truly-private headers currently (since they all need to be included from public headers) but there could be. I'd propose a layout like this: Or maybe "src" rather than "impl" if that's not a name which is meaningful enough to most people. It's my preference for "implementation" but I'm not sure if that's widespread. |
Not in detail, no. I do remember there were problems with one compiler including a different file than another. Don't ask me which compiler/platform it was because I don't remember. What I do remember is spending half a day banging my head against the desk because of completely nonsensical compiler/linker errors that made absolutely no sense until I somehow realized the compiler was including the wrong file. That is a waste of time I do not want to repeat or risk imposing on somebody else.
When weighing the need to add an additional
Sure if most of the code you're using is under your control and there are basically no external dependencies that's a valid approach (even though it has problems as well). But as soon as you add any significant external library that philosophy goes up in flames. Furthermore, if the includes to a 3rd party library have a unique prefix it is clearly separated from your own stuff, at the front of the
Ah I misinterpreted what you wrote. Right as long as
The tool may be usable on its own, but you still need to link the result, in addition to the support lib. The latter could just as well come prebuilt as binary so I don't really see the difference. At the end of the day you need both to get a usable result unless all you're generating are unbridged types.
From what I can tell |
|
I think something which is making consensus difficult here is that Djinni behaves quite differently from a standard C++ library. I'm generally against allowing an external library to impose global requirements on my code, but I can generally address that by wrapping it in my own library, so that only the code which directly uses it needs to worry about extra include paths, macros, or whatever. Djinni makes that difficult since it generates code which can be used broadly, and thus its impact is all over my codebase. I think the "public" Djinni headers actually usually don't include support-library headers, but there are other assumptions (such as the need to include extended record headers) which really suggest that even the "private" parts of the generated code should be in the same library as the code hand-written code around it. That makes the generated code harder to isolate. There is also generated code in layers like ObjC++ and JNI whose users might not be used to dealing with the standard practices of C++ libraries. My general strategy for avoiding imposing new requirements or compatibility breakages on Djinni's users has been to make everything optional/configurable. The large number of command-line arguments are the result of that. Unfortunately the support-lib source isn't generated, so it can't easily be configurable in the same way. (This issue has added to my feeling that it would be good to make the support-lib code generated, or at least configurable, but that's a separate feature.) I'd suggest we agree to disagree on include path preferences, and look for a compromise option which leaves the freedom up to the developer. If conflicting header names are your primary concern, I'd suggest that might be addressed by renaming some of the headers. Most of them already have a name beginning with "DJI" or "djinni_", so you could add such a prefix to those which don't, which look to be implementation headers, so they're unlikely to cause more breakages in hand-written code. With that in addition to relative internal includes, it seems like the risk of any difficult-to-diagnose issues in unusual compilers should be minimized, and you can configure the generated code to use unique paths as desired. Importantly I'd really like the compatibility-breakage here to be limited to something a typical user (who #includes generated sources, but not the support-lib directly) can address by modifying the generator command-line and nothing else, and I think this suggestion would accomplish that. Separately responding to the file layout ideas: I'm okay with src vs. include to separate public vs. private sources. I find "src" less obviously private on its own, but when put next to include it's pretty clear, and would be even moreso if someday in a theoretical install there were also a "libs" directory for pre-built binaries. I don't there's any need to mix together the Scala source for the generator with the sources for the support-lib. Users interact with them in very different ways, and they may even be distributed separately in future. I wouldn't mind re-naming the top-level djinni/src directory to clarify that it's for the generator only, though I think we might be getting into the realm of small pay-off for compatibility-breaking changes. |
That's certainly a noble goal but at some point one has to ask themselves whether supporting/maintaining a bazillion options is worth the trouble (especially if people end up not using these options).
CMake's Another issue that just crossed my mind however is the include prefix for Apple frameworks. #248 currently offers the ability to build a Specifically in the case of Djinni we end up with paths like So the question remaining here is: does the compromise include having |
|
Agreed that configurability has trade-offs, particularly if there are no users. In this case we know there's at least one user who wants each option (being you and me :) Yes, I'm on board with relocating the public headers so that a djinni prefix directory is present, allowing people to follow the prefixing practices you suggest. My main suggested tweak is for the internal includes between headers to use relative paths rather than "djinni/whatever..." so that people who don't want to follow those practices can use them too. I think that means that both your and my codebases should be able to use the same header layout, simply by tweaking the Djinni command-line. As an aside, in your your CMake/Framework example, I feel like the in-repo layout of the headers doesn't really matter since they're getting copied into an install directory, or a framework anyway, but I certainly see the point of keeping the layout consistent. |
|
@artwyman The problem I had was when trying to create a framework with cocoapods on iOS. |
|
@4brunu ah, so CocoaPods was flattening the directory structure of the includes? That seems like a general problem with include dirs with any sort of structure, whether they used relative paths, or prefixed paths. Unless that kind of issue is common, I do think it's good to maintain the structure of the Djinni includes, so there's a clear separation between the headers for JNI, ObjC++, and shared use. |
|
@artwyman Yes, it was that. |
|
Yeah, Xcode does that thing where it just flattens the folder structure and dumps everything into a single Header directory when building frameworks. I never understood why they do that. It's just stupid. |
|
Hmm, Xcode does also tend to encourage unqualified header includes, since it automatically finds headers by name if they're in the same project, ragardless of their path. If this kind of directory-collapsing is common, and we're re-arranging headers anyway, I wonder if we should consider a layout without any structure, by putting all the headers in the same folder. It would be a bit of a hassle to mix together, say, JNI and ObjC++ headers in the same folder, though, since it would be easy for them to get added to projects which can't use them. We might need to duplicate the common headers in multiple folders, which is annoying. I'm not sure I'm actually suggesting to do that right now, since it seems overcomplicated as I think through it. It's just a thought. |
Which is so stupid I wonder how it ever made it in. It encourages a practice that is totally non-portable. It may be convenient for small/amateur projects but is pretty much a useless "feature" for large-scale development and is a source of ambiguities. But oh hey, it's Apple. They don't want you to write portable code... Fortunately it is not a problem when using a framework as it finds nested headers inside a framework. It's only a problem if people build frameworks with the Xcode-specific "public header" feature which nobody uses in C++ for obvious reasons. Every C++ project I have seen so far in Xcode has a user-defined copy phase to move the headers in a structure-preserving way.
That pretty much sums up my experience with most parts of Xcode
I claim there are three types of library users in regards to Xcode:
Anyway, enough ranting. |
|
Oh, yeah. The user-defined "copy headers" phase is what I've seen in action before. I wasn't sure if the suggestion here was that it was impossible to preserve structure even with that. Agreed that Xcode makes a lot of Apple-specific assumptions. They're not interested in encouraging cross-platform, for good financial reasons. Let's leave directory-flattening out of the discussion here, then. Seems like it's a bad behavior, which developers can work around when they need to. |
This separation of public and private headers allows users to specify
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-prefixvalues.Note this is a breaking change but I think it is important to have a clear separation of public and private files as discussed in the mobilecpp group. It also makes it more obvious from the folder structure what users have to put as their header search path when using djinni as well as to a more "standardized" and unambiguous way of specifying support library include prefixes when generating files.