Bug 225849

* BuildRoot isn't the right one

* Wouldn't it be a good idea to add directories holding truetype 
fonts located in %{_datatdir}/fonts to the gnuplot-4.0.0-refers_to.patch
patch?

* A suggestion: depend on the font packages. It has pros and cons.

* unuseful requires:
Requires: libpng

* I don't like the License tag. It appears not to be free software as it
is now, although it is. Maybe it would be better to have something like 
License: Gnuplot
And in a comment say something along
# changes must be distributed as patches

* RPM_OPT_FLAGS is unuseful on make command line, and %{?_smp_mflags}
is missing. I tested that replacing with
 make %{?_smp_mflags}

seems right.

Is
PATH=$RPM_BUILD_DIR/gnuplot-%{version}:$PATH
really needed?

* The %post scriptlet should be run everytime not only at install time

* A suggestion: in %{files} use wildcard to handle different compression or
no compression:

%{_mandir}/man1/gnuplot.1*
%{_infodir}/gnuplot.info*


* The doc in psdoc isn't generated rightly. I suggested cd to the directory
and
make ps_symbols.ps ps_fontfile_doc.pdf

Then distribute only 
ps_fontfile_doc.pdf ps_guide.ps  ps_symbols.ps ps_file.doc

It implies a BuildRequires on latex for pdflatex

Note that a new version 4.2.0 is available. I would be interesting to have 
this version in F7.

http://sourceforge.net/forum/forum.php?forum_id=671915

(In reply to comment #1)
> * BuildRoot isn't the right one
fixed

> * Wouldn't it be a good idea to add directories holding truetype 
> fonts located in %{_datatdir}/fonts to the gnuplot-4.0.0-refers_to.patch
> patch?
The fonts directories are changed too often to have them here.

> * A suggestion: depend on the font packages. It has pros and cons.
I'm not sure I like to discuss this with gnuplot upstream.

> * unuseful requires:
> Requires: libpng
fixed

> * I don't like the License tag. It appears not to be free software as it
> is now, although it is. Maybe it would be better to have something like 
> License: Gnuplot
> And in a comment say something along
> # changes must be distributed as patches
fixed 

> * RPM_OPT_FLAGS is unuseful on make command line, and %{?_smp_mflags}
> is missing. I tested that replacing with
>  make %{?_smp_mflags} 
> seems right.
fixed

> Is
> PATH=$RPM_BUILD_DIR/gnuplot-%{version}:$PATH
> really needed?
fixed 

> * The %post scriptlet should be run everytime not only at install time
fixed
> 
> * A suggestion: in %{files} use wildcard to handle different compression or
> no compression:
> 
> %{_mandir}/man1/gnuplot.1*
> %{_infodir}/gnuplot.info*
I think the present version is right and it will help to reveal changes in new
versions.
> 
> 
> * The doc in psdoc isn't generated rightly. I suggested cd to the directory
> and
> make ps_symbols.ps ps_fontfile_doc.pdf
>
> Then distribute only 
> ps_fontfile_doc.pdf ps_guide.ps  ps_symbols.ps ps_file.doc
fixed
 
> It implies a BuildRequires on latex for pdflatex
> 
> 

Add comment #2
version 4.2.0 will be put to fc8 now it is too late to upgrade.

(In reply to comment #3)

> > * Wouldn't it be a good idea to add directories holding truetype 
> > fonts located in %{_datatdir}/fonts to the gnuplot-4.0.0-refers_to.patch
> > patch?
> The fonts directories are changed too often to have them here.

What do you exactly mean here? I think that at least the 
/usr/share/fonts/default/ghostscript/
/usr/share/fonts/default/Type1/
directories should be there. And certainly
/usr/share/fonts/bitstream-vera/

> > * A suggestion: depend on the font packages. It has pros and cons.
> I'm not sure I like to discuss this with gnuplot upstream.

Why do you have to discuss with upstream? It is a packaging 
issue.

> > * I don't like the License tag. It appears not to be free software as it
> > is now, although it is. Maybe it would be better to have something like 
> > License: Gnuplot
> > And in a comment say something along
> > # changes must be distributed as patches
> fixed 

You didn't exactly do what I said, but it is not a problem.

> I think the present version is right and it will help to reveal changes in new
> versions.

As you like.
 
> > 
> > * The doc in psdoc isn't generated rightly. I suggested cd to the directory
> > and
> > make ps_symbols.ps ps_fontfile_doc.pdf
> >
> > Then distribute only 
> > ps_fontfile_doc.pdf ps_guide.ps  ps_symbols.ps ps_file.doc
> fixed

From what I see it isn't fixed at all...

Patrice what preciesly are the problems you need to change before you proved
this review?

I think that the font directories are important for the integration in 
fedora.

The docs are not a blocker, but I gave all the informations needed
to implement it. Do you want a patch for that?

Otherwise at the moment I don't have any time to devote to merge
reviews (it is also for other reviews I am involved in), so somebody
else should take the review, or you'll have to wait (notice that I 
never formally took the review, because I knew that I could be taken
by 'real life' tasks).

A side note is that the comments I made in the gd merge review are
in fact useful for the new gnuplot png driver, and also to avoid
bogus dependencies in gnuplot.

I have a bit more time currently, but it seems that you haven't
responded to my last comment. I think that having the right 
fonts is very important for gnuplot usability. Assigning to me.

# rpm -ql gnuplot-emacs
/usr/share/emacs/site-lisp/gnuplot-gui.el
/usr/share/emacs/site-lisp/gnuplot-gui.elc
/usr/share/emacs/site-lisp/gnuplot.el
/usr/share/emacs/site-lisp/gnuplot.elc
/usr/share/emacs/site-lisp/info-look.20.2.el
/usr/share/emacs/site-lisp/info-look.20.3.el
/usr/share/emacs/site-lisp/site-start.d/gnuplot-init.el

0) Subpackage gnuplot-emacs should really be called emacs-gnuplot for
consistency with other emacs add-on packages.

