Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize repo (headers moved to src/, bindings/ and examples/ made root-level dirs) & update build system #582

Merged
merged 21 commits into from
Oct 19, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Oct 19, 2020

REALLY IMPORTANT BIT

This PR is a resurrection of #504. The headline is that it moves all of libopenshot's headers out of the include/ directory, and places them all in the src/ directory alongside their implementation files. (The hierarchy is preserved: include/Qt/* moves to src/Qt, etc.) Meanwhile, the bindings/ and examples/ subdirectories are moved out of src/ and placed at the root of the repo.

To read my justification for this change, along with reasons why I feel it's worth the upheaval it will cause, see my initial proposal in issue #471.

(This PR thus: Fixes #471, though that issue's already been closed by the Stale bot.)

Slightly less important bit (with some additional details)

The original PR also had to be closed, but in that case it was due to the difficulties we encountered trying to reconcile it with the current repo state in the wake of several weeks' worth of other changes to develop. As such, this PR is being fast-tracked to merge quickly, pretty much as soon as it passes the necessary checks (and before any other changes land in develop). The hope is that this will help prevent the conflicts that would inevitably arise if any other changes are merged. @jonoomph has already signed off (in #504) on the concept of these changes, it's just a question of creating the opportunity to make them.

Given that this PR touches very nearly every source file in the repository, merging it will most likely result in conflicts appearing in many or all of the other open PRs. That's unfortunate, but also unavoidable given the nature of these changes.

After the merge, I will attempt to resolve any open PR conflicts on behalf of their submitters. There may be some I'm not able to sort out on my own, though, and I'll be forced to leave their resolution to the author. In addition, if anyone has local changes on a PR branch that they haven't yet pushed to Github, merging this PR will probably make that process messier and more complicated. Either way, I apologize in advance for this one-time disruption.

Just wanted to preemptively bring this impending codepocalypse to everyone's attention. #YouHaveBeenWarned

Courtesy-tagging submitters of all open PRs: @ckirmse @jonoomph @kartchnb @musteresel @chad3814 @PhysSong @aopa-frogslayer @kirillmakhonin.

Not at all important bit (with excessive details regarding the specifics of the PR)

From the original PR description:

This implements the proposal I made in #471, that header files live in the src/ directory alongside their *.cpp files, rather than being kept in an include/ directory.

  • Move all /include/**/*.h to /src/... (with same hierarchy)
  • Move /src/examples to /examples
  • Move /src/Qt/demo to /examples/qt-demo
  • Move /src/bindings/ to /bindings
  • Adjust source files (far fewer relative includes):
    • /src/**/*.cpp
    • /src/**/*.h
    • examples/**/*.cpp
    • tests/**/*.cpp
  • Change #include "JuceHeader.h" to #include <JuceHeader.h> everywhere, since that header isn't part of our local source for the purposes of a libopenshot build

I actually didn't do that, this time. It's still a good idea, I just left it out of the changes in the new branch. Perhaps I'll add it in before merge.

  • Adjust all CMakeLists.txt files to account for new include path
  • Add an examples/CMakeLists.txt, and move the logic for compiling and installing the necessary components to that file
  • Add logic to tests/CMakeLists, examples/CMakeLists, /CMakeLists.txt so that build components are imported locally when needed
  • Update Doxygen.in file to build docs from new locations

This will be confusing, at first, when looking for moved files. (I've confused myself a couple of times already.) The relocated bindings directory is the one that throws me the most. But it really is much simpler this way. Both in terms of humans editing the code, and in terms of the contents of the code.

The reason src/examples, src/Qt/demo, and src/bindings got moved out of src/ is in small part practical: If there are directories in src/ without any installable headers, the install(DIRECTORY...) CMake command will end up creating empty directories in the output location. So, I wanted to avoid that.

But at the same time, the fact that that happens makes the point: none of those directories' contents are part of the libopenshot source code itself. So they really don't belong in the libopenshot source dir. It makes more sense if they're separated.

The Doxygen docs mostly come out better, now. It never could follow the parent-relative #includes, so in the past none of the source code listings' #includes linked to the header in question. Now almost all of them do, except for a few #include "../foo.h" statements in the src/effects/ or src/Qt paths. (And in theory I could eliminate those using the include path, since #include "ReaderBase.h" should mean the same thing in src/effects/Blur.h that it means anywhere else. But I haven't tackled that for this PR, so it [still] uses #include "../ReaderBase.h".)

In addition, enhancements are made to the build system:

  1. FindOpenShotAudio.cmake now creates an IMPORTED target OpenShot::Audio to hold all of its configs. The configs are also still supplied in variables like OpenShotAudio_INCLUDE_DIRS, OpenShotAudio_LIBRARIES, etc., but the old LIBOPENSHOT_AUDIO_INCLUDE_DIRS and etc. variables are completely gone. The strongly recommended method of consuming the results of find_package(OpenShotAudio) is via the OpenShot::Audio IMPORTED target.

  2. One reason OpenShot::Audio is preferred: that target (and only the target) now incorporates all of the additional/conditional build configuration relating to libopenshot-audio. This includes things like defining the HAVE_ISFINITE and IGNORE_JUCE_HYPOT macros, the extra DLL/framework linked into libopenshot on Win32 and MacOS platforms, and some compile options that need to be set in various scenarios. All of that logic used to be scattered around the various CMakeLists.txt files, but it's now all been consolidated as part of FindOpenShotAudio.cmake. The module exports all relevant configuration in the form of properties applied to the OpenShot::Audio target.

  3. When src/CMakeLists.txt detects a successful find_package(ImageMagick...) check, it now creates a target ImageMagick::Magick++ and applies the necessary package configuration by setting properties on that target. (Actually, it creates a target Magick++_TARGET, then aliases ImageMagick::Magick++ to that target. The "whys" of that are nebulous, uninteresting, and best chalked up to "CMake nonsense" and left at that.) target_link_libraries(openshot PUBLIC ImageMagick::Magick++) then attaches all of that configuration to the libopenshot library build.

  4. The reason for all of these newly-added targets is, it allows us to take advantage of automatic config propagation across the entire build system. Previously, tests/CMakeLists.txt had to duplicate some of the configuration from src/CMakeLists.txt, because non-target configs were limited to the src/ directory (and subdirectory) context. And while the non-target configs were previously available to src/bindings/*/CMakeLists.txt as subdirectories of src/ (and src/examples/ was configured by src/CMakeLists.txt), now that those directories are moved out of src/ and have their own CMakeLists.txt files that's no longer the case.

    CMake targets are global by definition, so any build configuration done in src/CMakeLists.txt by means of target properties will automatically be propagated to tests/, bindings/ and examples/ without the need for any redundant code. This makes the CMakeLists.txt files in those directories less complex, while at the same time making the overall build system more resilient and predictable since it's no longer possible for CMakeLists.txt inconsistencies to cause config changes from directory to directory.

    The now-redundant configuration previously being (re-)done in tests/CMakeLists.txt has been removed. (The sole exception to this is the optional DeckLink BlackMagic integration, which for the moment is still done via legacy non-target configs and therefore still has some minor duplication.)

A couple of other minor changes are also included:

  1. In testing examples/Example.rb I unearthed several bugs in our Ruby support, from code errors in that example file to build-configuration errors in bindings/ruby/CMakeLists.txt. This PR fixes all of those bugs, and after applying those fixes I was able to run Example.rb with Fedora's ruby command and have it display the image retrieved via GetFrame() as expected.

  2. I renamed the MappedMetadata typedef to MetadataMap in the SWIG interface files, simply because it's the proper name based on the pattern established by the surrounding code. That change shouldn't affect any code, as those typedefs are only used in the generated SWIG code and aren't typically accessed directly. (The names are exported in the SWIG-generated bindings, though, so it's possible some code out there makes use of the bindings in some esoteric way that involves directly referencing the Python class openshot.MappedMetadata. If so, then the rename to openshot.MetadataMap breaks that code, and you have my apologies.)

- src/bindings/ moves to /bindings/
- src/examples/ moves to /examples/
- Contents of include/ merged into src/ with same hierarchy
- src/Qt/demo/ moves to examples/qt-demo/
FindOpenShotAudio.cmake: Enhance with targets

- Also, migrate as much config as possible from CMakeLists.txt files
  to properties of IMPORTED OpenShot::Audio target (including platform-
  specific configs)
- Also, add missing ENABLE_COVERAGE option(), tweak option processing
Ruby: Relax SWIG version for compatibility check

- Turns out the Ruby-2.7.0-compatibility commit made it into SWIG 4.0.2

Ruby bindings: Fix all kinds of brokenness

- Turns out int64_t function args require stdint.i for auto-conversion.
- The imported module name is (still) 'Openshot' — lowercase 's'.
- #TIL that accidentally dropping the OUTPUT_NAME config leads to
  a non-loadable shared library, due to the filename being wrong.
- Parameters assigned to ImageMagick::Magick++ will now follow
  the 'openshot' shared-library target wherever it's linked, even in
  build subdirectories that aren't children or siblings of the
  location where the target was created.
- Removed the redundant code duplicating `find_package(ImageMagick)`
  checks. Multiple dependency scans have only been necessary as a
  workaround to overcome CMake variables' restricted scope.
  Targets do not share that limitation.
- FindOpenShotAudio takes over -DDEBUG, -DHAVE_ISFINITE logic
  (now set on OpenShot::Audio target when appropriate)
- Tweaks to BlackMagic dependency discovery
- Reverse the test-disabling logic, `DISABLE_TESTS FALSE` cache
  variable changed to `ENABLE_TESTS TRUE`
@ferdnyc ferdnyc added build Issues related to compiling or installing libopenshot and its dependencies code Source code cleanup, streamlining, or style tweaks labels Oct 19, 2020
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #582 into develop will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #582   +/-   ##
========================================
  Coverage    49.44%   49.45%           
========================================
  Files          129      129           
  Lines        10263    10261    -2     
========================================
  Hits          5075     5075           
+ Misses        5188     5186    -2     
Impacted Files Coverage Δ
examples/Example.cpp 0.00% <ø> (ø)
examples/ExampleHtml.cpp 0.00% <ø> (ø)
examples/qt-demo/main.cpp 0.00% <ø> (ø)
src/AudioBufferSource.cpp 0.00% <ø> (ø)
src/AudioDeviceInfo.h 0.00% <ø> (ø)
src/AudioReaderSource.cpp 0.00% <ø> (ø)
src/AudioReaderSource.h 0.00% <ø> (ø)
src/AudioResampler.cpp 0.00% <ø> (ø)
src/CacheBase.cpp 68.75% <ø> (ø)
src/CacheBase.h 75.00% <ø> (ø)
... and 118 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f9e4e2...9050cc7. Read the comment docs.

@jonoomph
Copy link
Member

@ferdnyc Not sure how this already has 2 conflicts, lol

@jonoomph
Copy link
Member

If we are gonna do it... then let's go for it. As mentioned before, I don't have any strong opinions related to where header files live. I know convention is strong, and the include folder is a common pattern, but I do agree it makes things simpler to keep them together, and distribute them the same as we do now. I like it! Let's merge.

@jonoomph
Copy link
Member

But I'll let you merge this one... so I can stand out of the blast radius. lol

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Oct 19, 2020

...WTH did I just do!?

@ferdnyc ferdnyc reopened this Oct 19, 2020
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Oct 19, 2020

Well, that was fun. Managed to briefly wipe out the entire branch with a bad git rebase --onto. All fixed now, but the branch will have to regenerate. I'll merge as soon as all the builds succeed.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Oct 19, 2020

All passed except the Gitlab Mac builder, which was stuck in previous jobs waiting on Apple. It's only just started building this branch now, but I'mma wait on it just to be really sure.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Oct 19, 2020

Huzzah! Here goes.... everything.

Boom. 💥

@ferdnyc ferdnyc merged commit 07a447c into develop Oct 19, 2020
@ferdnyc ferdnyc deleted the new-header-move branch October 22, 2020 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to compiling or installing libopenshot and its dependencies code Source code cleanup, streamlining, or style tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Developers RFC: Includes reorganization
3 participants