Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: pps-tools - LinuxPPS user-space tools
https://bugzilla.redhat.com/show_bug.cgi?id=692069
Summary: Review Request: pps-tools - LinuxPPS user-space tools Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: mlichvar@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Story Points: ---
Spec URL: http://mlichvar.fedorapeople.org/tmp/pps-tools.spec SRPM URL: http://mlichvar.fedorapeople.org/tmp/pps-tools-0-0.1.20100413git74c32c.fc14.... Description: This package includes the LinuxPPS user-space tools.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
--- Comment #1 from Miroslav Lichvar mlichvar@redhat.com 2011-03-30 07:06:28 EDT --- The -devel subpackage contains only one file, the timepps.h header and there are no dependencies, maybe it could go to the main package?
The header file is a build requirement for ntp, chrony and gpsd packages, which currently include their own copy of the file.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
Ankur Sinha sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |sanjay.ankur@gmail.com AssignedTo|nobody@fedoraproject.org |sanjay.ankur@gmail.com Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
--- Comment #2 from Ankur Sinha sanjay.ankur@gmail.com 2011-08-09 12:05:25 EDT --- Review:
+ OK - NA ? ISSUE
+ Package meets naming and packaging guidelines + Spec file matches base package name. + Spec has consistant macro usage. + Meets Packaging Guidelines. + License + License field in spec matches ? License file included in package ^^ Please include debian/copyright in the package's %doc section. Is the README worth including in %docs.
+ Spec in American English + Spec is legible. + Sources match upstream md5sum: ^^^ git archives, checked with diff: [ankur@ankur pps-tools]$ pwd /home/ankur/dump/pps-tools [ankur@ankur pps-tools]$ diff -ur ../../rpmbuild/SOURCES/pps-tools/ ./ Only in ./: .git
- Package needs ExcludeArch + BuildRequires correct - Spec handles locales/find_lang - Package is relocatable and has a reason to be. + Package has %defattr and permissions on files is good. + Package has a correct %clean section. + Package has correct buildroot %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) ^^ If you're not building for rhel etc., you can get rid of the above 3 portions.
+ Package is code or permissible content. - Doc subpackage needed/used. + Packages %doc files don't affect runtime.
+ Headers/static libs in -devel subpackage. ^^ Even though there's only one header, I think we should leave it in the -devel package.
- Spec has needed ldconfig in post and postun - .pc files in -devel subpackage/requires pkgconfig - .so files in -devel subpackage. ? -devel package Requires: %{name} = %{version}-%{release} ^^ The devel is only a header. No sonames or anything here. Don't think this is required. Need to confirm.
- .la files are removed.
- Package is a GUI app and has a .desktop file
+ Package compiles and builds on at least one arch. + Package has no duplicate files in %files. + Package doesn't own any directories other packages own. + Package owns all the directories it creates. + No rpmlint output. + final provides and requires are sane: == pps-tools-0-0.1.20100413git74c32c.fc17.i686.rpm == Provides: pps-tools = 0-0.1.20100413git74c32c.fc17 pps-tools(x86-32) = 0-0.1.20100413git74c32c.fc17
Requires: /bin/sh libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.3.4) rtld(GNU_HASH)
== pps-tools-0-0.1.20100413git74c32c.fc17.src.rpm == Provides:
Requires:
== pps-tools-debuginfo-0-0.1.20100413git74c32c.fc17.i686.rpm == Provides: pps-tools-debuginfo = 0-0.1.20100413git74c32c.fc17 pps-tools-debuginfo(x86-32) = 0-0.1.20100413git74c32c.fc17
Requires:
== pps-tools-devel-0-0.1.20100413git74c32c.fc17.i686.rpm == Provides: pps-tools-devel = 0-0.1.20100413git74c32c.fc17 pps-tools-devel(x86-32) = 0-0.1.20100413git74c32c.fc17
Requires:
SHOULD Items:
+ Should build in mock. + Should build on all supported archs ^^ builds on both i386 and x86_64
- Should function as described. - Should have sane scriptlets. + Should have subpackages require base package with fully versioned depend. Not required for this package.
+ Should have dist tag + Should package latest version - check for outstanding bugs on package. (For core merge reviews)
Issues:
1. Only the license/docs need to be included.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
--- Comment #3 from Miroslav Lichvar mlichvar@redhat.com 2011-08-09 12:49:05 EDT --- Thanks for the review. I've updated the spec to include the README and copyright files and also included a symlink in /usr/include/sys for better compatibility. The devel package shouldn't need anything from the base package.
http://mlichvar.fedorapeople.org/tmp/pps-tools-0-0.2.20100413git74c32c.fc14....
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
Ankur Sinha sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #4 from Ankur Sinha sanjay.ankur@gmail.com 2011-08-09 13:08:54 EDT --- Hello,
I was just wondering if it would be better to link it like this:
ln -s $RPM_BUILD_ROOT%{_includedir}/timepps.h $RPM_BUILD_ROOT%{_includedir}/sys
instead of
ln -s ../timepps.h $RPM_BUILD_ROOT%{_includedir}/sys
The rest looks good!
[ankur@ankur SRPMS]$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/*.rpm pps-tools.i686: W: no-manual-page-for-binary ppsbind pps-tools.i686: W: no-manual-page-for-binary ppswatch pps-tools.i686: W: no-manual-page-for-binary ppsfind pps-tools.i686: W: no-manual-page-for-binary ppstest pps-tools.src: W: invalid-url Source0: pps-tools-20100413git74c32c.tar.gz pps-tools-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/pps-tools/timepps.h pps-tools-devel.i686: W: no-documentation pps-tools-devel.i686: E: incorrect-fsf-address /usr/include/timepps.h 4 packages and 0 specfiles checked; 2 errors, 6 warnings.
Please do request upstream to correct the FSF address.
XX APPROVED XX
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
--- Comment #5 from Miroslav Lichvar mlichvar@redhat.com 2011-08-10 09:42:59 EDT --- I think relative symlinks are preferred for this.
Thanks for the review!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
Miroslav Lichvar mlichvar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #6 from Miroslav Lichvar mlichvar@redhat.com 2011-08-10 09:45:44 EDT --- New Package SCM Request ======================= Package Name: pps-tools Short Description: LinuxPPS user-space tools Owners: mlichvar Branches: f15 f16 InitialCC:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=692069
--- Comment #7 from Jon Ciesla limb@jcomserv.net 2011-08-10 11:09:59 EDT --- Git done (by process-git-requests).
package-review@lists.fedoraproject.org