[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

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

 



https://bugzilla.redhat.com/show_bug.cgi?id=1324204



--- Comment #6 from Petr Šabata <psabata@xxxxxxxxxx> ---
Okay, let's take a look.  Sorry for the delay.

(In reply to Petr Šabata from comment #3)
> * Your package is noarch, therefore defining the first three macros is fairly
>   pointless.  You're not getting any debuginfo for your shell script anyway.
>   You can drop these.

Removed.  Ack.

> * A cosmetic detail: consider expanding the tabs and aligning all the values.

You still use tabs but it looks reasonable with tabwidth=8.

> * Another cosmetic detail: remove all trailing whitespace.

Removed.  Ack.

> * Summary should begin with a capital letter.

Corrected.  Ack.

> * It's unclear whether the license is really `GPLv3' or `GPLv3+'.  Since you
>   are also the upstream -- if you're sure you really want `GPLv3', consider
>   noting that in the README file.

Changed to GPL3+ and clarified upstream.  Ack.

> * The source tarball included in the SRPM differs from the one available on
>   GitHub (!).

The files are the same now.  Ack.

> * A split (and ordered) list of runtime dependencies would be more readable.
>   For example:
>     Requires:  curl
>     Requires:  dnsmasq-utils
>     ...

It looks reasonable now.  I have yet to verify whether it's actually correct.

> * I'm not going to comment your your runtime dependencies yet.  Provide an
>   updated package with sources available upstream first.
> 
> * Your package has no %changelog entries.  Add one. "Initial release" would
> do.

Changelog is fine.  Ack.

New comments:

* Your SPEC file name doesn't match the package name.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming

* %description line 29 is too long.
  Line length shouldn't exceed 80 characters; cut it into two.

* %{_libexecdir}/%{name}-helper.sh permissions look strange.
  The file isn't world readable/executable.  Wouldn't 755 make more sense?

* It appears 440 permissions are more common for sudoers files.
  This isn't very important but consider changing yours.

* Mark your sudoers file as configuration.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files

* You should own the %{_datadir}/bash-completion/completions directory.
  Just change line 45 to %{_datadir}/bash-completion/completions.

I'll review your dependencies tomorrow, hopefully.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
http://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx




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