Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 196824 (php-pear-Mail-Mime)
Summary: | Review Request: php-pear-Mail-Mime | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Brandon Holbrook <holbrookbw> |
Component: | Package Review | Assignee: | Christopher Stone <chris.stone> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | ||
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-06 13:45:08 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779, 189195 |
Description
Brandon Holbrook
2006-06-27 02:22:42 UTC
MUST ITEM CHECKLIST: - rpmlint output: W: php-pear-Mail_Mime invalid-license PHP License W: php-pear-Mail_Mime invalid-license PHP License Warnings can be ignored O package is not named according to php packaging naming guidelines SHOULD be php-pear-Mail-Mime - spec file name matches %{name} %{name} needs to be renamed to php-pear-Mail-Mime - package meets packaging guidelines - package is licensed with open source compatible license - license field matches actual license - source contains license file in %doc - spec file is in American English - spec file is legible - sources match upstream 0012fd2406597e60083bc4bce751cef2 Mail_Mime-1.3.1.tgz - package successfully compiles and builds on FC-5 x86_64 O The %post and %postun should have: php-pear >= 1.4.9 - package does not use locales - package does not contain shared libraries in default paths (no need to run ldconfig) - package is not relocatable - package does not own all directories it creates Package should own /usr/share/pear/Mail directory - package does not contain any duplicate %files - permissions are set properly - package contains proper %clean section - macro usage is consistant - package contains permissible content - package does not have large documentation - package does not include header files or static libraries - package does not use pkgconfig files - package does not contain library with suffix - package does not require a devel subpackage - package does not contain any .la files - package is not a gui and does not need a .desktop file - package does not own files or directories owned by other packages MUST - Must change package name to php-pear-Mail-Mime (all other occurances of Mail_Mime are okay, just %{name} and the filename should be changed) - Package must own %{peardir}/Mail/ add %dir %{peardir}/Mail - Must Add: Requires(post): php-pear >= 1.4.9 Requires(postun): php-pear >= 1.4.9 Currently you do not provide the version numbers and since 1.4.9 is required for the options used in these sections they should be added. - Change %defattr to (-,root,root,-) I have added the version dependencies for (post) and (postun), as well as modified the defattr. I have no problem changing the package name or owning the Mail/ directory, but have some questions first: I named this package using FE's guidelines for perl/cpan packages 'perl-CPANDIST' where CPANDIST is the name of the module in CPAN. I adopted this for pear to read 'php-pear-PEARDIST'. Since Mail_Mime is not a sub-package of Mail, using a '-' to delimit Mail and Mime seems misleading to me. Mail_Mime does not depend on Mail, but Mail-Mime looks like it might. I'd love to change it if you still feel the 'no underscore' rule precludes all other naming conventions, just putting in my 0.02 :) Also, and along the same lines, owning the Mail/ directory seems like a bad idea to me. Pear contains many Mail_xxx packages (http://pear.php.net/search.php?q=mail&in=packages&x=0&y=0), none of which depend on the Mail package, but (I assume) all of which place files in the Mail/ directory. It's hard to say which of these packages should own Mail/, but we also can't rely on Mail to own it, as one could easily have Mail_Mime installed without installing Mail. (For FE, we could force the Mail package to own Mail/ and then force Mail_xxx packages to require Mail even though they don't...) The packages should be delimited with dashes not underscores, the perl packages do the same thing, it's just a naming convention and has nothing to do with dependencies. From the naming guidelines: "When naming packages for Fedora, the maintainer should use the dash '-' as the delimiter for name parts. The maintainer should NOT use an underscore '_', a plus '+', or a period '.' as a delimiter." As far as owning the directories, all packages that use /usr/share/php/Mail should own this directory. I discussed this with a FESCo member and this is what he told me to do. This will have to be added in the PHP packaging guidelines somewhere, as many packages like Net/ Image/ Auth/ etc are like this. Basically if there is no clear hierarchy, then all packages must own the directory. Spec URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime.spec SRPM URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime-1.3.1-3.src.rpm Sounds like great rationale to me! All points are done. All MUST items have been fixed. APPROVED. One final note, please add a comment in the spec file for why you add quotes around the word 'install'. It should be noted in comments in the spec file that you are doing this to surpress rpmlint warnings. Great! Comment added. Now what do I have to do about getting sponsored? Oh I cannot approve this package until you are sponsered, changing the status back to FE-REVIEW. I'll see if I can get tibbs to sponser you. Does one of your bugs block FE-NEEDSPONSER? Yes, bug 181445 You can get sponsered more quickly by doing reviews other people's packages. I have some php packages up for review at: http://fedoraproject.org/wiki/ChristopherStone (check the "Package Im working on" section) The Requires Line should read: Requires: php php-pear(PEAR) php-pear(Net_SMTP) Add a dependency on the php-pear-Net-SMTP review. The version requirement for php-pear is not needed in Requires. Actually, please change it to: Requires: php pear-php(PEAR) Requires(hint): php-pear(Net_SMTP) Please ignore comments #10 and #11 I got confused on the dependencies because the URL field is incorrect. The URL field should be: http://pear.php.net/package/Mail_Mime So basically all you should have is: Requires: php pear-php(PEAR) erm I mean php-pear, not pear-php (getting late here, time for sleep) Spec URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime.spec SRPM URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime-1.3.1-5.src.rpm ...just to show I've done these last few items :) Just out of curiosity, what's the difference between php-pear and php-pear(PEAR)? php-pear(PEAR) is supposed to mean it needs the "PEAR class" So, one last thing I've noticed, you should have: BuildRequires: php-pear >= 1:1.4.9 version 1:1.4.9 is needed for --packagingroot lol, done Spec URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime.spec SRPM URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime-1.3.1-6.src.rpm Hi Brandon, Please update your spec file based off of the following template: https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=135472 More information here: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=198706 Spec URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime.spec SRPM URL: http://theholbrooks.org/RPMS/php-pear-Mail-Mime-1.3.1-7.src.rpm Mail_Mime has been redone to comply with the template provided, with a few exceptions (mostly to raise questions to you fine folks and elicit feedback). Mail_Mime doesn't come with _any_ documentation, so I've kept the PHP license file from my last revision as SOURCE1 and install it where appropriate (licence files are good, though perhaps unneeded with pear packages? and rpmlint whines when you don't have any docs) Also, I've kept the (mostly pointless) %check section around because it's not hurting anything and _some_ check seems better than nothing... shall I delete it for uniformity's sake? I also kept 'install' quoted to appease rpmlint. Lastly, I read in the discussion that we needed to BR: php-pear >= 1:1.4.9-1.2, but the 1.2 doesn't appear in the template file. Is that an oversight? Installing the license file is still okay and encouraged. The template does not include it because it cannot make assumptions on what the license is. Keeping the %check section is fine and doesn't need to be removed. Please remove the quotes from install. rpmlint is going to be fixed, infact it is already fixed, it just needs to be released to the public. The new BR was added to the cvs. Everything else looks good. Approved. One final note, you may want to remove the "PEAR:" from the Summary. The package name already indicates it is a pear package. Removed the quotes from install and added the strict-er BR: I also removed PEAR: from the summary, though it appears that many (if not all) existing PEAR modules use this prefix in their summary, maybe we should standardize? Imported and successfully built. |