Developers RFC: Includes reorganization

[ferdnyc]

I've been thinking about a major reorg to the repo header file management, for a couple of reasons (which I'll explain). So I wanted to post my thoughts here and invite feedback/commentary from both the other libopenshot developers, and our library consumers. (Though my current plans have considered the impact on the latter, and include pains taken to keep it to a minimum, at least short-term.) As well as, of course, anyone else who's interested and can offer any relevant insights or experience.

Motivations

My concerns regarding the current header setup are,

1. As I detail in bindings: Remove relative header paths #469, relative include paths can be problematic. And relative include paths that leave the root of the source tree can be disastrous. Depending where that path is anchored, you could end up with a file from almost anywhere. #include "../../../include/anything" is just a bad idea and a recipe for problems.
2. Without the relative paths, all we have are bare filenames, and that's a poor way to organize a set of API headers. #include "Color.h"? Do you know how many possible matches there could be for that name?¹
3. Related to the previous item: The include paths used by library consumers look nothing like the paths we use internally. That means there are aspects of our code that are simply not tested at any point during development. In fact, as internal developers we never even get a glimpse of what it's like to code against libopenshot as an external consumer. That's a troubling hole in the development plan.
4. The whole src/ vs. include/ structure for a codebase is pretty dated, these days, and it shows in the hoops we have to jump through to make it work. The CMake/Qt AUTOMOC, for example, by default will scan for and process Qt classes in any implementation-header file pair it finds in a directory tree — but it expects to find both files together. It took manually adding all of the Qt headers as source files of the library, in the CMake configs, to get AUTOMOC working with the current organization. (That setup works fine, but it was an extra PITA that could've been avoided.)
5. On a personal note, I find it particularly hard to edit a split directory structure like we use. It's pretty rare that editing a particular .cpp file doesn't send me off to open its header file as well, or vice versa. Most editors don't make that very convenient — not nearly as convenient as having the files side-by-side would be, anyway.

Proposals

So, given all that, I'm actually considering a PR that would go one of two very different routes. And I'll say up front, I vastly prefer the first option. But if it's objectionable for some reason, the second would be my compromise proposal. I'm advocating that we do one of the following...

1. Move all of the headers into the src/ directory, keeping the directory hierarchy the same. So, each filename.h file would be located in the same directory as the corresponding filename.cpp file, and it would be included in the internal code as just #include "Frame.h" or #include "Qt/VideoRenderer.h". (My concerns about generic pathnames are lessened considerably when the files are all in or below the source file tree, rather than having to move upwards out of it.)
2. Create a subdirectory include/openshot/, and move all of the headers inside of that directory. The build-time include path would be set to ${PROJECT_SOURCE_DIR}/include, and headers would be included by internal source code using e.g. #include "openshot/Frame.h" or #include "openshot/Qt/PlayerDemo.h".

Discussion

What I like about the first option is that it addresses Motivation points 1, 4, and 5, while as I mentioned greatly lessening my concerns regarding point 2. The only box it doesn't tick is point 3.

The second option is actually primarily focused on Motivation point 3, and it also addresses point 2 much better than the alternative, while also covering point 1. Unfortunately it's no relief at all when it comes to points 4 and 5. Still, the earlier Motivation items are a much higher priority, since 4 and 5 are mostly just about convenience/preferences.

Issues considered in the formation of these proposals

Header installation

The primary motivation for separating headers from source code tends to be packaging/installing, since (in theory) it's easiest if the install can copy ${PROJECT_SOURCE_DIR}/include/ to /usr/local/include/libopenshot/ or wherever, and done — there's your header export. Easy-peasy.

Nice theory, but CMake doesn't work that way. It already doesn't work that way. Currently we're installing the headers from CMake with the following command:

install(DIRECTORY ${CMAKE_SOURCE_DIR}/include/
  DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/libopenshot
  FILES_MATCHING PATTERN "*.h")

That could be changed to install(DIRECTORY ${CMAKE_SOURCE_DIR}/src/ ...) and it would work exactly the same, thanks to the FILES_MATCHING PATTERN "*.h". There is simply no compelling reason why headers need to be segregated from their source files, anymore.

External API consumers

Regardless which of these plans we go with, my intention is that when the headers are installed, they go into a new include/libopenshot/ subdirectory named openshot, and EXTERNAL libopenshot developers would access the headers using #include <openshot/Class.h> statements, protecting them from filename clashes when using our API.

(It also means that every class is included via a header path with the exact same, complete name — including the namespace. That's not only safer, it actually makes more sense.) If we were to go with proposal 2, then internally we'd be doing much the same. (We'd probably just use double-quoted names instead of angle-bracketed names, since the headers aren't system headers when not installed.)

Unprefixed include deprecation

To ease the transition to the new paths, and so that we don't break every external API consumer's code completely, what I'd want to do (and again, this goes for either internal reorg proposal) is set up a transitional set of "shim" files that emit deprecation warnings. Here's how that would look:

• All of the new headers get installed into prefix/include/libopenshot/openshot/.
• The include path for the library is defined as prefix/include/libopenshot, same as always.
• API users are meant to change their code to the newly-canonical #include <openshot/Frame.h> form.

But since that'll take some time, for the next couple of releases or so, a set of "relocation" headers would also be installed one level up, matching the previous hierarchy. For every "real" header prefix/libopenshot/openshot/(*/)filename.h, there'd be a corresponding "redirect" header.

A redirect header prefix/libopenshot/FOO.h would look roughly like this:

#ifndef OPENSHOT_REDIRECT_HAVE_WARNED
#define OPENSHOT_REDIRECT_HAVE_WARNED

OPENSHOT_DEPRECATED_MSG("Unprefixed \"CLASS.h\" includes are deprecated and will
be removed in a future release. Use \\#include <openshot/CLASS.h> style includes instead.")
#endif

#include "./openshot/FOO.h"

With some macro magic borrowed from Qt or the like to emit deprecation messages, that should make the reorganization transparent for external API users and prevent their projects breaking, while still making sure to inform them of the change and give them time to update their code.

The bindings

Even though with #469 I've fixed the bindings so that they could live anywhere, in any of these scenarios, my thinking is that they'd move out of the src directory completely, probably into a bindings/ directory at the top level of the repo, alongside src/ and possibly include/. There's no reason to have them buried so deeply that the Python module builds as src/bindings/python/openshot.py inside the build dir.

Especially since we really need to start building openshot.py / _openshot.so as a Python package, complete with metadata that OpenShot can probe for as a dependency, and so setuptools can install it instead of us having to play stupid distro tricks in CMake. That structure will most likely add one additional directory level.

The Ruby bindings, by the same token, should really be packaged as a rubygem. (Though I don't think that'll require a subdirectory.)

The rest of src/

Who needs it at all? To my mind, the "src/" distinction is artificial and meaningless anyway. IMHO src/ should really be called lib/ or libopenshot/, since that's what it's the source for, specifically. In fact, I'd even be in favor of renaming it, and rearranging the repo structure so that...

• src renamed to /libopenshot/
• src/examples/ moved to /examples/
• src/Qt/demo/ also moved to /examples/
• src/bindings/ moved to /bindings/, as mentioned
• The test media files in src/examples/ moved to a new /data/, since they're not the source for anything.
• (Possible/worth exploring...)
  • src/effects/ gets relocated to /effects/ and built as a distinct subdirectory with its own CMakeLists.txt, which would put us in a much better position to finally begin incorporating @kirillmakhonin's DynamicEffects proposal from WIP: DynamicEffects: Add code for Linux with tests #345. (At least in the short term the effects would still get linked into libopenshot.so same as ever, only the build steps would change.)
  • We unbundle jsoncpp from /thirdparty/jsoncpp/, since it's weird that this one arbitrary dependency, of all the libraries we require, is bundled in an out-of-date state inside our repo.
  • We bundle, instead, libopenshot-audio as /extern/libopenshot-audio/ — not by cloning its source there, but as either a Git submodule or a CMake external project. (Either of which would give the option of downloading the source to be compiled during the libopenshot build process.) We could then support installing and packaging them together, so they're conceptually a single library that just happens to be split into two separate loadable object files.

That last item is getting way outside the scope of this proposal, but it would (for example) make it workable to finally begin doing libopenshot CI builds on Windows and MacOS. Whereas today we only test on Linux because it's the only platform where we can pull libopenshot-audio in from the daily PPA as a dependency.

Keeping up with the Indianas

In proposing any of this, I'd be remiss if I didn't at least make a cursory survey of the repo organization in use by other projects, to get a feel for what others have deemed appropriate for their own situation, at least. (Though whether or how that would apply to ours is certainly debatable.) Looking over some long-lived, active codebases that we'd be familiar with...

1. JUCE: First, close to home, JUCE installs as a collection of modules, each a directory containing a multi-level hierarchy of .cpp and corresponding .h files side-by-side.
2. Qt: Maintains a separate Git repo per module (QtCore, QtMutimedia, etc.), where each repo houses multiple libraries, plugins, and tools. They do use a /src/ directory, but it contains subdirectories per logical build unit, each containing .cpp and .h files side-by-side.
3. FFmpeg: Repo has a top-level directory per library (libavcodec, libavformat, etc.) and one called fftools containing the commandline sources. Each directory contains .c and .h files side-by-side.
4. ImageMagick: Nearly identical to FFmpeg. Top-level Magick++, MagickWand, etc. dirs, plus uilities for the commandline sources and man page sources. For the libraries,
   • MagickWand and MagickCore contain .c and .h files side-by-side
   • Magick++ contains a lib/ subdir holding .cpp files, and a Magick++ subdir beneath that containing all the headers, which is a hybrid option I'd meant to raise — we could have an openshot/ header dir inside the source tree, instead of in /include/, so that Frame.cpp would #include "openshot/Frame.h" the same way external API callers would.
5. VLC: The odd bird of the bunch. Has both src/ and include/ as toplevel repo dirs. Maintains a separation between public and private headers — src/ contains a hierarchy of .c files, some of which have private headers alongside. Includes in source files are a complex mix of #include <vlc_codec.h> public-header includes (from /include/vlc_codec.h), #include "something.h" local private-header includes, and #include "../somethingelse.h" relative private-header includes. Looks confusing as all getout, TBH.

Conclusion

So, there you have it. I actually found it hard to locate an example of another project with any sort of include/ directory at all, until VLC — if I hadn't thought to check what the French have been up to, I wouldn't have found a single example.

Sorry this turned into such a tome, but I wanted to be comprehensive. High-level summary maybe forthcoming if anyone wants, though of course you'd have to read all the way down to here before you'd see that offer. I'm a stinker.

CC: @jonoomph @eisneinechse @SuslikV @musteresel @mkarg @DylanC @N3WWN @jeffski @ckirmse @chad3814 , kirillmakhonin who I already tagged earlier, and of course whichever really obvious user I'll end up having left out, so I can feel guilty later.

Notes

1. I can give you some idea: On my system, there are at least six completely distinct Color.h files, out of 29 total matches for that filename (many are duplicates) — and that's with the capital 'C'! If I include color.h, that number rises to 58 total file matches, representing roughly 15 distinct files.

[SuslikV]

1. Move all of the headers into the src/ directory...

I can live with it.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.