Re: RFS: presage

[Matteo Vescovi <matteo.vescovi@yahoo.co.uk>]

Paul Wise wrote:
> On Fri, Jun 4, 2010 at 5:51 AM, Matteo Vescovi
> <matteo.vescovi@yahoo.co.uk> wrote:
>
>   
> > I am looking for a sponsor/reviewer for my package "presage".
> >     
>
> Here is a review:
>   

Many thanks for reviewing my package.

I started off fixing the reported issues with debian (quilt) patches, 
but soon realized that many fixes would have benefitted the upstream 
package, so I wore my upstream developer hat and integrated those fixes 
upstream into presage-0.8.3, which this new upload of presage package is 
based on.

The new package can be found here:
- URL: http://mentors.debian.net/debian/pool/main/p/presage
- Source repository: deb-src http://mentors.debian.net/debian unstable 
main contrib non-free
- dget 
http://mentors.debian.net/debian/pool/main/p/presage/presage_0.8.3-1.dsc


> Since you are packaging a library, I presume you have read
> libpkg-guide and its two bugs?  

I had read libpkg-guide, but not the two bugs. Done now.

> You should never install *.pyc files in the .deb, in Debian they are
> always generated by the maintainer scripts.
>   

Done, removed .pyc file from python-presage.install.

> libpresage1 contains a file that doesn't appear to change its
> path/name when the ABI is changed (/etc/presage.xml). This is against
> policy and the file needs to be moved elsewhere, probably to the -data
> package.
>   

Fixed, moved file to -data package.

> Installation/compilation instructions do not belong in binary
> packages, especially for non-Debian platforms. Please remove these
> files from the doc package:
>
> doc/INSTALL_Cygwin_dev_env.txt
> doc/INSTALL_MinGW_MSYS_dev_env.txt
>   

Done, removed files from -doc package.

> resources/la_coscienza_di_zeno.txt is under a non-free license. You
> need to remove it from the orig.tar.gz.
>   

Done.

> Your menu files don't list an icon. you might want to build-dep on
> imagemagick so you can convert the upstream PNG to an XPM.  

Added icon and pointed .menu file to it.

> It looks like the package includes an embedded code copy of scintilla.
> Usually Debian prefers different packages to be packaged separately.
> Is that needed? If it is, please notify the security team and give
> more details so they can record this information in their
> embedded-code-copies file. Same goes for tinyxml.
>   

Right, I modified presage build system to use the system libtinyxml 
rather than the embedded copy.

As for Scintilla, an embedded copy is used because scintilla is 
currently not packaged for Debian.

> You might want to look at using debhelper 7 instead of cdbs:
>
> https://penta.debconf.org/dc9_schedule/events/418.en.html
> http://kitenet.net/~joey/blog/entry/cdbs_killer___40__design_phase__41__/
>
> Your watch file isn't specific enough:
>
> pabs@chianamo:~/tmp/presage-0.8.2$ uscan
> presage: Newer version (extra-data-arpa-1.0) available on remote site:
>   http://qa.debian.org/watch/sf.php/presage/presage-extra-data-arpa-1.0.tar.gz
>   (local version is 0.8.2)
>   

Fixed watch file.

> Insert standard whine about package descriptions being duplicates of
> each other. Please make the binary package descriptions describe the
> binary packages, not the upstream project.
>   

Fixed. Removed redundant duplicate text from descriptions.

> Your Standards-Version is out of date, please read debian-policy
> upgrading.txt and update it.
>   

Done.

> Your Vcs-Browser URL should point to trunk too.
>   

Done.

> Do you really need to distribute the static library and .la file? In
> Debian we seem to be moving towards not shipping those.
>   

Ok, removed them.

> debian/README.Debian is not needed, splitting out libs/etc is standard
> practice in Debian.
>   

Removed it too.

> Is this warning something to be concerned about?
>
> configure: CMU-Cambridge SLM tools not found. ARPA ngram language
> model will not be built.
>   

Nothing to be concerned about. This means that no ARPA ngram language 
models will be built from the training text corpus for the 
ARPAPredictor, which is an optional predictor and thus not required for 
libpresage to work.

> The package fails to build, it looks like a race condition since it
> builds with debuild -j1:
>
> make[3]: Entering directory `/tmp/buildd/presage-0.8.2/bindings'
> Making all in python
> make[4]: Entering directory `/tmp/buildd/presage-0.8.2/bindings/python'
> /usr/bin/swig -c++ -python -I../../src/lib -o presage_wrap.cpp -outdir
> . ./../presage.i
> make[4]: *** No rule to make target `presage_wrap.h', needed by `all'.  Stop.
> make[4]: *** Waiting for unfinished jobs....
> make[4]: Leaving directory `/tmp/buildd/presage-0.8.2/bindings/python'
>   

Fixed, tested building with -j2 on my 2-core machine.

> gcc warnings:
>
> gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall
> -Wstrict-prototypes -g -O2 -g -Wall -O2 -fPIC -I../../src/lib
> -I/usr/include/python2.5 -c presage_wrap.cpp -o
> build/temp.linux-x86_64-2.5/presage_wrap.o
> cc1plus: warning: command line option "-Wstrict-prototypes" is valid
> for Ada/C/ObjC but not for C++
>   

The -Wstrict-prototypes flag comes from Python distutils module, which 
is used to build presage python extension module. Not sure how to remove 
the warning. It is harmless though, as the flag is ignored.

> g++ -DHAVE_CONFIG_H -I. -I../../.. -pthread -I/usr/include/gtk-2.0
> -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo
> -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/
> -I/usr/include/pixman-1 -I/usr/include/freetype2
> -I/usr/include/libpng12 -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include -pthread -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include -I./scintilla/include -I./scintilla/src
> -DNDEBUG -DGTK -DSCI_LEXER -DG_THREADS_IMPL_NONE -g -O2 -g -Wall -O2
> -c scintilla/src/Editor.cxx  -fPIC -DPIC -o
> .libs/libscintilla_la-Editor.o
> scintilla/src/Editor.cxx: In member function 'void
> Editor::LayoutLine(int, Surface*, ViewStyle&, LineLayout*, int)':
> scintilla/src/Editor.cxx:2049: warning: array subscript has type 'char'
> scintilla/src/Editor.cxx:2052: warning: array subscript has type 'char'
> scintilla/src/Editor.cxx:2055: warning: array subscript has type 'char'
> scintilla/src/Editor.cxx: In member function 'virtual void
> Editor::NotifyStyleToNeeded(int)':
> scintilla/src/Editor.cxx:4090: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void Editor::NotifyChar(int)':
> scintilla/src/Editor.cxx:4101: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifySavePoint(bool)':
> scintilla/src/Editor.cxx:4108: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyModifyAttempt()':
> scintilla/src/Editor.cxx:4118: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'virtual void
> Editor::NotifyDoubleClick(Point, bool, bool, bool)':
> scintilla/src/Editor.cxx:4124: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyHotSpotDoubleClicked(int, bool, bool, bool)':
> scintilla/src/Editor.cxx:4134: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyHotSpotClicked(int, bool, bool, bool)':
> scintilla/src/Editor.cxx:4143: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void Editor::NotifyUpdateUI()':
> scintilla/src/Editor.cxx:4152: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void Editor::NotifyPainted()':
> scintilla/src/Editor.cxx:4158: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyIndicatorClick(bool, int, bool, bool, bool)':
> scintilla/src/Editor.cxx:4166: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'bool
> Editor::NotifyMarginClick(Point, bool, bool, bool)':
> scintilla/src/Editor.cxx:4184: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyNeedShown(int, int)':
> scintilla/src/Editor.cxx:4198: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyDwelling(Point, bool)':
> scintilla/src/Editor.cxx:4206: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void Editor::NotifyZoom()':
> scintilla/src/Editor.cxx:4215: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'virtual void
> Editor::NotifyModified(Document*, DocModification, void*)':
> scintilla/src/Editor.cxx:4388: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/Editor.cxx: In member function 'void
> Editor::NotifyMacroRecord(unsigned int, uptr_t, sptr_t)':
> scintilla/src/Editor.cxx:4523: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
>  g++ -DHAVE_CONFIG_H -I. -I../../.. -pthread -I/usr/include/gtk-2.0
> -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo
> -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/
> -I/usr/include/pixman-1 -I/usr/include/freetype2
> -I/usr/include/libpng12 -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include -pthread -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include -I./scintilla/include -I./scintilla/src
> -DNDEBUG -DGTK -DSCI_LEXER -DG_THREADS_IMPL_NONE -g -O2 -g -Wall -O2
> -c scintilla/src/ScintillaBase.cxx  -fPIC -DPIC -o
> .libs/libscintilla_la-ScintillaBase.o
> scintilla/src/ScintillaBase.cxx: In member function 'void
> ScintillaBase::AutoCompleteCancel()':
> scintilla/src/ScintillaBase.cxx:296: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/ScintillaBase.cxx: In member function 'void
> ScintillaBase::AutoCompleteCharacterDeleted()':
> scintilla/src/ScintillaBase.cxx:337: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/ScintillaBase.cxx: In member function 'void
> ScintillaBase::AutoCompleteCompleted()':
> scintilla/src/ScintillaBase.cxx:357: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/src/ScintillaBase.cxx: In member function 'void
> ScintillaBase::CallTipClick()':
> scintilla/src/ScintillaBase.cxx:445: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
>  g++ -DHAVE_CONFIG_H -I. -I../../.. -pthread -I/usr/include/gtk-2.0
> -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo
> -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/
> -I/usr/include/pixman-1 -I/usr/include/freetype2
> -I/usr/include/libpng12 -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include -pthread -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include -I./scintilla/include -I./scintilla/src
> -DNDEBUG -DGTK -DSCI_LEXER -DG_THREADS_IMPL_NONE -g -O2 -g -Wall -O2
> -c scintilla/gtk/ScintillaGTK.cxx  -fPIC -DPIC -o
> .libs/libscintilla_la-ScintillaGTK.o
> scintilla/gtk/ScintillaGTK.cxx: In member function 'void
> ScintillaGTK::NotifyKey(int, int)':
> scintilla/gtk/ScintillaGTK.cxx:1042: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> scintilla/gtk/ScintillaGTK.cxx: In member function 'void
> ScintillaGTK::NotifyURIDropped(const char*)':
> scintilla/gtk/ScintillaGTK.cxx:1051: warning: missing braces around
> initializer for 'Sci_NotifyHeader'
> g++ -DHAVE_CONFIG_H -I. -I../../..  -pthread -I/usr/include/gtk-2.0
> -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo
> -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/
> -I/usr/include/pixman-1 -I/usr/include/freetype2
> -I/usr/include/libpng12 -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include   -pthread -I/usr/include/glib-2.0
> -I/usr/lib/glib-2.0/include   -I../../../src/lib -I./scintilla/include
>    -g -O2 -g -Wall -O2 -c -o gprompter-gprompter.o `test -f
> 'gprompter.cpp' || echo './'`gprompter.cpp
> gprompter.cpp: In function 'void on_char_added(SCNotification*,
> ScintillaObject*, Presage*)':
> gprompter.cpp:175: warning: format '%d' expects type 'int', but
> argument 2 has type 'uptr_t'
> gprompter.cpp:183: warning: format '%d' expects type 'int', but
> argument 2 has type 'uptr_t'
> gprompter.cpp:184: warning: format '%c' expects type 'int', but
> argument 2 has type 'uptr_t'
> gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall
> -Wstrict-prototypes -g -O2 -g -Wall -O2 -fPIC -I./src/lib
> -I/usr/include/python2.5 -c bindings/python/presage_wrap.cpp -o
> /tmp/buildd/presage-0.8.2/./build/temp.linux-x86_64-2.5/bindings/python/presage_wrap.o
> cc1plus: warning: command line option "-Wstrict-prototypes" is valid
> for Ada/C/ObjC but not for C++
> gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall
> -Wstrict-prototypes -g -O2 -g -Wall -O2 -fPIC -I./src/lib
> -I/usr/include/python2.6 -c bindings/python/presage_wrap.cpp -o
> /tmp/buildd/presage-0.8.2/./build/temp.linux-x86_64-2.6/bindings/python/presage_wrap.o
> cc1plus: warning: command line option "-Wstrict-prototypes" is valid
> for Ada/C/ObjC but not for C++
>   

I examined the scintilla compilation warnings and they seem harmless. I 
think they might be false positives.

> doxygen warnings:
>
> doxygen
> Warning: Tag `USE_WINDOWS_ENCODING' at line 66 of file Doxyfile has
> become obsolete.
> To avoid this warning please update your configuration file using "doxygen -u"
> Warning: Tag `DETAILS_AT_TOP' at line 158 of file Doxyfile has become obsolete.
> To avoid this warning please update your configuration file using "doxygen -u"
> Warning: Tag `MAX_DOT_GRAPH_WIDTH' at line 1199 of file Doxyfile has
> become obsolete.
> To avoid this warning please update your configuration file using "doxygen -u"
> Warning: Tag `MAX_DOT_GRAPH_HEIGHT' at line 1207 of file Doxyfile has
> become obsolete.
> To avoid this warning please update your configuration file using "doxygen -u"
>   

Fixed.

> Generating call graph for function
> PredictorRegistry::updat/tmp/buildd/presage-0.8.2/src/lib/core/profileManager.h:47:
> Warning: Unsupported xml/html tag <sysconfdir> found
> /tmp/buildd/presage-0.8.2/src/lib/predictors/recencyPredictor.h:46:
> Warning: Found unknown command `\frac'
> /tmp/buildd/presage-0.8.2/src/lib/predictors/recencyPredictor.h:46:
> Warning: Found unknown command `\lambda'
> /tmp/buildd/presage-0.8.2/src/lib/predictors/recencyPredictor.h:50:
> Warning: Found unknown command `\lambda'
> e
>   

