Bug 283650 - panorama tool can't find cpfind version on OS X
Summary: panorama tool can't find cpfind version on OS X
Status: RESOLVED FIXED
Alias: None
Product: digikam
Classification: Applications
Component: Plugin-Generic-Panorama (show other bugs)
Version: unspecified
Platform: MacPorts macOS
: NOR normal
Target Milestone: ---
Assignee: Digikam Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-09 15:32 UTC by brad
Modified: 2016-07-10 05:33 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.4.0


Attachments
adds code to parse the first and second line of output of cpfind --version (468 bytes, patch)
2011-10-09 15:32 UTC, brad
Details

Note You need to log in before you can comment on or make changes to this bug.
Description brad 2011-10-09 15:32:11 UTC
Created attachment 64364 [details]
adds code to parse the first and second line of output of cpfind --version

Version:           2.2.0 (using KDE 4.7.1) 
OS:                OS X

When digikam tries to determine the version of the cpfind binary needed for the pano stitching tool, it only parses the first line of output of 'cpfind --version', this does not work on OS X.

For some reason, on OS X, the output of cpfind --version is the following, with the version information appearing on the second line.
    ThreadQueue created
    Hugins cpfind 2011.0.1 built by Harry van der Wolf
    based on Pan-o-matic by Anael Orlinski
    ...


Reproducible: Didn't try

Steps to Reproduce:
simply try pano stiching tool on OS X.

Actual Results:  
digikam can't determine cpfind version and fails. 

Expected Results:  
to find correct version info.

a small patch should be attached.
Comment 1 Benjamin Girault 2011-10-09 16:22:04 UTC
This is a bug in the package (port) of Hugin for Mac OS X. There should not be any debug printed by the ZThread version of Hugin.

Please contact the maintainer of this port and ask about that (I have no idea how to enable, and therefore to disable, debug in hugin's ZThread, and do not know the Macports).
Comment 2 brad 2011-10-09 16:28:27 UTC
The patched attached in the initial report, fixes the bug. It is a trivial fix in digikam/kipi's code. Why not commit the patch?
Comment 3 Benjamin Girault 2011-10-09 17:38:11 UTC
This is not a real solution, but a hack, and I already have produced a patch for the packagers (see bug #280736).

My point is that if one cannot rely on the output of a program, one cannot parse it efficiently (I am thinking about the future of the panorama tool).
Comment 4 brad 2011-10-09 17:53:11 UTC
Understood. You're right, parsing output can be tricky. 

I will try to come up with a more flexible solution in the coming days. I'm thinking a simple regex across the entire output of cpfind --version will do the trick. Of course i will try to make a solution that could be adapted to the other binaries necessary for panos too. 

Does that sound like a reasonable solution?
Comment 5 Benjamin Girault 2011-10-09 18:51:42 UTC
I already investigated this bug and concluded that the problem is not the plugin but Hugin's packages (some of them).

In fact, hugin is using ZThread, and bundles it. However, for some reason that I am not aware of, some packages are built with debug support of the ZThreads, which introduces some extra output, and in particular the line "ThreadQueue created", which then mess up the whole parsing. In the configure script of the ZThreads (the official package), one can enable the debug with "--enable-debug", but it is disabled by default.

I am not completely against patching, but IMHO that doesn't make sense for two reasons: a package is something that should be stable, and therefore without extra debug output, and the panorama plugin is not meant to test Hugin's tools, such that support for Hugin's debug output doesn't make any sense.

The patch I provided is more efficient than yours (it tests the second line rather than the first, instead of testing both), so anyone who wants to build the plugin from the source can just grab it and patch. On the other hand, packagers should take care of Hugin's packaging and fix this debug, IMHO.
Comment 6 brad 2011-10-10 21:47:04 UTC
I agree with your sentiment on principle; that is, output should be stable, reliable and consistant. However in practice, i know that parsing output is always going to be a moving target; output changes, whether intentionally or unintentionally; that will trip up programs like digikam trying to parse output. As a result, i believe the code to parse this output would improve over time. Also, any disruptions to the output are completely out of our control, whereas the code to parse the output is completely in our control; so it is much easier to alter digikam's code than to alter another project's code or another project's packaging system. Ultimately, this is your code and the decision to patch is yours, but this bug will persist. 

Below is a version of the checkSystem() function for cpfindbinary.cpp, which handles and parses the out with a regular expression regardless of where the version information occurs in the output. It is not as straightforward as i hoped as QT doesn't appear to allow for variable names of captured text, but the code is concise (less than 10 lines of functional code for the whole function). 

Alternatively, you could simply parse the output of 'cpfind -h |grep -v -i thread' which would eliminate all the ZThread output.


#include <QRegExp>

...

void CPFindBinary::checkSystem()
{
    QProcess process;
    process.start(path(), QStringList() << "-h");
    m_available       = process.waitForFinished();                                        

    QRegExp versionRegExp(".*Hugins cpfind (?:Pre-Release )?((\\d+){1}.(\\d+){1}.(\\d+){0,1}.([0-9a-fxA-FX]+){0,1}).*", Qt::CaseInsensitive);

    if (versionRegExp.exactMatch(process.readAllStandardOutput()))                        
    {                                                                                     
        // kDebug() << "cap(0) " << versionRegExp.cap(0); // whole output of stdout       
        // kDebug() << "cap(1) " << versionRegExp.cap(1); // whole version string ex. 2011.1.1     
        // kDebug() << "cap(2) " << versionRegExp.cap(2); // major version string ex. 2011
        // kDebug() << "cap(3) " << versionRegExp.cap(3); // minor version string ex. 1   
        // kDebug() << "cap(4) " << versionRegExp.cap(4); // patch version string ex. 1   
        // kDebug() << "cap(5) " << versionRegExp.cap(5); // hex value after version string        
                                                                                          
        m_version = versionRegExp.cap(2); // major version string ex. 2011                
        kDebug() << "Found " << path() << " version: " << version() ;                     
    } 
    else                                                                                  
    {
        kDebug() << " **** regexp didn't match version";                                  
    }                                                                                     
}
Comment 7 Benjamin Girault 2011-10-12 17:47:01 UTC
Using RegExp is an overkill here because we are looking for a specific substring. Using grep is too, because it means starting an additional program, and allocating a stack for it, etc... The way it is done currently is fine, I would just add a loop over the line, that's all (let's say between 3 and 5 lines.

Please note that I do not want to do it not because I can't (it's something completely trivial to do), but because I do not see the point in fixing a bug in a software by patching a dirty hack on another one. For example, if your mouse do not move the pointer on the screen, you will not try to fix the screen...

That being said, I am thinking about this issue, and trying to find a nice way to handle this.
Comment 8 brad 2011-10-13 14:57:13 UTC
On Wednesday, October 12, 2011 05:47:02 PM benjamin.girault@gmail.com wrote:
> https://bugs.kde.org/show_bug.cgi?id=283650
> 
> 
> 
> 
> 
> --- Comment #7 from  <benjamin girault gmail com>  2011-10-12 17:47:01 ---
> Using RegExp is an overkill here because we are looking for a specific
> substring. 
>
in general regular expressions tend to be less maintainable too (i.e. very 
ugly) -- so for the longevity of the code a simpler solution is probably the 
best solution.

> Using grep is too, because it means starting an additional
> program, and allocating a stack for it, etc...
>
Agreed. At the time i suggested this, i thought you would only need to shell-
out once, but after trying a few things and reading about QProcess, you would 
need to shell-out twice. So this is definitely not the best solution either. 
 
> That being said, I am thinking about this issue, and trying to find a nice
> way to handle this.
>
Thank you. It'll be much appreciated.

Cheers, 
 -- brad
Comment 9 Benjamin Girault 2011-11-14 23:08:49 UTC
(In reply to comment #7)
> That being said, I am thinking about this issue, and trying to find a nice way
> to handle this.

I pushed a patch on the repository that handles this issue. An extra warning is raised when the plugin is launched.
Comment 10 caulier.gilles 2011-11-25 09:52:46 UTC
Benjamin,

This file can be closed now, or you need more feedback after 2.4.0 to be sure ?

Gilles Caulier
Comment 11 Benjamin Girault 2011-11-27 10:25:39 UTC
Yes, It can be closed. This is a minor modification, and I am confident that it won't introduce a new bug and that it will work as expected.