| Summary: | External libclapack not used [patch] | ||
|---|---|---|---|
| Product: | [Applications] digikam | Reporter: | nucleo <nucleo> |
| Component: | Portability-Runtime | Assignee: | Digikam Developers <digikam-bugs-null> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | caulier.gilles, kdebugs.20.orzelf, kevin.kofler, marcel.wiesweg, rdieter, vivo75+kde |
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | unspecified | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/digikam/84d745a59bd0f9d9697b50b9f8558d368d956faf | Version Fixed In: | 3.2.0 |
| Sentry Crash Report: | |||
| Attachments: | patch fixes build against external libclapack | ||
Created attachment 69328 [details]
patch fixes build against external libclapack
With this pacth digikam builds against external libclapack but in Help - Components still shown that it is internal (the same with external libpgf).
This is just a firs try to use atlas instead, and is probably not usable directly as is. But if something like this is agreeable in principle, we could work to add conditionals to support both lapack and atlas. (and in a perfect world, move torward using something like eigen instead, but that's even more work... :) ) I don't think this patch can be merged as is, without adding some conditional to support both vanilla clapack and ATLAS's version of clapack which is not quite the same thing. (We're using ATLAS's version in Fedora, we don't have a system package of vanilla clapack.) And what about porting the code to use Eigen instead? It's the preferred solution for linear algebra in the KDE world and it can do this simple operation (dgesv is just solving a linear system of equations by straightforward LU decomposition). any patch is welcome ;-) To all, Please look patch attached to this file about eigen support : https://bugs.kde.org/show_bug.cgi?id=251563 Gilles Caulier I would like to remove internal clapack from digiKam core. If i understand all the way, we have two possibilities : 1/ Using Atlas shared library (see patch from this file). 2/ Using Eigen shared library (see patch from bug #251563). For both solution, only relevant plugin must be not compiled. Only Refocus tool from editor and BQM use it - see Sharpen plugin. This dependency must be optional. I don't see this behavior in patches. Also, what's the best solution : Atlas or Eigen ? As Francesco said in the past, Eigen has a lots of supplementary dependencies. Gilles Caulier Hello. I'm the author of the "eigen" patch, and also a developer on eigen. Just to fix a few points : 1) i don't know who francesco is and where he made the statement about dep, but it's false. Citing our homepage http://eigen.tuxfamily.org "Eigen doesn't have any dependencies other than the C++ standard library." 2) it seems there's a misunderstanding about eigen. Eigen is based on templates, and as such is NOT a library. It's not something you linke statically or dynamically. You only need to have eigen include files at compile time, and this will generate only the needed code for your application. 3) i dont remember about my patch (it was submitted very long ago and not accepted as is if i remember well), but it's probably true that it didn't include the relevant part to make eigen support optional. But this is probably very easy to do, and more easily done by someone used to digikam code (and its build system). 4) You seem to be very "cold"/"shy" with using eigen , not to say frightened. Eigen has its roots in KDE (was started and hosted here), and, more importantly, eigen is way, way, better than any such library. I can say it , i'm not an initial author. Whatever your criteria are (fiability, speed, dependencies, hope of being maintained in 10 years, efficiency, beauty of code, beauty of API, friendliness of authors, and even closeness to KDE). I did choose eigen long ago after having made a HUGE comparisons of all existing stuff, and it was not only the best, but beating all others with a very large margin. You can't make a bad choice going for them. I know the KDE team just love making plugins and allowing different backends and such. But here, it's just useless and a waste of time, imho. 1/ ==> Lok this comment : https://bugs.kde.org/show_bug.cgi?id=251563#c13 All things being equal feature-wise, eigen is definitely the way to go (imho) Rex, If i understand well comment #7 from Thomas, Eigen is already used by KDE core ? If yes, where exactly ? For which components, and which features ? Gilles Caulier kdeplasma-addons(wallpapers/mandelbrot), kdeartwork(krotation,kpendulum screenssavers) core modules use eigen2 Mind you, it's a build-time dependency only, so nice that it adds nothing at runtime. Thanks Rex, So, Eigen is the right way to go... Gilles (In reply to comment #8) > 1/ ==> Lok this comment : https://bugs.kde.org/show_bug.cgi?id=251563#c13 Oh, this one is not about eigen having big dependencies. But the fact that some modules in KDE still use eigen2 while the current version of eigen is 3. This is no great deal. Actually, we (the eigen team) have access to KDE code and we planned to port those to eigen3, but we kinda failed to find the time to do so. Though i'm pretty sure there's no much work to do, if ever. Eigen 2 and 3 API are very similar, and eigen 3 is more about adding new features. Sure, few API's were obsolated, but not much iirc. Just to clarify. Francesco was saying that it was better to stay with eigen2, and my patch on this other bug is against eigen2. So everything seems... just fine. isn't it ? As the author of the patch which started this thread, I'd also go with Eigen (in fact, I wrote https://bugs.kde.org/show_bug.cgi?id=251563#c11 a year ago, but didn't get around to updating the Fedora package to pick up the changed patch). I did the minimum change to get rid of the bundled library, but observed right when I wrote it that it needs only one LAPACK function, so it'd be fairly easy to use something different. Eigen seems a better fit for KDE than clapack, and it also avoids the ugly compatibility issues between different clapack implementations (reference clapack vs. ATLAS). As for what version, I'd say the latest version (Eigen 3) would be the better choice, it's not so nice to be stuck on an old version. (In reply to comment #6) > I would like to remove internal clapack from digiKam core. > > If i understand all the way, we have two possibilities : > > 1/ Using Atlas shared library (see patch from this file). > 2/ Using Eigen shared library (see patch from bug #251563). > > For both solution, only relevant plugin must be not compiled. Only Refocus > tool from editor and BQM use it - see Sharpen plugin. This dependency must > be optional. I don't see this behavior in patches. > > Also, what's the best solution : Atlas or Eigen ? As Francesco said in the > past, Eigen has a lots of supplementary dependencies. > > Gilles Caulier Gilles, is the other way around those are the packages that already DEPEND ON eigen, it was supportive. Sorry again for my sucking english :-( The only problem I could see at the time is that eigen2 was used by most application and the upgrade to eigen3 was slow. Hopefully this has been solved by now. Cheers Another question : Currently, Atlas library is checked to find a external clapack implementation from host computer. If nothing is found, internal implementation from digiKam core is used instead. If internal implementation is removed, and Eigen check added, Do you think that both Eigen and Atlas checks must still used, or only Eigen (optional Altlas dependency removed in this case) ? Gilles Caulier My point of view is that you should only use/depend on eigen. Remember it's a compile-time only dependancy. That would mean * remove the internal lapack copy * remove the external lapack library check and related logic * the eigen check is included in my patch I'd say the same, just Eigen should be enough. And by the way, I'd personally not bother making it optional. In fact, as a distro packager, I don't like optional dependencies at all, I want my build to fail hard if I'm missing a BuildRequires, not silently build a crippled package. PS (and sorry for the 3 posts in sequence): (and for us, even printing a warning to stdout or stderr at build time is still "silent", we rarely look at build logs for successful builds) (In reply to comment #20) > And by the way, I'd personally not bother making it optional. In fact, as a > distro packager, I don't like optional dependencies at all, I want my build > to fail hard if I'm missing a BuildRequires, not silently build a crippled > package. Kevin what you want is a package that fail hard if a *requested* option miss a dependancy. If for whatever reason someone don't need that function it could still be a good thing being able to remove it. However if unwanted the function (and dep.) should NOT magically being added That said in this case depend unconditionally upon eigen seem a good thing since it already work on all digikam supported platforms, it's a build time dependancy and what use it is almost universally useful. just 2c > Kevin what you want is a package that fail hard if a *requested* option miss a dependancy.
Unfortunately, that's not good enough (and it's a common mistake made by developers, and in fact the whole optional dependency system in KDE gets that wrong). The package should fail hard if I did NOT explicitly request that I DON'T want the dependency. Everything not explicitly disabled should be required. Otherwise, when you add some new functionality, it is still likely to be missing in packages, because if we don't know to add the BuildRequires, we also won't know about the -D switch to add to explicitly request the feature. Opt-in requesting is entirely useless for packaging, we already opt in by adding a BuildRequires to our packages, it needs to be opt-out!
I think we should probably have some "packager mode" in CMake (a -D flag we could add to our %cmake RPM macro) where all the optional dependencies are required unless explicitly disabled. But this is getting way out of the scope of this bug. ;-)
(In reply to comment #23) Fair enough, only thing yet uncovered are mutually exclusive options. |
User-Agent: Opera/9.80 (X11; Linux i686; U; ru) Presto/2.10.229 Version/11.61 Build Identifier: External libclapack not detected without this patch: diff -ur digikam-2.1.1/core/cmake/modules/FindCLAPACK.cmake digikam-2.1.1-clapack-atlas/core/cmake/modules/FindCLAPACK.cmake --- digikam-2.1.1/core/cmake/modules/FindCLAPACK.cmake 2011-09-14 08:22:02.000000000 +0200 +++ digikam-2.1.1-clapack-atlas/core/cmake/modules/FindCLAPACK.cmake 2011-09-23 07:09:37.000000000 +0200 @@ -27,7 +27,7 @@ endif(CLAPACK_INCLUDE_DIR) endif(CLAPACK_INCLUDE_DIR) - find_library(CLAPACK_LIBRARY clapack) + find_library(CLAPACK_LIBRARY clapack PATH_SUFFIXES atlas) if(CLAPACK_LIBRARY) message(STATUS "Found clapack library: ${CLAPACK_LIBRARY}") endif(CLAPACK_LIBRARY) diff -ur digikam-2.1.1/core/libs/dimg/filters/sharp/matrix.cpp digikam-2.1.1-clapack-atlas/core/libs/dimg/filters/sharp/matrix.cpp --- digikam-2.1.1/core/libs/dimg/filters/sharp/matrix.cpp 2011-09-14 08:22:03.000000000 +0200 +++ digikam-2.1.1-clapack-atlas/core/libs/dimg/filters/sharp/matrix.cpp 2011-09-23 10:19:09.000000000 +0200 @@ -36,7 +36,6 @@ extern "C" { -#include "f2c.h" #include "clapack.h" } @@ -653,14 +653,12 @@ int RefocusMatrix::dgesv(const int N, const int NRHS, double* A, const int lda, double* B, const int ldb) { - int result = 0; - integer i_N = N, i_NHRS = NRHS, i_lda = lda, i_ldb = ldb, info; - QScopedArrayPointer<integer> ipiv(new integer[N]); + int result; + QScopedArrayPointer<int> ipiv(new int[N]); // Clapack call. - dgesv_(&i_N, &i_NHRS, A, &i_lda, ipiv.data(), B, &i_ldb, &info); + result = clapack_dgesv (CblasColMajor, N, NRHS, A, lda, ipiv.data(), B, ldb); - result = info; return (result); } Reproducible: Always Actual Results: Expected Results: