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

No longer require MODULE_EXPORTS and fix related issues #435

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

marktsuchida
Copy link
Member

@marktsuchida marktsuchida commented Jan 24, 2024

MMDevice (in ModuleInterface.h) conditionally defined MODULE_API based on whether MODULE_EXPORTS was defined. The intent was (presumably) that device adapters would define MODULE_EXPORTS and therefore expose the module interface functions such as InitializeModuleData() (defined by the device adapter) and GetNumberOfDevices() (defined in ModuleInterface.cpp), whereas MODULE_EXPORTS would not be defined when building MMCore so that those functions are not included.

In practice, there were a few issues:

  • In the Visual Studio build, we use the same MMDevice static library for both MMCore and device adapters. This static library was built with MODULE_EXPORTS, so the device module interface functions were being included in MMCoreJ_wrap.dll (this is harmless).
  • When MODULE_EXPORTS was not defined, MODULE_API was incorrectly defined to __declspec(dllimport), which is irrelevant when libraries are loaded via LoadLibrary(). This also meant that ModuleInterface.cpp did not compile on Windows without MODULE_EXPORTS defined.
  • In the Autotools build, MODULE_EXPORTS was never defined in any case (and had no effect either). The module interface functions were being included in libMMCoreJ_wrap (again, harmless).
  • The macro name MODULE_EXPORTS is too generic.
  • It would make more sense to make the device adapter case the default, and only define a macro when building MMCore.

So this PR replaces MODULE_EXPORTS with another macro, MMDEVICE_CLIENT_BUILD, with roughly the opposite meaning. Device adapters no longer need to define any macro, whereas building MMCore should define MMDEVICE_CLIENT_BUILD.

In addition, MODULE_API is now defined for GCC/Clang to explicitly give the functions default visibility, so that we can use -fvisibility=hidden when building device adapters in the future (doing so has a number of advantages but is not urgent).

To work around the way our Visual Studio and Autotools builds work (i.e., the common MMDevice static library), it is still possible to build MMCore using an MMDevice static library built without MMDEVICE_CLIENT_BUILD, as long as MMDEVICE_CLIENT_BUILD is defined when including the MMDevice headers during the MMCore build. This is similar to how MODULE_EXPORTS was previously being used on Windows. MMCoreJ_wrap will expose unnecessary symbols, but this is the same as the previous situation.

Note that device adapters (including any that are outside of this repo) do not need to change: defining MODULE_EXPORTS now has no effect but also does not do any harm.

It would be nice to also rename MODULE_API to something like MMDEVICE_API, but that would require changing the source code of all device adapters; probably not worth the disruption at this time. I did make it so that MODULE_API is not defined when MMDEVICE_CLIENT_BUILD is defined.

Finally, none of this affects the device interface version.


Manual testing

  • Builds on Windows/vcxproj
    • DemoCamera exports CreateDevice
  • Builds on macOS/Autotools
    • DemoCamera exposes CreateDevice
  • Builds on Linux/Autotools
    • DemoCamera exposes CreateDevice
  • pymmcore (setuptools) passes import test (with MMDEVICE_CLIENT_BUILD added)

This uses obsolete CMake constructs (not "modern CMake"). And it will
(continue to) bit-rot in the absense of regular testing.
MODULE_EXPORTS is defined in MMDeviceAdapter.props, but some device
adapter projects (old ones, and copied or cargo-culted ones) redundantly
defined it.

```sh
find DeviceAdapters -name '*.vcxproj' -print0 \
    | xargs -0 gsed -i 's/MODULE_EXPORTS;//'
```

(`gsed` being GNU sed)
The new macro has (roughly) the opposite meaning as the (intent of)
MODULE_EXPORTS: it should be defined when building MMCore and undefined
when building device adapters.

The source is now _capable_ of excluding the module interface functions
when building MMCore (and the MMCore Meson build does so).

Symbol visibility is set for GCC/Clang so that -fvisibility=hidden can
be used when building device adapters (but this is not done for now).

No changes to device adapters is required.
@marktsuchida marktsuchida marked this pull request as ready for review January 24, 2024 20:01
@marktsuchida marktsuchida merged commit ff6ba58 into main Jan 24, 2024
3 checks passed
@marktsuchida marktsuchida deleted the mmdevice-fix-module-exports branch January 24, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant