[Bug 895541] Review Request: ptbl - Periodic Table

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--- Comment #44 from RudraB <rudra.banerjee@xxxxxxxxx> ---
(In reply to comment #43)
> As you are upstream, why are the man page and the desktop file not part of
> the standard distribution tarball?

Since this is my first project(both as a packager and upstreamer), initially
there is no man/desktop file(I had help as the style of gnome). I add them
later to meet the requirement of fedora. Now, once the evolution started,
changeing the content of source is discouraged. That is why it is what it is.
Once the review is over (and if it is selected, ahem), I will create a update
soon, with those changes.

> 
> 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.

Noted. I have changed spec as suggested. Looks prettier.

-- 
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=j0mXohWYVq&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]