1) These files should be installed in their own sub-directory
/usr/share/emacs/site-lisp/gnuplot

2) The source elisp (*.el) files should be packaged in a emacs-gnuplot-el
subpackage - they are not needed at runtime (basically they are the equivalent
to devel files). That is except for gnuplot-init.el.

3) info-look.20.2.el and info-look.20.3.el should be byte compiled.

Thanks for your help.
Patrice,
your comments are incorporate in gnuplot-4.2.0-4.fc8. 
Jonathan, I'm not sure about your comments but 
0,2/ I don't think this should be done (0/ the common system is to call
subpackage the way it is now - there are only two exceptions (emacs-git,
emacs-gtypist).)
1,3/ I will look to this point more precisely - now I'm not sure about the right
directory/what should be done with attached files
Could you please look to gnuplot-4.2.0-4.fc8 whether this version is good enough
to get fedora-review flag?

I just build gnuplot-4.2.0-5.fc8 when emacs  files are moved to
/usr/share/emacs/site-lisp/gnuplot subdirectory.

Created attachment 202811 [details]
fixes for the spec file

Here is a spec file patch with the following changes:

- don't BuildRequires itself (and remove gnuplot-4.2.0-ver.patch)
- put X11 app-defaults file with right name in the right directory
- don't try to make ps_fontfile_doc.ps, Makefile is broken
- keep timestamps
- ship more doc files
- own latex directories, since latex may not be installed
- ship more docs
- add needed Requires(..) and minor fixes in info scriptlets

I am in vacation for the next week, in case somebody is waiting
for responses...

Thanks. Fixed.

You should add to the directories searched for fonts:

/usr/share/fonts/dejavu-lgc
/usr/share/fonts/liberation

Hopefully the font directory won't change too often or gnuplot
will try to use more generic font mechanisms. But in the mean time
it is required.

You should have a look at
http://fedoraproject.org/wiki/Packaging/Emacs
your emacs add-ons packaging doesn't follow the guideline.

* unuseful requires:
Requires: libpng

RPM_OPT_FLAGS="$RPM_OPT_FLAGS" is not used during build.

