Bug 1324204

Currently I'm working toward next release (0.5) and below is snapshot from the most recent commit c287b1e1a23c0a9e3f874c04f1d0954b56cf367d.

Spec URL: https://ssl.famera.cz/tmp/fast-vm-fedora22.spec
SRPM URL: https://ssl.famera.cz/tmp/fast-vm-0.4-1.fc22.src.rpm

I'll take a look and possibly sponsor Ondrej.

What is your FAS username?  You didn't include it in the request.

As for the review, I'll just start from the top.

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

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

* Another cosmetic detail: remove all trailing whitespace.

* Summary should begin with a capital letter.

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

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

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

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

FAS username: ondrejhome

SRPM URL: https://github.com/OndrejHome/fast-vm/releases/download/0.5/fast-vm-0.5-1.fc22.src.rpm
Spec URL: https://raw.githubusercontent.com/OndrejHome/fast-vm/develop/rpm/fast-vm-fedora22.spec

- removed unneeded macro in the start
- alignments and white spaces hopefully fixed
- summary corrected
- license changed to GPLv3+
- source tarball and SRPM is now from exactly same source (GitHub)
- list of dependencies split and ordered, additionally changed some dependencies to recommends only
- added initial changelog entry

new version of fast-vm (0.6)

SRPM URL: https://github.com/OndrejHome/fast-vm/releases/download/0.6/fast-vm-0.6-4.fc22.src.rpm
Spec URL: https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm-fedora22.spec

- no major changes in RPM .spec file since 0.5 (just added changelog entry)

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.

> * Your SPEC file name doesn't match the package name.
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming
- renamed to fast-vm.spec

> * %description line 29 is too long.
>   Line length shouldn't exceed 80 characters; cut it into two.
- done, put into new line

> * %{_libexecdir}/%{name}-helper.sh permissions look strange.
>  The file isn't world readable/executable.  Wouldn't 755 make more sense?
- the file is not intended to be executable by anyone other than root, but can be readable by people - changed to 744.

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

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

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

Thank you for suggestions. New .spec file and SRPM with changes from above can be found below. 

SRPM URL: https://github.com/OndrejHome/fast-vm/releases/download/0.6.1/fast-vm-0.6.1-1.fc22.src.rpm
Spec URL: https://raw.githubusercontent.com/OndrejHome/fast-vm/develop/rpm/fast-vm.spec

(In reply to Ondrej Faměra from comment #7)
> > * Your SPEC file name doesn't match the package name.
> >   https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming
> - renamed to fast-vm.spec

Ack.

> > * %description line 29 is too long.
> >   Line length shouldn't exceed 80 characters; cut it into two.
> - done, put into new line

Ack.

> > * %{_libexecdir}/%{name}-helper.sh permissions look strange.
> >  The file isn't world readable/executable.  Wouldn't 755 make more sense?
> - the file is not intended to be executable by anyone other than root, but
> can be readable by people - changed to 744.

Alright.

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

Ack.

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

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

Ack.

> Thank you for suggestions. New .spec file and SRPM with changes from above
> can be found below. 
> 
> SRPM URL:
> https://github.com/OndrejHome/fast-vm/releases/download/0.6.1/fast-vm-0.6.1-
> 1.fc22.src.rpm
> Spec URL:
> https://raw.githubusercontent.com/OndrejHome/fast-vm/develop/rpm/fast-vm.spec

--

Now for the dependencies -- we've gone through them together in person so I'll just list the required changes in here without explaining that in detail:

- Don't require bash
- Require coreutils
- Require ncurses
- Require openssh-clients
- Require sed
- dnsmasq-utils should be required, not recommended
- Recommend awk
- Remove the libguestfs-tools-ca "suggests"

> - Don't require bash
> - Remove the libguestfs-tools-ca "suggests"
- removed

> - Require coreutils
> - Require ncurses
> - Require openssh-clients
> - Require sed
- added

> - dnsmasq-utils should be required, not recommended
- updated the code in the fast-vm-helper.sh so the dnsmasq-utils can be only recommended

> - Recommend awk
- added as 'required' as the new listing functions ('fast-vm list') requires this

Updated .spec file and SRPM below

SRPM URL: https://github.com/OndrejHome/fast-vm/releases/download/0.7/fast-vm-0.7-1.fc22.src.rpm
Spec URL: https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm.spec

Hotfix 0.7.1

- wrong 'awk' requirement, changed to 'gawk'
- updated the description of the package to match the description
from the README

SRPM URL: https://github.com/OndrejHome/fast-vm/releases/download/0.7.1/fast-vm-0.7.1-1.fc22.src.rpm
Spec URL: https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm.spec

Acking all the changes.
The package builds and install fine.

I'm tentatively approving the review.

However, before I really do that and sponsor you into the packaging group, I'd like to see you prepare another package.  It doesn't have to be anything huge.  Just to see you really begin to understand the whole packaging thing.

If you're out of ideas, check out the package wishlist:
https://fedoraproject.org/wiki/Package_maintainers_wishlist?rd=PackageMaintainers/WishList

Fedora 22 changed to end-of-life (EOL) status on 2016-07-19. Fedora 22 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.