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) - Review Request: php-pear-Mail-Mime
Summary: Review Request: php-pear-Mail-Mime
Keywords:
Status: CLOSED NEXTRELEASE
Alias: php-pear-Mail-Mime
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christopher Stone
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 189195
TreeView+ depends on / blocked
 
Reported: 2006-06-27 02:22 UTC by Brandon Holbrook
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-09-06 13:45:08 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Brandon Holbrook 2006-06-27 02:22:42 UTC
Spec URL: http://theholbrooks.org/RPMS/php-pear-Mail_Mime.spec
SRPM URL: http://theholbrooks.org/RPMS/php-pear-Mail_Mime-1.3.1-2.src.rpm
Description: A Package to enable easy creation of complex multipart emails. If you look for a simple API for creating such emails, then Mail_Mime class will probably suffice. Else you can use Mail_mimePart, which gives you better control about MIME creation.

This .spec file is 95% that found in php-pear-Mail by Remi Collet, approved 4 weeks ago.  This PEAR module is required by horde, also up for review at bug 189195.  Also should be noted, I still need a sponsor.

Comment 1 Christopher Stone 2006-06-27 23:31:41 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,-)


Comment 2 Brandon Holbrook 2006-06-28 01:03:39 UTC
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...)

Comment 3 Christopher Stone 2006-06-28 01:11:13 UTC
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.

Comment 4 Brandon Holbrook 2006-06-28 02:55:49 UTC
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.

Comment 5 Christopher Stone 2006-06-28 03:02:53 UTC
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.

Comment 6 Brandon Holbrook 2006-06-28 03:47:56 UTC
Great!  Comment added.  Now what do I have to do about getting sponsored?

Comment 7 Christopher Stone 2006-06-28 03:58:53 UTC
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?

Comment 8 Brandon Holbrook 2006-06-28 04:00:57 UTC
Yes, bug 181445

Comment 9 Christopher Stone 2006-06-28 04:11:29 UTC
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)


Comment 10 Christopher Stone 2006-06-28 09:33:51 UTC
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.

Comment 11 Christopher Stone 2006-06-28 09:35:19 UTC
Actually, please change it to:
Requires: php pear-php(PEAR)
Requires(hint): php-pear(Net_SMTP)


Comment 12 Christopher Stone 2006-06-28 10:03:07 UTC
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)


Comment 13 Christopher Stone 2006-06-28 10:04:24 UTC
erm I mean php-pear, not pear-php (getting late here, time for sleep)

Comment 14 Brandon Holbrook 2006-06-29 01:39:20 UTC
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)?

Comment 15 Christopher Stone 2006-06-29 01:49:32 UTC
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


Comment 17 Christopher Stone 2006-09-04 00:30:15 UTC
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


Comment 18 Brandon Holbrook 2006-09-06 04:34:22 UTC
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?

Comment 19 Christopher Stone 2006-09-06 04:55:24 UTC
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.

Comment 20 Christopher Stone 2006-09-06 04:57:44 UTC
One final note, you may want to remove the "PEAR:" from the Summary.  The
package name already indicates it is a pear package.

Comment 21 Brandon Holbrook 2006-09-06 13:45:08 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.