Spec URL: https://sourceforge.net/projects/libstoragemgmt/files/review/libstoragemgmt.spec SRPM URL: https://sourceforge.net/projects/libstoragemgmt/files/review/libstoragemgmt-0.0.4-1.fc16.src.rpm Description: libStorageMgmt is a library that will provide a vendor agnostic open source storage application programming interface (API) for storage arrays. Up stream project is located: https://sourceforge.net/projects/libstoragemgmt/ Note: Package review submitter is up stream developer.
rpmlint is clean. You can make it easier by just saying %{python_sitelib} under files instead of specifying each file, it will pick up everything under there. Same goes for _libdir and _bindir and _includedir, and the * shouldn't be needed. Recommend putting a blank line between entries in the changelog Recommend using either %{buildroot} or $RPM_BUILD_ROOT, but not both. libstoragemgmt also has commandline programs, but the description only says this is a library. Should these go in a %{name}-utils pkg? At least mention them, and add manpages for them down the line.
Thanks for the review Andy! > You can make it easier by just saying %{python_sitelib} under files instead of > specifying each file, it will pick up everything under there. Same goes for > _libdir and _bindir and _includedir, and the * shouldn't be needed. Done, but the /* appears to be required. Otherwise I get rpmlint errors (standard-dir-owned-by-package ) when checking the rpm binaries. Additionally I ran into issues when I used: %{_libdir} instead of %{_libdir}/*.so.* Everything worked fine on x86_64, but failed to build clean on i686 (F16). I got some weird dependency for debug. Resulting rpm binaries were unusable. What was really weird is a mock i686 build on x86_64 worked fine. > Recommend putting a blank line between entries in the changelog Done > Recommend using either %{buildroot} or $RPM_BUILD_ROOT, but not both. Done > libstoragemgmt also has commandline programs, but the description only says > this is a library. Should these go in a %{name}-utils pkg? At least mention > them, and add manpages for them down the line. Done, added more information to the description. Man pages will be forthcoming in a future release. Review files updated. I am looking for a sponsor.
> but the /* appears to be required. Correct. You also need it behind %{python_sitelib} for exactly the reason you've given. Example with expanded macros: /usr/bin/* would include any files in /usr/bin, whereas /usr/bin or the more explicit /usr/bin/ would include the directory /usr/bin and anything in it. So, %{python_sitelib} would also include Python's site-packages directory, which does not belong into your package. > Summary: A library for storage array management The majority of package summaries omit leading articles, which makes them much more pretty when displayed in in Anaconda (and package tools). > Requires: %{name} = %{version}-%{release} https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > %post > /sbin/ldconfig Just want to point out, this means that there is no automatic dependency on /sbin/ldconfig. > %files devel > %defattr(-,root,root,-) > %doc README COPYING.LIB https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
Thank you for the review Michael! > Correct. You also need it behind %{python_sitelib} for exactly the reason > you've given. Done >> Summary: A library for storage array management > > The majority of package summaries omit leading articles, which makes them much > more pretty when displayed in in Anaconda (and package tools). Done >> Requires: %{name} = %{version}-%{release} > > https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package Done >> %post >> /sbin/ldconfig > > Just want to point out, this means that there is no automatic dependency on > /sbin/ldconfig. Are you suggesting I make an explicit dependency for this? >> %files devel >> %defattr(-,root,root,-) >> %doc README COPYING.LIB > > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing Done Spec file updated.
Updated links: Spec URL: http://libstoragemgmt.git.sourceforge.net/git/gitweb.cgi?p=libstoragemgmt/libstoragemgmt;a=blob_plain;f=libstoragemgmt.spec.in SRPM URL: https://sourceforge.net/projects/libstoragemgmt/files/yum/fc/16/SRPMS/
A few things: * You need BuildRequires: systemd-units for %{_unitdir} to evaluate. * You're enabling this service by default, which is almost certainly not appropriate. You're also not using the proper scriptlets for systemd. See: https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd Correct those items, and I will do a review.
(In reply to comment #6) > A few things: > > * You need BuildRequires: systemd-units for %{_unitdir} to evaluate. Done > * You're enabling this service by default, which is almost certainly not > appropriate. You're also not using the proper scriptlets for systemd. See: > https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd The library service does not require configuration and does not provide network services, thus it meets the criteria to be enabled by default (https://fedoraproject.org/wiki/Starting_services_by_default). I did remove the service starts. > Correct those items, and I will do a review. Great, thank you!
Your SRPM does not appear to reflect the spec file changes, so I took your spec.in file and tried to build it with the 0.0.7 tarball from sourceforge, but it fails to compile. In general, please remember that whenever you make changes to a spec file, you should be incrementing Release and adding a changelog entry to describe the changes made to the spec. (Release resets to 1 if Version goes up). This workflow is why I generally do not recommend that projects autogenerate their spec files. Can you please upload a new working SRPM somewhere for me to review? (Also, you shouldn't need to have that @VERSION@ string in the Source URL, just use %{version} there.)
(In reply to comment #8) > In general, please remember that whenever you make changes to a spec file, > you should be incrementing Release and adding a changelog entry to describe > the changes made to the spec. (Release resets to 1 if Version goes up). This > workflow is why I generally do not recommend that projects autogenerate > their spec files. I believe I am using the change log incorrectly for the spec file. Based on your input I should only be documenting the changes to the spec file in the change log for the spec file not the changes that were made to the upstream project too? If this is true then most of my spec file change logs will simply document a version bump for upstream project. > Can you please upload a new working SRPM somewhere for me to review? I just spun a new release, updated tarball and SRPMS are available 0.0.8. > (Also, you shouldn't need to have that @VERSION@ string in the Source URL, > just use %{version} there.) Changed. Thanks!
The SHA256 sums on the source file in your SRPM and the source file on the sourceforge website do not match: 874f537102fbdb4171f4482704549b113ff5ccb6e8cccc0564e2791bc56f4ac6 libstoragemgmt-0.0.8.tar.gz 23721de69bebf0ef9202a165e50bc571b65c053fa06388e0b31dc7c28fc9cec7 rpmbuild/SOURCES/libstoragemgmt-0.0.8.tar.gz Not sure how you managed that, but you need to fix it. Once that is resolved, I think I can finish out this review.
(In reply to comment #10) > The SHA256 sums on the source file in your SRPM and the source file on the > sourceforge website do not match: > > 874f537102fbdb4171f4482704549b113ff5ccb6e8cccc0564e2791bc56f4ac6 > libstoragemgmt-0.0.8.tar.gz > 23721de69bebf0ef9202a165e50bc571b65c053fa06388e0b31dc7c28fc9cec7 > rpmbuild/SOURCES/libstoragemgmt-0.0.8.tar.gz > > Not sure how you managed that, but you need to fix it. Once that is > resolved, I think I can finish out this review. I have some simple scripts I use to automate the creation of binary rpms for the yum repo I have on sourceforge. For each supported os version and platform I was fetching as source and running through the entire build process (was good at testing all steps). Unfortunately, make dist/distcheck appears to not be a signature repeatable operation, thus the reason the tarballs have different signatures (but identical file content). I have created a new release 0.0.9 with some new scripts and have verified that all the tarballs match as I no longer generate the rpm binaries directly out of source control. Hopefully, this will allow you to finish out the review. Thanks!
== Review == Good: - rpmlint checks return: libstoragemgmt.src: W: spelling-error %description -l en_US lsmcli -> clime libstoragemgmt.src: W: spelling-error %description -l en_US lsmd -> LSD libstoragemgmt.x86_64: W: non-standard-uid /var/run/lsm libstoragemgmt libstoragemgmt.x86_64: W: non-standard-gid /var/run/lsm libstoragemgmt libstoragemgmt.x86_64: W: non-standard-uid /var/run/lsm/ipc libstoragemgmt libstoragemgmt.x86_64: W: non-standard-gid /var/run/lsm/ipc libstoragemgmt libstoragemgmt.x86_64: W: no-manual-page-for-binary smis_lsmplugin libstoragemgmt.x86_64: W: no-manual-page-for-binary sim_lsmplugin libstoragemgmt.x86_64: W: no-manual-page-for-binary smispy_lsmplugin libstoragemgmt.x86_64: W: no-manual-page-for-binary ontap_lsmplugin libstoragemgmt-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 11 warnings. All safe to ignore. - package meets naming guidelines - package meets packaging guidelines - license (LGPLv2+) OK, text in %doc, matches source - spec file legible, in am. english - source matches upstream (a4c40334c5bc18872ccac2121a05baa610b5f5d989308f2cb2dae1bd177eccd5) - package compiles on Fedora 17 (x86_64) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - permissions ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file - devel package ok - no .la files - post/postun ldconfig ok - devel requires base package n-v-r APPROVED. Tell me your Fedora Account Username and I will go sponsor you.
> Tell me your Fedora Account Username and I will go sponsor you. Great, thank you! Fedora account username: tasleson
You're now sponsored into the "packager" group. Continue from this step: https://fedoraproject.org/wiki/PackageMaintainers/Join#Get_Sponsored If you have any questions, feel free to email me.
New Package SCM Request ======================= Package Name: libstoragemgmt Short Description: Storage array management library Owners: tasleson Branches: f17 InitialCC:
re-setting spot's fedora-review+ and setting fedora-cvs? .
Git done (by process-git-requests).
libstoragemgmt-0.0.9-1.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/libstoragemgmt-0.0.9-1.fc17
libstoragemgmt-0.0.9-1.fc17 has been pushed to the Fedora 17 testing repository.
libstoragemgmt-0.0.9-1.fc17 has been pushed to the Fedora 17 stable repository.
Package Change Request ====================== Package Name: libstoragemgmt New Branches: el6 Owners: tasleson InitialCC: Requesting EPEL branch for existing fedora package.