Fixed.

> dpkg-shlibdeps warnings:
>
> dpkg-shlibdeps: warning: dependency on libdl.so.2 could be avoided if
> "debian/libpresage1/usr/lib/libpresage.so.1.1.1" were not uselessly
> linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libz.so.1 could be avoided if
> "debian/presage-gprompter/usr/bin/gprompter" were not uselessly linked
> against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libfontconfig.so.1 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libatk-1.0.so.0 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on librt.so.1 could be avoided if
> "debian/presage-gprompter/usr/bin/gprompter" were not uselessly linked
> against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libgio-2.0.so.0 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libcairo.so.2 could be avoided
> if "debian/presage-gprompter/usr/bin/gprompter" were not uselessly
> linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libgthread-2.0.so.0 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libpangocairo-1.0.so.0 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libfreetype.so.6 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
> dpkg-shlibdeps: warning: dependency on libpangoft2-1.0.so.0 could be
> avoided if "debian/presage-gprompter/usr/bin/gprompter" were not
> uselessly linked against it (they use none of its symbols).
>   

Mmm.. I'm not sure how to resolve those warnings... Can you please point 
me in the right direction? Is it a matter of passing 
LDFLAGS="-Wl,--as-needed" to the configure line?

> dpkg-gencontrol warning:
>
> dpkg-gencontrol: warning: package python-presage: unused substitution
> variable ${python:Versions}
>   

I'm not sure how to resolve this... Any suggestions?

> lintian complaints:
>
> W: presage source: ancient-standards-version 3.7.3 (current is 3.8.4)
> P: presage-doc: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL
> P: libpresage-data: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL
> I: presage: hyphen-used-as-minus-sign usr/share/man/man1/text2ngram.1.gz:7
> P: presage: copyright-refers-to-symlink-license usr/share/common-licenses/GPL
> P: libpresage1: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL
> I: libpresage1: spelling-error-in-binary ./usr/lib/libpresage.so.1.1.1
> Commiting Committing
> X: libpresage1: shlib-calls-exit usr/lib/libpresage.so.1.1.1
> I: libpresage1: no-symbols-control-file usr/lib/libpresage.so.1.1.1
> P: libpresage-dev: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL
> P: python-presage: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL
> P: presage-gprompter: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL
> P: presage-pyprompter: copyright-refers-to-symlink-license
> usr/share/common-licenses/GPL  

Fixed copyright-refers-to-symlink-license lintian complaints and 
ancient-standards-version.


Cheers,
- Matteo