[Bug 895541] Review Request: ptbl - Periodic Table

[bugzilla at redhat.com]

Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=895541

Susi Lehtola <susi.lehtola at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |susi.lehtola at iki.fi

--- Comment #43 from Susi Lehtola <susi.lehtola at iki.fi> ---
As you are upstream, why are the man page and the desktop file not part of the
standard distribution tarball?

You need to document the sources of these files as per
https://fedoraproject.org/wiki/Packaging:SourceURL

**

The %install section looks a bit messed up. Please remove empty statements such
as
 find %{buildroot} -type f 
and the line continuations from
 desktop-file-install --dir=%{buildroot}%{_datadir}/applications %{SOURCE2}
as you are using longer lines than that just below.

As you are using desktop-file-install, there's no need to run
 desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop


Also the statement
 rm -vf %{buildroot}%{_datadir}/images/ptbl.png
 install -p -D -m 0644 images/*
%{buildroot}%{_datadir}/gnome/help/ptbl-manual/C/images/%{name}.png
 install -p -D -m 0644 images/* %{buildroot}%{_datadir}/pixmaps/%{name}.png

is rather odd. First of all, the use of the * wildcard would suggest that there
are many files to install, but from the syntax it seems that it's just a single
file. Why not type out the full filename (which probably is just pbtl.png)?

Note that the -v and -f flags to rm are somewhat unnecessary, as the rm command
is still shown in the rpmbuild output.

**

You are mixing ptbl and %{name}, which is bad style. Please choose a single
form and stick with it.

**

In the %files section
 %{_datadir}/gnome/help/ptbl-manual/C/images/%{name}.png
makes the RPM package own just that file, but the RPM also creates the
directories
 %{_datadir}/gnome/help/ptbl-manual/
 %{_datadir}/gnome/help/ptbl-manual/C/
 %{_datadir}/gnome/help/ptbl-manual/C/images/
which end up unowned. Changing the statement to 
 %{_datadir}/gnome/help/ptbl-manual
or preferably
  %{_datadir}/gnome/help/ptbl-manual/
for the sake of clarity will fix this issue.

**

Lastly, the %changelog is a wall of text. Please separate the entries with a
single blank line.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=Nigv1ztV2Z&a=cc_unsubscribe