rpmint issues that are, in my opinion worth fixing:
gnuplot.i386: W: file-not-utf8 /usr/share/doc/gnuplot-4.2.0/ChangeLog
gnuplot.i386: W: spurious-executable-perm
/usr/share/doc/gnuplot-4.2.0/demo/html/webify.pl
gnuplot.i386: E: explicit-lib-dependency libpng
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.0/demo/html/webify.pl perl(HTML::Entities)
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.0/demo/html/webify.pl perl(ctime.pl)
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.0/demo/html/webify.pl perl(Env)
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.0/demo/html/webify.pl /usr/bin/perl
gnuplot-emacs.i386: W: no-documentation
gnuplot-debuginfo.i386: W: spurious-executable-perm
/usr/src/debug/gnuplot-4.2.0/src/getcolor.h

Thanks. Fixed in gnuplot-4.2.2-2.fc9.

The iconv on fontfile_latex.dem epslatex.dem epslatex.dem fontfile.dem
are completly wrong. The encoding is set in these files to latin1,
they should be kept as is.

Also I don't know what demo.edf is but running iconv on it is certainly
wrong, since it looks like there is some binary info in it.

You still don't follow the emacs guidelines, since there is a 
versionned dependency on emacs(bin) required. Also you should certainly
use the 
%define emacs_version %(pkg-config emacs --modversion)
%define emacs_lispdir %(pkg-config emacs --variable sitepkglispdir)
%define emacs_startdir %(pkg-coonfig emacs --variable sitestartdir)
with apropriate buildrequires.

Thanks fixed in gnuplot-4.2.2-3.fc9.

Please note those macros are broken as used in the Emacs addon guidelines - I
have emailed Tom Callaway with the fixes twice, but he hasn't yet responded (and
he owns the wiki page).

Anyway, they should really be as below which (a) fixes the "coonfig" typo, and
also stops CVS tagging breaking.

# If the emacs-el package has installed a pkgconfig file, use that to determine
# install locations and Emacs version at build time, otherwise set defaults.
%if %($(pkg-config emacs) ; echo $?)
%define emacs_version 22.1
%define emacs_lispdir %{_datadir}/emacs/site-lisp
%define emacs_startdir %{_datadir}/emacs/site-lisp/site-start.d
%else
%define emacs_version %(pkg-config emacs --modversion)
%define emacs_lispdir %(pkg-config emacs --variable sitepkglispdir)
%define emacs_startdir %(pkg-config emacs --variable sitestartdir)
%endif

For the interested, Tom has now updated the guidelines :)

Thanks for your help Jonathan, in gnuplot-4.2.2-3.fc9 there is used the similar
construction as you attach here. 
I wrote to Tom Callaway too :) (before he change the wiki).

There is still
gnuplot.i386: W: spurious-executable-perm
/usr/share/doc/gnuplot-4.2.2/demo/html/webify.pl
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.2/demo/html/webify.pl perl(HTML::Entities)
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.2/demo/html/webify.pl perl(ctime.pl)
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.2/demo/html/webify.pl perl(Env)
gnuplot.i386: W: doc-file-dependency
/usr/share/doc/gnuplot-4.2.2/demo/html/webify.pl /usr/bin/perl

This can be fixed with chmod a-x on webify.pl, either in %prep 
or %install.


Also the file eg7.eps is referenced in tutorial.dvi, so I think
it is better to ship it in %docs, like

%docs tutorial/eg7.eps

Thanks both problems are fixed in gnuplot-4.2.2-4.fc9.

Source match upstream

b9dd34e8210a65336b19ee408766a66f  gnuplot-4.2.2.tar.gz

Source archive timestamp is not kept. It is too late now, but please
remember next time to use spectool -g or wget -N:
-rw-rw-r-- 1 dumas dumas 2832174 sep  1 05:36 gnuplot-4.2.2.tar.gz
-rw-rw-r-- 1 dumas dumas 2832174 oct 25 13:35 gnuplot-4.2.2.tar.gz-orig


Everything looks right now. I haven't checked precisely the
data files timestamps but there is no multilib issue here.

APPROVED.

Thanks for your great work Patrice. I'm closing this bug.