Bug 283650

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).

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?

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).

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?

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.

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";                                  
    }                                                                                     
}

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.

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

(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.

Benjamin,

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

Gilles Caulier

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.