Re: Bug#426581: meshlab - anyone still working on this
Teemu Ikonen writes ("Re: Bug#426581: meshlab - anyone still working on this"):
> http://esko.osmas.info/~tmx/meshlab/
I've reviewed this and it's looking reasonably good.
Firstly, two general observations:
* You should run lintian, particularly after making a wholly new
package.
* With a new package you should run it and play with its features.
This is a good way to find bugs :-).
Now for my detailed comments:
After I built it (on lenny), I ran meshlab from an xterm. It opened
an entirely blank override-redirect pale grey window covering the top
left area of my display, as well as the main window. What is that
blank window for ? Can it be got rid of ?
I tested it on a .off from the AIM@SHAPE library and it seemed to more
or less work. However there were a couple of things which made it
segfault. For example, if I select any shader other than none, or if
I ask for `Edit / Vertex painting' and then click on the object. I
assume that these are installation problems of some kind (ie, bugs in
the packaging).
If I ask for `Edit / Measure' I can't get the rest of the edit menu
back without clicking on the measuring tape in the toolbar. I assume
this is unintentional and is an upstream UI bug.
If I run the program with --help or -h, it tries to open them as
files. It ought at least to bomb out with a message saying please
refer to the meshlab wiki (with a URL).
lintian says:
W: meshlab: menu-item-uses-apps-section /usr/share/menu/meshlab:2
W: meshlab: menu-item-creates-new-section Apps/Technical /usr/share/menu/meshlab:2
W: meshlab: description-contains-homepage
W: meshlab source: debian-rules-ignores-make-clean-error line 38
These should be fixed I think. To get these messages say
lintian meshlab_1.1.0-1_i386.deb
lintian meshlab_1.1.0-1.dsc
and you can get a fuller explanation with `lintian -i'.
lintian also says:
W: meshlab: latest-debian-changelog-entry-without-new-version
which is because of your poorly chosen `1.1.0b~cvs20070528-1' version
number. This can be ignored if you like, or you could retrospectively
add a ~ after 1.1.0 if you feel like it.
(You may get different messages with sid's lintian - please check.)
This looks like a mistake:
+++ meshlab-1.1.0/debian/install
...
+meshlab/src/meshlab/shaders/*.frag usr/share/meshlab/shaders
+meshlab/src/meshlab/shaders/*.gdp usr/share/meshlab/shaderss
+meshlab/src/meshlab/shaders/*.vert usr/share/meshlab/shaders
Note the extra `s' in `shaderss'.
I think the things I've mentioned so far ought to be addressed somehow
(for the bugs, perhaps just mentioned in a README) before an upload.
The rest is icing on the cake:
Eyeballing the patch I notice an awful lot of
-INCLUDEPATH += ../.. ../../../../sf ../../../../code/lib/glew/include
+INCLUDEPATH += ../.. ../../../../sf
Maybe it would be possible to suggest to prepare a patch for upstream
submission which replaces ../../../../code/lib/glew/include with
$(GLEWINCLUDEPATH) or some such, which would be set in one place.
Likewise various places where glew.c is referred to.
Changing the same thing in many places like this is asking for merge
conflicts in the future.
There are quite a few places where we see things like this
+#if defined(DEBIAN)
+ QDir shadersDir = QDir(QString("/usr/share/meshlab/shaders"));
+#else
QDir shadersDir = QDir(qApp->applicationDirPath());
I'm not sure -DDEBIAN is the right answer here. If possible I would
try to persuade upstream to provide a compilation option for FHS
compliance.
In the same area, I note that you appear to have been slighly careless
with the whitespace in one of these:
- QDir shadersDir = QDir(qApp->applicationDirPath());
-#if defined(Q_OS_WIN)
+#if defined(DEBIAN)
+ QDir shadersDir = QDir(QString("/usr/share/meshlab/shadersrm"));
+#else
+ QDir shadersDir = QDir(qApp->applicationDirPath());
+# if defined(Q_OS_WIN)
Note that the indentation of the the applicationDirPath()-based
initialisation has changed (perhaps due to tab/space conversion). It
is a good idea to be very careful not to do this as it just causes
spurious merge conflicts in the future.
I notice that you had to change two very similar sets of code
initialising a shadersDir. You should submit a patch upstream to move
this into a single function to avoid repetition (if you haven't
already).
Ian.
Reply to: