Re: New Packager question again: can you point me to a not flawed package?
* Paul Johnson <pauljohn32@gmail.com> [080531 22:19]:
> For example, consider the source for the ess package, an Emacs addon
> that I use ("apt-get source ess"). Peruse the diff file, and you
> find several items that are not in debian rules, such as this:
>
> --- ess-5.3.0.orig/lisp/ess-mode.el
> +++ ess-5.3.0/lisp/ess-mode.el
> @@ -167,6 +167,8 @@
> (define-key ess-mode-map "{" 'ess-electric-brace)
> (define-key ess-mode-map "}" 'ess-electric-brace)
> (define-key ess-mode-map "\e\C-h" 'ess-mark-function)
> + (if (featurep 'xemacs)
> + (define-key ess-mode-map [(meta backspace)] 'backward-kill-word))
> (define-key ess-mode-map "\e\C-q" 'ess-indent-exp)
> (define-key ess-mode-map "\177" 'backward-delete-char-untabify)
> (define-key ess-mode-map "\t" 'ess-indent-command)
>
>
> I think this should be a patch. If Debian guidelines required people
> to prove that they can build packages without opening (and exposing
> the source to possible editing), things like this would go into
> patches. I think.
For this package (at least the version in etch, for the unstable version
someone seems to have screewed it up w.r.t. to the .orig tarball seriously),
I'd strongly discourage using any seperate patches:
debian/changelog | 285 ++++++++++++++++++++++++++++++++++++++++++++++
debian/compat | 1
debian/control | 17 ++
debian/copyright | 20 +++
debian/dirs | 1
debian/doc-base | 14 ++
debian/docs | 13 ++
debian/emacsen-install | 47 +++++++
debian/emacsen-remove | 15 ++
debian/emacsen-startup | 54 ++++++++
debian/postinst | 51 ++++++++
debian/postinst.debhelper | 16 ++
debian/prerm | 42 ++++++
debian/prerm.debhelper | 16 ++
debian/rules | 87 ++++++++++++++
debian/watch | 7 +
doc/refcard/refcard.tex | 6
lisp/ess-mode.el | 2
And the non-debian patches being:
--- ess-5.3.0.orig/doc/refcard/refcard.tex
+++ ess-5.3.0/doc/refcard/refcard.tex
@@ -2,7 +2,7 @@
\usepackage{multicol}
\usepackage{parskip}
\usepackage{a4}%-Northamerica: {fullpage}
-\usepackage{svn}%!
+%\usepackage{svn}%!
\addtolength{\textheight}{20mm}
\addtolength{\topmargin}{-16mm}
@@ -23,7 +23,7 @@
\pagestyle{empty}
\begin{document}
-\SVN $Date: 2006-04-04 19:25:56 +0200 (Tue, 04 Apr 2006) $
+%\SVN $Date: 2006-04-04 19:25:56 +0200 (Tue, 04 Apr 2006) $
%\begin{multicols}{1}
\begin{center}
{\LARGE ESS \ \ \ \ {\large
@@ -33,7 +33,7 @@
\smallskip
{\small updated for ESS 5.3.0}% {\footnotesize --- needs \em{more} updating!}}
- \\[1ex] {\tiny \SVNDate}
+ \\[1ex] %{\tiny \SVNDate}
\footnotesize --- as of \today
\end{center}
\begin{enumerate}
--- ess-5.3.0.orig/lisp/ess-mode.el
+++ ess-5.3.0/lisp/ess-mode.el
@@ -167,6 +167,8 @@
(define-key ess-mode-map "{" 'ess-electric-brace)
(define-key ess-mode-map "}" 'ess-electric-brace)
(define-key ess-mode-map "\e\C-h" 'ess-mark-function)
+ (if (featurep 'xemacs)
+ (define-key ess-mode-map [(meta backspace)] 'backward-kill-word))
(define-key ess-mode-map "\e\C-q" 'ess-indent-exp)
(define-key ess-mode-map "\177" 'backward-delete-char-untabify)
(define-key ess-mode-map "\t" 'ess-indent-command)
I doubt you can become any more clean that that in what is changed.
(a comment in those file would have been even better though, both
for reading the diff and for people reading the installed files)
> Isn't that what people were recommending to me yesterday in here?
Patch systems are good when there are enough patches that having all
in one .diff.gz get's hard to understand. Using them where there are
only disadvantages is ideology.
> I was thinking, well, maybe it is just that one package. I also use
> the emacs addon auctex. So I got the source for that, examined the
> diff, and found many changes that were confined to the debian
> directory, but also plenty that float about in various parts of the
> source code. Consider just a couple, which don't seem trivial to my
> eye:
> --- auctex-11.84.orig/tex.el
> +++ auctex-11.84/tex.el
> @@ -1292,11 +1292,14 @@
> (directory-files (or master-dir ".") nil regexp))))
> (if files
> (when (or (not TeX-clean-confirm)
> - (dired-mark-pop-up " *Deletions*" 'delete
> - (if (> (length files) 1)
> - files
> - (cons t files))
> - 'y-or-n-p "Delete files? "))
> + (condition-case nil
> + (dired-mark-pop-up " *Deletions*" 'delete
> + (if (> (length files) 1)
> + files
> + (cons t files))
> + 'y-or-n-p "Delete files? ")
> + (wrong-type-argument ; e.g. with Emacs 21
> + (y-or-n-p (format "Delete %S? " (car files))))))
> (dolist (file files)
> (delete-file (concat master-dir file))))
> (message "No files to be deleted"))))
Hey, this even has a comment, why it is supposed to be needed!
> --- auctex-11.84.orig/preview/Makefile.in
> +++ auctex-11.84/preview/Makefile.in
> @@ -60,7 +60,7 @@
>
> install-hint:
> @echo 'Congratulations! Build is complete.'
> - @echo
> + @echo
> @echo 'Now, run "make install" as root, or whatever user has permissions'
> @echo 'to write to the install directory.'
>
> @@ -130,13 +130,13 @@
>
> clean:
> rm -f latex.out testdocstrip.tex preview-latex.el
> - rm -rf *.prv *.elc *~ *.aux *.dvi *.log
> + rm -rf *.prv *.elc *~ *.aux *.dvi *.log
> (cd latex ; $(MAKE) clean)
>
These two indeed look some problem with spacing. An observant maintainer
should have catched those. But I guess they would have splipped in with
a patch system with the same likelyhood.
> distclean: clean
> rm -f config.log config.cache config.status
> rm -f Makefile */Makefile auto.el
>
> -maintainer-clean:
> +maintainer-clean: distclean
> $(MAKE) distclean
> rm -rf autom4te.cache
That does indeed look a bit strange and could help a comment, but how
should having a patch system changed anything about that?
> --- auctex-11.84.orig/preview/latex/Makefile.in
> +++ auctex-11.84/preview/latex/Makefile.in
> @@ -65,5 +65,5 @@
> $(PDFLATEX) '\nonstopmode
> \AtBeginDocument{\OnlyDescription}\input{preview.drv}'
>
> clean:
> - rm -f *~ *.aux *.dvi *.drv *.log
> - rm -f $(TEXMFGEN) preview.ins preview-mk.ins
> + rm -f *~ *.aux *.dvi *.drv *.log *.pdf
> + rm -f $(TEXMFGEN) preview.ins preview-mk.ins
that is clear.
> --- auctex-11.84.orig/preview/latex/preview.idx
> +++ auctex-11.84/preview/latex/preview.idx
> @@ -0,0 +1,12 @@
> +\indexentry{preview={\ttfamily preview} (environment)|usage}{6}
> +\indexentry{environments:preview=>{\ttfamily preview}|usage}{6}
> +\indexentry{nopreview={\ttfamily nopreview} (environment)|usage}{6}
> +\indexentry{environments:nopreview=>{\ttfamily nopreview}|usage}{6}
> +\indexentry{PreviewMacro=\verb!*+\PreviewMacro+|usage}{6}
> +\indexentry{PreviewMacro*=\verb!*+\PreviewMacro*+|usage}{7}
> +\indexentry{PreviewEnvironment=\verb!*+\PreviewEnvironment+|usage}{8}
> +\indexentry{PreviewEnvironment*=\verb!*+\PreviewEnvironment*+|usage}{8}
> +\indexentry{PreviewSnarfEnvironment=\verb!*+\PreviewSnarfEnvironment+|usage}{8}
> +\indexentry{PreviewOpen=\verb!*+\PreviewOpen+|usage}{8}
> +\indexentry{PreviewClose=\verb!*+\PreviewClose+|usage}{8}
> +\indexentry{ifPreview=\verb!*+\ifPreview+|usage}{8}
This is definitly a bug in missing a rm in a clean target. But as you see,
anyone can spot that easily.
> I like the image viewer "gqview", so I checked out its source
> packaging. The diff has a massive section of stuff that was changed
> by automake, "config.guess" "config.sub" and I THINK "aclocal.m4" and
> such. I gather from what was said yesterday that those things should
> be excluded from the diff file.
Nowadays it is usually recommended to not put them into the diff. But that
is no absolute thing and has changed over the time because of a more important
thing: The stability of the package. You need to change config.sub and config.guess
and some times other parts to get it actually working.
Here you have to choose between having more differences in the source package
and having a package needing more things when building and having heigher chances
to fail to build if different versions of autotools have subtile differences
in behaviour.
As autotools are currently stable, I'd recommend in running them in build time,
but some years ago the opposite was true.
If having a patch system, putting that in a different patch would be nice, but
regenerations of those files are really easy to spot and really easy to ignore
(I doubt anyone unable to do so would be able to understand any code).
> But look at some of the other changes
> in the diff that reach into the guts of gqview's source code itself.
> There should have been a patch, right?
Well, given the amount and kind of changes, I doubt there would be much
advantage in this case.
Thre is one obvious change in Makefile.am to change some dirs.
(if that is the only change causing the autoremake, I'd have suggested
changing the Makefile.in instead or overwriting with command line arguments).
In filelist.c, filelist.h rcfile.c some changes obvious checking for results
of writing and in rcfile.c a change in modifying writing of config file
in a way not destroying the old one if writing the new one fails.
> Note some of these changes are
> purely white-space changes, possibly even mistaken taps of the space
> bar by the packager.
> --- gqview-2.0.4.orig/src/main.c
> +++ gqview-2.0.4/src/main.c
> @@ -169,18 +169,18 @@
> */
> static gchar *html_browsers[] =
> {
> - /* Redhat has a nifty htmlview script to start the user's preferred browser */
> - "htmlview", NULL,
> + /* Debian's sensible-browser is similar to Redhat's htmlview */
> + "sensible-browser", NULL,
> /* GNOME 2 */
> - "gconftool-2", "gconftool-2 -g /desktop/gnome/url-handlers/http/command",
> + "gconftool-2", "gconftool-2 -g /desktop/gnome/url-handlers/http/command",
> /* KDE */
> - "kfmclient", "!kfmclient exec \"%s\"",
> + "kfmclient", "!kfmclient exec \"%s\"",
> /* use fallbacks */
> "firefox", NULL,
> - "mozilla", NULL,
> - "konqueror", NULL,
> - "netscape", NULL,
> - NULL, NULL
> + "mozilla", NULL,
> + "konqueror", NULL,
> + "netscape", NULL,
> + NULL, NULL
> };
If you mean this with "purely white-space changes", please take a look at the
changed file before making some claims.
> --- gqview-2.0.4.orig/src/editors.c
> +++ gqview-2.0.4/src/editors.c
> @@ -47,14 +47,14 @@
>
>
> static gchar *editor_slot_defaults[] = {
> - N_("The Gimp"), "gimp-remote -n %f",
> - N_("XV"), "xv %f",
> + N_("The Gimp"), "gimp-remote %f",
> N_("Xpaint"), "xpaint %f",
> NULL, NULL,
> NULL, NULL,
> NULL, NULL,
> NULL, NULL,
> NULL, NULL,
> + NULL, NULL,
> N_("Rotate jpeg clockwise"), "%vif jpegtran -rotate 90 -copy all
> -outfile %p_tmp %p; then mv %p_tmp %p;else rm %p_tmp;fi",
> N_("Rotate jpeg counterclockwise"), "%vif jpegtran -rotate 270 -copy
> all -outfile %p_tmp %p; then mv %p_tmp %p;else rm %p_tmp;fi",
Well, that is very obvious, too.
So this package has three little changes (changes of program defaults,
savely overwrite config file and change of paths), that do no overlap
and have no danger of no being mixed up. What should be incorrect with
this one?
Bernhard R. Link
Reply to: