Hide Forgot
Spec URL: https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm-fedora22.spec SRPM URL: https://github.com/OndrejHome/fast-vm/releases/download/0.4/fast-vm-0.4-22.fc22.src.rpm Description: fast-vm is a script for defining VMs from images provided in thin LVM pool. fast-vm is taking care of: - defining the VMs from provided XML in libvirt - creating thin LV thin snaphost as storage devices for VMs - making static IP DHCP reservation in libvirt network definition for MAC address of VM
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.