Skip to content

Pass log messages from projectM to Poco's logger - #132

Open
SwooshyCueb wants to merge 2 commits into
projectM-visualizer:masterfrom
SwooshyCueb:projectm-logging
Open

Pass log messages from projectM to Poco's logger#132
SwooshyCueb wants to merge 2 commits into
projectM-visualizer:masterfrom
SwooshyCueb:projectm-logging

Conversation

@SwooshyCueb

Copy link
Copy Markdown

No description provided.

@kblaschke kblaschke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is certainly a great addition, this code won't compile/link with all libprojectM versions, as the logging API is a new feature in the unreleased 4.2 version.

At the very least, the projectM version should be checked at compile time and then enable certain features according to the libprojectM API version the app is being built against. This is to make sure the application can be built & used on as many platforms as possible, including those not shipping the latest libprojectM (LTS distros for example won't update libraries to higher versions, only releasing bug and security fixes).

There are different ways of implementing such a check, all of which will include wrapping code into #ifdef/#if macros:

  • Comparing the projectM major/minor version in #if (PROJECTM_VERSION_MAJOR >= 4) && (PROJECTM_VERSION_MINOR >=2) preprocessor directives
  • Let CMake check for each function to be available using the CheckFunctionExists module and including the projectM header.
  • Determine the projectM version in CMake (via the projectM4_VERSION CMake variable defined by the package) and set compiler defines for each minor version available in the one used for building.

I don't think the built app should be backwards-compatible at runtime (e.g. replacing the projectM shared library from the build with an older one), as this would imply lots of additional work in the code and runtime checks for available functions. This won't be a use case, and thus only forward compatibility is needed.

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.

2 participants