feat(build): Introduce API export macros via GenerateExportHeader#1139
Conversation
This commit introduces a standardized, cross-platform mechanism for managing API symbol visibility in shared libraries. It replaces previous custom macros with CMake's standard `GenerateExportHeader` module. A new `MLT_API` macro is now consistently applied to all public API declarations across the framework to ensure proper symbol visibility (`dllexport`/`dllimport`) on Windows. The SWIG interface file (`mlt.i`) has also been updated with `%define MLT_API` to maintain compatibility with the bindings generator. This change lays the groundwork for robust shared library builds on all platforms, particularly Windows, by isolating symbol visibility logic from other build configurations.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces standardized cross-platform API symbol visibility management for the MLT framework using CMake's GenerateExportHeader module. The changes focus on replacing manual export declarations with a systematic approach to handle shared library exports.
- Introduces CMake
GenerateExportHeadermodule to automatically create export headers - Applies
MLT_APImacro to all public API function declarations across framework headers - Updates SWIG interface to support the new export macro syntax
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/framework/CMakeLists.txt | Configures CMake to generate export header and install it |
| src/framework/mlt.h | Adds export header include to main framework header |
| src/framework/*.h | Applies MLT_API macro to all public function declarations |
| src/swig/mlt.i | Defines empty MLT_API macro for SWIG compatibility |
| src/win32/strptime.c | Adds export macro to Windows compatibility function |
| src/framework/mlt_consumer.c | Applies export macro to global mutex variables |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
CC @jlskuz |
src/framework/mlt_image.h
Outdated
| MLT_API int mlt_image_format_size(mlt_image_format format, int width, int height, int *bpp); | ||
| MLT_API void mlt_image_format_planes( |
There was a problem hiding this comment.
| MLT_API int mlt_image_format_size(mlt_image_format format, int width, int height, int *bpp); | |
| MLT_API void mlt_image_format_planes( | |
| MLT_DEPRECATED_EXPORT int mlt_image_format_size(mlt_image_format format, int width, int height, int *bpp); | |
| MLT_DEPRECATED_EXPORT void mlt_image_format_planes( |
| generate_export_header(mlt | ||
| BASE_NAME mlt | ||
| EXPORT_MACRO_NAME MLT_API | ||
| EXPORT_FILE_NAME mlt_export.h | ||
| DEPRECATED_MACRO_NAME MLT_DEPRECATED | ||
| NO_EXPORT_MACRO_NAME MLT_NO_EXPORT | ||
| STATIC_DEFINE MLT_STATIC_DEFINE | ||
| ) |
There was a problem hiding this comment.
No need to overwrite the defaults with default values:
| generate_export_header(mlt | |
| BASE_NAME mlt | |
| EXPORT_MACRO_NAME MLT_API | |
| EXPORT_FILE_NAME mlt_export.h | |
| DEPRECATED_MACRO_NAME MLT_DEPRECATED | |
| NO_EXPORT_MACRO_NAME MLT_NO_EXPORT | |
| STATIC_DEFINE MLT_STATIC_DEFINE | |
| ) | |
| generate_export_header(mlt | |
| EXPORT_MACRO_NAME MLT_API | |
| ) |
@ddennedy @bmatherly Any preference on the naming MLT_EXPORT (CMake's default) vs. MLT_API
There was a problem hiding this comment.
Also no preference from me. Are there any best practices or popular examples you can reference?
There was a problem hiding this comment.
I can tell you now that export is more common, but it is a bike shed topic.
There was a problem hiding this comment.
Please be considerate and not nitpick and turn petty things into committee decisions.
There was a problem hiding this comment.
Thinking more about this and backed by the new findings regarding export headers for every module I think it would be better to use MLT_EXPORT instead of MLT_API. Would you mind to change it?
There was a problem hiding this comment.
I agree if adding calls to generate_export_header for each module fixes it because it will make the names more consistent, less cmake lines, more clear.
|
Hi @jlskuz, thank you for the review! I've applied both of your suggestions in the latest commit. The generate_export_header call is now simplified, and I've marked the two functions in mlt_image.h as deprecated. |
ddennedy
left a comment
There was a problem hiding this comment.
When I checkout your branch in my msys2/MINGW64/Windows11 environment, I get the following compilation error:
[234/430 17.4/sec] Building CXX object src/modules/decklink/CMakeFiles/mltdecklink.dir/consumer_decklink.cpp.obj
FAILED: src/modules/decklink/CMakeFiles/mltdecklink.dir/consumer_decklink.cpp.obj
C:\msys64\mingw64\bin\g++.exe -DUSE_SSE -Dmltdecklink_EXPORTS -IC:/Projects/Shotcut/src/mlt/src/modules/decklink/win -IC:/Projects/Shotcut/src/mlt/src/framework/.. -IC:/Projects/Shotcut/src/mlt/build/Qt_6_x_MinGW_64_bit-RelWithDebInfo/src/framework -O2 -g -DNDEBUG -std=c++20 -fdiagnostics-color=always -mmmx -msse -msse2 -MD -MT src/modules/decklink/CMakeFiles/mltdecklink.dir/consumer_decklink.cpp.obj -MF src\modules\decklink\CMakeFiles\mltdecklink.dir\consumer_decklink.cpp.obj.d -fmodules-ts -fmodule-mapper=src\modules\decklink\CMakeFiles\mltdecklink.dir\consumer_decklink.cpp.obj.modmap -MD -fdeps-format=p1689r5 -x c++ -o src/modules/decklink/CMakeFiles/mltdecklink.dir/consumer_decklink.cpp.obj -c C:/Projects/Shotcut/src/mlt/src/modules/decklink/consumer_decklink.cpp
In file included from C:/Projects/Shotcut/src/mlt/src/framework/mlt_factory.h:26,
from C:/Projects/Shotcut/src/mlt/src/framework/mlt.h:45,
from C:/Projects/Shotcut/src/mlt/src/modules/decklink/consumer_decklink.cpp:22:
C:/Projects/Shotcut/src/mlt/src/framework/mlt_repository.h:53:37: error: function 'void mlt_register(mlt_repository)' definition is marked dllimport
53 | #define MLT_REPOSITORY MLT_API void mlt_register(mlt_repository repository)
| ^~~~~~~~~~~~
C:/Projects/Shotcut/src/mlt/src/modules/decklink/consumer_decklink.cpp:1255:1: note: in expansion of macro 'MLT_REPOSITORY'
1255 | MLT_REPOSITORY
| ^~~~~~~~~~~~~~
If I turn off that module, the problem occurs again in src/modules/rtaudio/consumer_rtaudio.cpp. Something breaks building C++ modules in Windows.
|
|
||
| /** A convenience macro to create an entry point for service registration. */ | ||
| #define MLT_REPOSITORY void mlt_register(mlt_repository repository) | ||
| #define MLT_REPOSITORY MLT_API void mlt_register(mlt_repository repository) |
There was a problem hiding this comment.
To fix the problem Dan faced I believe we need to revert the change in this line and add export headers for every module and use them like
MLTFREI0R_EXPORT MLT_REPOSITORY
| generate_export_header(mlt | ||
| BASE_NAME mlt | ||
| EXPORT_MACRO_NAME MLT_API | ||
| EXPORT_FILE_NAME mlt_export.h | ||
| DEPRECATED_MACRO_NAME MLT_DEPRECATED | ||
| NO_EXPORT_MACRO_NAME MLT_NO_EXPORT | ||
| STATIC_DEFINE MLT_STATIC_DEFINE | ||
| ) |
There was a problem hiding this comment.
Thinking more about this and backed by the new findings regarding export headers for every module I think it would be better to use MLT_EXPORT instead of MLT_API. Would you mind to change it?
This pull request introduces a standardized, cross-platform mechanism for managing API symbol visibility in shared libraries, strictly focusing on this single concern.
Key Changes:
GenerateExportHeader: The build system now uses CMake's standardGenerateExportHeadermodule to create a header that manages symbol visibility. This replaces the previous hand-written macro.MLT_APIMacro Application: A newMLT_APImacro, generated by the module, has been applied to all public API declarations throughout the framework. This ensures properdllexport/dllimportbehavior for shared library builds on Windows.mlt.i) is updated with%define MLT_APIto ensure the bindings can be generated without syntax errors.All other changes related to
CMakePresets.json, new dependencies, or vcpkg-specific functions have been removed to keep this PR focused. They will be submitted in separate, subsequent pull requests.This foundational change makes it easier to review and merge future platform-specific improvements.