Spec URL: http://bzr.coffeecode.net/scratch/perl-MARC-Charset/perl-MARC-Charset.spec SRPM URL: http://bzr.coffeecode.net/scratch/perl-MARC-Charset/perl-MARC-Charset-1.33-1.fc17.src.rpm Description: Hi, I'm in the process of packaging dependencies for the Evergreen library system (http://evergreen-ils.org) in the hopes that one day we will be able to install Evergreen via yum. This package is one of the core requirements for library systems as it deals with a legacy data encoding dating from the 70's that needs to be converted to UTF8 to be useful to most software. Making this package available in Fedora will benefit not just Evergreen, but also Koha and other library systems that use Perl. I work closely with my peers in the code4lib community and speak with the previous and current maintainers of the Perl MARC packages on a nearly-daily basis. Fedora Account System Username: dscott
In anticipation of a comment regarding the arch specificity of the package and rpmlint's report that it found no binary, perl-MARC-Charset creates a GDBM_File binary object that is not portable. It stores this object in the same Perl directory as the MARC/Charset/Table.pm module. In an effort to prevent two arch-specific packages from being installed on the same system and thereby clobbering one another, I've introduced a symbolic link that points at libdir from MARC/Charset/Table. My intention is that this should introduce a conflict should someone attempt to install two versions of the same arch-specific package. (Debian appears to take the same approach, for whatever that's worth). I checked on #fedora-devel and there didn't seem to be horrified reactions, and they also suggested that rpmlint's warning about no-binary could be ignored as it doesn't understand gdbm, but this is my first time dealing with an oddity like this. If there's a better recommended practice, I'm all ears.
Also, this is my first package review request and I do need a sponsor :)
Okay, I'll bite - if only because I live in a community called Evergreen and we don't have a public library system - maybe this will help ;) It's very promising that you noticed the arch-dependent nature of the package and came up with a solution. I had a similar issue with perl-Lingua-EN-Tagger which I resolved by making the whole package arch-dependent, passing INSTALLVENDORLIB=%{perl_vendorarch} to Makefile.PL so that the whole package gets installed under %{_libdir}/perl5/vendor_perl. And since this approach avoids conflicts entirely, I guess it counts as a better recommended practice. You're quite right that rpmlint's no-binary "error" can be ignored in this case, but we also build debuginfo packages and rpmlint also complains about empty-debuginfo-package. In this case, it's appropriate to disable debuginfo package by adding %global debug_package %{nil} to the spec file. (see https://fedoraproject.org/wiki/Packaging:Debuginfo for more info). Otherwise, it's a fairly simple package. There's just a few minor issues: you don't need to specify the BuildRoot tag - https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag you don't need to explicitly Require: perl(Unicode::Normalize) - rpm detects this dependency automatically. you should explicitly BuildRequire any dependencies that are part of perl-core itself - although they're pulled in as transient dependencies at the minute, they might not be in future (the Red Hat perl folks tend to pretty insistent on this): perl(base) perl(Carp) perl(charnames) perl(constant) perl(Encode) perl(POSIX) perl(Storable) perl(strict) perl(Unicode::Normalize) perl(Unicode::UCD) perl(warnings) And as a sponsor, it would also be nice to see a couple of informal reviews of other packages. https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
Nearly forgot - you should also use DESTDIR instead of PERL_INSTALL_ROOT in %install section. Bug 562316 has some background.
Iain: thanks so much for your detailed review comments! I do hope that Evergreen (http://evergreen-ils.org) will be useful to the community of Evergreen; you can always pass my contact info on for help if that comes to pass. I've revised the package SPEC and SRPM according to your advice (and added a line to the ChangeLog crediting you for your feedback): Spec URL: http://bzr.coffeecode.net/scratch/perl-MARC-Charset/perl-MARC-Charset.spec SRPM URL: http://bzr.coffeecode.net/scratch/perl-MARC-Charset/perl-MARC-Charset-1.33-1.fc17.src.rpm In retrospect, pointing at %{perl_vendorarch} seems like the obvious way to solve the arch-specific problem. Thanks for your other suggestions, as well, that in some cases helped turn a statement of fact ("Fedora does not require the presence of the BuildRoot tag") into a prescriptive guideline ("Do not include the BuildRoot tag.") or flagged an archaic practice (not using DESTDIR for the install argument). I have also delved back into a number of the packaging documents to increase my familiarity with the guidelines, which were pretty overwhelming on my first pass through them. I plan on reviewing other package submissions, but wanted some reassurance that I was on the right track and wouldn't be misdirecting other would-be packages. Thanks so much for your generous donation of time to help me out!
Oh, and because I'm going for bonus points: http://koji.fedoraproject.org/koji/taskinfo?taskID=4153096
Package Review ============== Key: - = N/A x = Pass ! = Fail ? = Not evaluated ==== Generic ==== [x]: MUST Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: MUST Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [-]: MUST %build honors applicable compiler flags or justifies otherwise. [x]: MUST All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: MUST Buildroot is not present Note: Unless packager wants to package for EPEL5 this is fine [x]: MUST Package contains no bundled libraries. [x]: MUST Changelog in prescribed format. [x]: MUST Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: Clean would be needed if support for EPEL is required [x]: MUST Sources contain only permissible code or content. [x]: MUST Each %files section contains %defattr if rpm < 4.4 Note: Note: defattr macros not found. They would be needed for EPEL5 [x]: MUST Macros in Summary, %description expandable at SRPM build time. [x]: MUST Package requires other packages for directories it uses. [x]: MUST Package uses nothing in %doc for runtime. [x]: MUST Package is not known to require ExcludeArch. [x]: MUST Permissions on files are set properly. [x]: MUST Package does not contain duplicates in %files. [x]: MUST Spec file lacks Packager, Vendor, PreReq tags. [x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf would be needed if support for EPEL5 is required [-]: MUST Large documentation files are in a -doc subpackage, if required. [-]: MUST If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: MUST License field in the package spec file matches the actual license. [x]: MUST Package consistently uses macros (instead of hard-coded directory names). [x]: MUST Package is named according to the Package Naming Guidelines. [x]: MUST Package does not generate any conflict. [x]: MUST Package obeys FHS, except libexecdir and /usr/target. [x]: MUST Package must own all directories that it creates. [x]: MUST Package does not own files or directories owned by other packages. [x]: MUST Package installs properly. [x]: MUST Requires correct, justified where necessary. [!]: MUST Rpmlint output is silent. rpmlint perl-MARC-Charset-1.33-1.fc18.x86_64.rpm perl-MARC-Charset.x86_64: E: no-binary 1 packages and 0 specfiles checked; 1 errors, 0 warnings. rpmlint perl-MARC-Charset-1.33-1.fc18.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /home/iarnell/rpmbuild/perl-MARC-Charset/MARC-Charset-1.33.tar.gz : MD5SUM this package : 02882113257742eef9c11adf29a16854 MD5SUM upstream package : 02882113257742eef9c11adf29a16854 [x]: MUST Spec file is legible and written in American English. [x]: MUST Spec file name must match the spec package %{name}, in the format %{name}.spec. [-]: MUST Package contains a SysV-style init script if in need of one. [x]: MUST File names are valid UTF-8. [x]: MUST Useful -debuginfo package or justification otherwise. [x]: SHOULD Reviewer should test that the package builds in mock. [-]: SHOULD If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: SHOULD Dist tag is present. [-]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [-]: SHOULD Package functions as described. [x]: SHOULD Latest version is packaged. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD SourceX is a working URL. [-]: SHOULD Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: SHOULD Package should compile and build into binary rpms on all supported architectures. [x]: SHOULD %check is present and all tests pass. [x]: SHOULD Packages should try to preserve timestamps of original installed files. [x]: SHOULD Spec use %global instead of %define. As mentioned earlier, rpmlint's no-binary "error" can be ignored. There are no blockers, so this package is APPROVED. Although I've not yet formally reviewed your other packages (sorry, a bit pushed for time today), the changes you made seem to be appropriate. You obviously know what you're doing, respond well to feedback, and the bonus points show some familiarity with our build system. I'm very pleased to welcome you as the latest member of the team. When making the SCM request for this package (and the others, once I've reviewed them - hopefully tomorrow), please add me (iarnell) and perl-sig to the IntialCC section. And if you haven't already joined Fedora's perl-devel mailing list, I strongly suggest doing so - you might want to filter out the bug/git spam, though.
New Package SCM Request ======================= Package Name: perl-MARC-Charset Short Description: Converts data encoded in MARC-8 to Unicode (UTF-8) New Branches: f17 Owners: dscott InitialCC: iarnell perl-sig
Doh! I missed something obvious. When you check this in, please add a BuildRequires: perl(ExtUtils::MakeMaker). At the minute, it's pulled in as a dependency of perl-Test-Simple via perl-devel - that may change in future as Test-Simple probably shouldn't require EU::MM. My bad, but no real problem.
Git done (by process-git-requests).
The process-git-requests script seems to have ignored your request for f17 branch. I guess it's very picky. For New Package requests, it seems to want "Branches:". You can fix it by making a Package Change Request - which does want "New Branches:".
Package Change Request ====================== Package Name: perl-MARC-Charset New Branches: f17 Owners: dscott InitialCC: iarnell perl-sig My bad for initially grabbing the template for "Package Change Request" and filling it out, then realizing that I needed the "New Package Change Request" template, but failing to modify "New Branches:" to "Branches:" as well. Thanks again Iain!
perl-MARC-Charset-1.33-1.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/perl-MARC-Charset-1.33-1.fc17
perl-MARC-Charset-1.33-1.fc17 has been pushed to the Fedora 17 testing repository.
perl-MARC-Charset-1.33-1.fc17 has been pushed to the Fedora 17 stable repository.