c-koi / gmic-qt

G'MIC-Qt is a versatile front-end to the image processing framework G'MIC.
GNU General Public License v3.0
206 stars 53 forks source link

Fix ENABLE_SYSTEM_GMIC #174

Closed lilyinstarlight closed 1 year ago

lilyinstarlight commented 1 year ago

We need to include CImg.h when gmic_core is not defined so that we can work on only shared libs and header files rather than requiring gmic.cpp to exist somewhere on the system

This reverts #172 because gmic_core is fundamentally incompatible with ENABLE_SYSTEM_GMIC because it requires having the G'MIC source rather than just the installed system files

This is implemented via a header indirection to detect when gmic_core is undefined and includes CImg.h similar to how gmic.h does when gmic_core is defined

@stefantalpalaru thoughts?

stefantalpalaru commented 1 year ago

it requires having the G'MIC source rather than just the installed system files

Not a problem for my Gentoo ebuild, because I combine gmic and gmic-qt in the same package: https://github.com/stefantalpalaru/gentoo-overlay/blob/9d7fe8de785526a7b9db12943f3bd59d3ef3b98d/media-gfx/gmic/gmic-3.2.0-r100.ebuild

That said, I tested your patch and it works as advertised. The only problem is that things will break when G'MIC and CImg APIs will diverge and there is no automated testing to catch it.

We can't really force upstream to support CImg if they don't want to.

lilyinstarlight commented 1 year ago

That said, I tested your patch and it works as advertised. The only problem is that things will break when G'MIC and CImg APIs will diverge and there is no automated testing to catch it.

The trouble is the G'MIC API with gmic_core enabled is just more or less a renamed CImg API (I mean there are references to gmic_library::cimg in gmic-qt even)

With gmic_core disabled, it provides a smaller API that is far too incomplete because gmic-qt expects to have the CImg API in the gmic_library namespace which is way more than the small stubs provide

If upstream has some other solution they would prefer (e.g. expanding the smaller API when gmic_core is not defined or allowing to build with gmic_core with only installed system files), then I'm happy to use anything that just actually fixes the build too

Let me know if I'm misunderstanding any of the internals between gmic-qt/G'MIC/CImg because I really only got acquainted with them over the last week or two

Thanks!

dtschump commented 1 year ago

I'm currently away for one week. I'll see what to do when I return. Thanks !

Le lun. 20 févr. 2023 à 12:42, Lily Foster @.***> a écrit :

That said, I tested your patch and it works as advertised. The only problem is that things will break when G'MIC and CImg APIs will diverge and there is no automated testing to catch it.

The trouble is the G'MIC API with gmic_core enabled is just more or less a renamed CImg API (I mean there are references to gmic_library::cimg in gmic-qt even)

With gmic_core disabled, it provides a smaller API that is far too incomplete because gmic-qt expects to have the CImg API in the gmic_library namespace which is way more than the small stubs provide

If upstream has some other solution they would prefer (e.g. expanding the smaller API when gmic_core is not defined or allowing to build with gmic_core with only installed system files), then I'm happy to use anything that just actually fixes the build too

Let me know if I'm misunderstanding any of the internals between gmic-qt/G'MIC/CImg because I really only got acquainted with them over the last week or two

Thanks!

— Reply to this email directly, view it on GitHub https://github.com/c-koi/gmic-qt/pull/174#issuecomment-1436829093, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNFMBPW5XOQ3CBU5HXDNA3WYNKBLANCNFSM6AAAAAAVBH4OAE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

lilyinstarlight commented 1 year ago

Closing in favor of #175 and opening one more PR in the gmic repo