CMake: actually fix ENABLE_SYSTEM_GMIC #175
Conversation
PR c-koi#172 is correct in that it is no longer possible to build without -Dgmic_core, due to G'MIC-Qt relying in now-private implementation details of G'MIC; namely, the extensions made in gmic.cpp to CImg. However, the solution chosen has significant shortcomings: 1. It blindly assumes that the consumed library has been built by a GCC-compatible compiler. This is easily inferred from the lack of symbol exports in {CImg,gmic}.{h,cpp}. 2. It makes no provision for the exported library type; G'MIC can be built statically or dynamically. 3. In Windows, when built with MSVC, the kind of symbol export that gmic_core implies is only available with a static libgmic. To fix this, this commit augments G'MIC-Qt's `ENABLE_DYNAMIC_LINKING` handling with target detection code for the above described cases. In the case where a compatible library is not found, a fallback is specified that will build libgmic as a separate target, then make G'MIC-Qt link against it in order to mimic the requirements. If a suitable system library is found, we augment it with the gmic.cpp plugin to make the symbols visible at compile time.
|
Thank you for this! Does this still require having the G'MIC source files to build gmic-qt? If so, I just took a look at the Arch pkgbuilds for it and we can probably use this and work around it similarly to those if we really need |
|
@lilyinstarlight by using the gmic-qt tarball, and patching this PR in, you should be ready to go. I use this approach to ship the Krita plugin (and will cut a patch once I get confirmation from @antonio-rojas). |
Oh lovely! I'll give it a try then today (nixpkgs is fetching from github rather than the tarball and I'm really not sure why, so I'll probably fix that too) |
|
Oh, I think I understand now. This looks like it does still require gmic-qt to build with the gmic source available (in fact it looks like this PR makes that more mandatory than before by including We can probably build like that then if we have to. We build gmic and gmic-qt from separate source trees right now, but if upstream is not supporting that going forward then we can adapt our packages to build them from the tarball that has both sources combined |
|
Okay so with switching gmic-qt to build from the gmic tarball, applying the patch from this PR, and modifying the cmakelists file in the gmic repo like arch does (and which I'll PR upstream), we seem to be able to build against the shared lib again Thank you again! I'll go close the other PR since it's more of a hack than anything and we now have a way forward for building G'MIC-Qt in nixpkgs |
How the build was fixed: * Fetch tarball instead since that is what upstream supports to build gmic-qt from. * Set `sourceRoot` within tarball * Fetch patch from GreycLab/gmic#435 into gmic * Fetch patch from c-koi/gmic-qt#175 into gmic-qt
| elseif(TARGET libgmic) | ||
| set(gmic_qt_LIBRARIES | ||
| ${gmic_qt_LIBRARIES} | ||
| libgmic |
There was a problem hiding this comment.
Good catch, looks like it was not actually linking the exported targets before:
| add_library(libgmicstatic STATIC ../src/gmic.cpp) | ||
| target_include_directories(libgmicstatic PUBLIC ../src) | ||
| # We need internal access into the gmic-core API. | ||
| target_compile_definitions(libgmicstatic PUBLIC gmic_core) | ||
| set_target_properties(libgmicstatic | ||
| PROPERTIES | ||
| AUTOMOC OFF | ||
| ) | ||
| target_link_libraries(libgmicstatic PUBLIC | ||
| ${PNG_LIBRARIES} | ||
| ${FFTW3_LIBRARIES} | ||
| ${ZLIB_LIBRARIES} | ||
| ${CURL_LIBRARIES} | ||
| ${EXTRA_LIBRARIES}) |
There was a problem hiding this comment.
I wonder if we shipped the GMic CMake files in the tarball, we could just rely on the targets that are already defined there.
There was a problem hiding this comment.
@dtschump has actually removed CMake support in G'MIC, so using them will court a second PR to remove them altogether here in the future.
There was a problem hiding this comment.
@dtschump has actually removed CMake support in G'MIC, so using them will court a second PR to remove them altogether here in the future.
Not completely true. I just don't want to maintain it (I am not interested in this build system). Last time I removed the CMakeLists from the official tarball was because it was not working anymore due to the changes I made on the source code. I'd be happy to put it back if I'm sure it works as expected (i.e. just as the Makefile does). I don't want to get negative feedback from users (that I couldn't handle) because of a non-functional CMakeLists.
There was a problem hiding this comment.
@dtschump by removing (support for) CMake, you also cease to support non-Unix operating systems like Windows. This means that to compile, you need a full blown MSYS shell or at the very least a MinGW toolchain on your path.
Moreover, the same applies with the recent translations regeneration scripts. These hard depend on a Bourne shell being available, which is necessary to trigger the regeneration of the filter translations.
There was a problem hiding this comment.
@amyspark , as I said before, I'm ok to put a CMakeList back in the source tarball if I'm sure it's not broken and that G'MIC compiles just fine with it. It's just a matter of having a maintainer who takes care of translating any changes in the Makefile into the CMakeList.
Concerning the translations, we also put all the necessary files in the provided source tarball to compile G'MIC:
- Current stable : https://gmic.eu/files/source/gmic_3.2.1.tar.gz
- Current development : https://gmic.eu/files/prerelease/gmic_prerelease.tar.gz
In any case, we consider the tarball file as the "full" sources of G'MIC. The git repo is just a working space to develop G'MIC, it's not intended to be a "ready-as-it-is" version of the sources.
If you want to compile G'MIC for Krita, then it would be better to fetch the G'MIC sources from the tarball rather than from the git repo.
There was a problem hiding this comment.
BTW, I'm talking about the tarball from the G'MIC web page, not those automatically provided by the github "releases" section.
How the build was fixed: * Fetch tarball instead since that is what upstream supports to build gmic-qt from. * Set `sourceRoot` within tarball * Fetch patch from GreycLab/gmic#435 into gmic * Fetch patch from c-koi/gmic-qt#175 into gmic-qt
How the build was fixed: * Fetch tarball instead since that is what upstream supports to build gmic-qt from. * Set `sourceRoot` within tarball * Fetch patch from GreycLab/gmic#435 into gmic * Fetch patch from c-koi/gmic-qt#175 into gmic-qt
How the build was fixed: * Fetch tarball instead since that is what upstream supports to build gmic-qt from. * Set `sourceRoot` within tarball * Fetch patch from GreycLab/gmic#435 into gmic * Fetch patch from c-koi/gmic-qt#175 into gmic-qt
How the build was fixed: * Fetch tarball instead since that is what upstream supports to build gmic-qt from. * Set `sourceRoot` within tarball * Fetch patch from GreycLab/gmic#435 into gmic * Fetch patch from c-koi/gmic-qt#175 into gmic-qt
The previous tarball src sometimes gets mutated (see e.g. [this comment][1]). This was changed from the Git src in fd3e2b4 (see also [upstream discussion][2]). However the delta seems simple; it had error: ``` In file included from /build/source/src/GmicProcessor.cpp:48: /nix/store/jk1dp7v01pisw0flybqwyjg63in6r9fp-gmic-3.3.5-dev/include/gmic.h:191:21: fatal error: gmic.cpp: No such file or directory 191 | #define cimg_plugin "gmic.cpp" ``` workaround this by linking `gmic.cpp` into the expected location as it appears in the tarball. Remove the now-unneeded updateScript, as it is trivial GitHub tags now. Unclear to me why cimg was not needed in buildInputs before, but it is now. This may appear to be downgrading, but this is only because the previous drv used the gmic version, not the gmic-qt version. [1]: NixOS#311734 (comment) [2]: c-koi/gmic-qt#175
The previous tarball src sometimes gets mutated (see e.g. [this comment][1]). This was changed from the Git src in fd3e2b4 (see also [upstream discussion][2]). However the delta seems simple; it had error: ``` In file included from /build/source/src/GmicProcessor.cpp:48: /nix/store/jk1dp7v01pisw0flybqwyjg63in6r9fp-gmic-3.3.5-dev/include/gmic.h:191:21: fatal error: gmic.cpp: No such file or directory 191 | #define cimg_plugin "gmic.cpp" ``` workaround this by linking `gmic.cpp` into the expected location as it appears in the tarball. cimg is now needed in buildInputs as it is bundled in the tarball, but not the Git src. Change the updateScript to simpler one that can use the Git tags. This may appear to be downgrading, but this is only because the previous drv used the gmic version, not the gmic-qt version. [1]: NixOS#311734 (comment) [2]: c-koi/gmic-qt#175
PR #172 is correct in that it is no longer possible to build without
-Dgmic_core, due to G'MIC-Qt relying in now-private implementation details of G'MIC; namely, the extensions made in gmic.cpp to CImg.However, the solution chosen has significant shortcomings:
To fix this, this commit augments G'MIC-Qt's
ENABLE_DYNAMIC_LINKINGhandling with target detection code for the above described cases. In the case where a compatible library is not found, a fallback is specified that will build libgmic as a separate target, then make G'MIC-Qt link against it in order to mimic the requirements. If a suitable system library is found, we augment it with the gmic.cpp plugin to make the symbols visible at compile time.This is a less-invasive alternative to #174 that fixes building on MSVC too.
Closes #174