f15 koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2603530

Since I am not yet sponsored I can only do an informal review for this package.
Please note that since I am quite new here I may have overseen some details.

---

Informal review:

[+] = ok
[o] = does not apply
[-] = not ok


[+] The rpmlint output seems to be ok:

jemalloc.src: W: spelling-error Summary(en_US) malloc -> mallow, Mallorca, Mallory
jemalloc.src: W: spelling-error %description -l en_US malloc -> mallow, Mallorca, Mallory
jemalloc.x86_64: W: spelling-error Summary(en_US) malloc -> mallow, Mallorca, Mallory
jemalloc.x86_64: W: spelling-error %description -l en_US malloc -> mallow, Mallorca, Mallory

- I vote for Mallorca, but if you really want to you can stick to malloc. ;-)

jemalloc.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 65: warning: `UR' not defined
jemalloc.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 67: warning: `UE' not defined

- honestly, I have no idea what those warnings mean. I saw they were no
  blockers in other reviews, but this is a bit disappointing. Maybe someone
  can say more about these.

jemalloc.x86_64: W: no-manual-page-for-binary pprof

- would be nice to have one but no blocker.

2 packages and 1 specfiles checked; 0 errors, 7 warnings.

[+] The package is named according to the guidelines
[+] Spec file name matches base package name
[+] The package follows the Packaging Guidelines
[+] The license is an approved license (BSD)
[+] The License field matches the actual license
[+] License file from source file is included in %doc
[+] The spec file is written in American English
[+] The spec file is legible
[+] Used sources match width upstream sources (md5)
	rpmdev-md5 jemalloc-2.0.1-1.fc14.src.rpm 
	4687f59c073975f39375ece49c402bdc  jemalloc-2.0.1.tar.bz2
	md5sum jemalloc-2.0.1.tar.bz2 
	4687f59c073975f39375ece49c402bdc  jemalloc-2.0.1.tar.bz2
[+] Package build at least on one primary architecture (x86_64 tested)
[+] No architectures are known not to work
[+] All build dependencies are listed in the spec file (checked with mock)
[o] No locales for the package
[+] Package stores shared libraries and calls ldconfig in %post/%postun
[+] Package does not bundle copies of system libraries
[o] Package is not relocatable
[-] Package does not own all directories that are created:
	-devel sub-package creates %{_includedir}/jemalloc but doesn't own it
[+] No files are listed more then once in the %files section
[+] File permissions are set properly (%defattr(...) is used)
[+] Consistent use of macros
[+] Package contains code and documentation only, no content
[o] No large documentation files
[+] %doc files do not affect runtime
[+] No Header files included outside of the -devel package
[o] No static libraries included
[+] Library files ending with .so correctly in a -devel package
[-] -devel package should require the package correctly as %{name} =
%{version}-%{release} (missed the -%{release} part)
[+] No libtool .la archives included
[o] No GUI application
[+] Package does not own files or directories that are owned by other packages
[+] All filenames are valid UTF-8

---

Further comments:
- The description contains a lot of trademarks. As far as I can tell they are
  used properly, but I feel a bit uncomfortable with that. Don't you think
  that the users searching for this kind of package know who uses it for what?
  However, I _think_ it is safe like that but maybe others can comment on this.
- A dot would be the perfect ending to the description of the devel package.
- Extensive globbing in the file section is a dangerous thing; you might include
  files and dirs you don't want to include and it gets more legible if you are a
  bit more explicit. Especially when you glob just one file like in %{_mandir}/man3/*.
- Would do no harm to use the %{name} macro in the SOURCE0 tag.

---

Conclusion:
Yes, the package needs some fixing right now, but please note that I like
the clean look of the SPEC file and overall I think it is almost complete.

Here are some additional notes:

- the Group of the devel package should be Development/Libraries

- I suggest to use the default %description for the devel package:
  The %{name}-devel package contains libraries and header files for
  developing applications that use %{name}.

- the man3 manual page belongs to the devel package as it contains the API
  documentation of the library

- Drop %{_bindir}/pprof from the package since it's part of google-perftools.

- replace %{_libdir}/*so with %{_libdir}/*.so 
  (or even better with %{_libdir}/libjemalloc.so)

- The same files must not be packaged multiple times. So, drop all %doc files
  from the devel package.


(In reply to comment #2)
> jemalloc.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 65:
> warning: `UR' not defined
> jemalloc.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 67:
> warning: `UE' not defined

The two groff macros UR and UE are used to create hyperlinks. However, the man backend currently doesn't evaluate them, i.e. the target URL doesn't show up in the manpage. But that isn't a problem at all.

(In reply to comment #3)
> - The same files must not be packaged multiple times. So, drop all %doc files
>   from the devel package.

The important thing here is that you should not duplicate files in interdependent packages. -devel requires the main package, so the files are going to be present anyway.

btw.
 Requires:       %{name} = %{version}
should read
 Requires:       %{name} = %{version}-%{release}
unless you have an *extremely good* reason not to use a fully versioned Requires.

* Golo Fuchert wrote
> Since I am not yet sponsored I can only do an informal review for
> this package.  Please note that since I am quite new here I may have
> overseen some details.

Thank you for the effort, Golo, and welcome to Fedora!

> jemalloc.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 65:
> warning: `UR' not defined
> jemalloc.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 67:
> warning: `UE' not defined
>
> - honestly, I have no idea what those warnings mean. I saw they were
>   no blockers in other reviews, but this is a bit
>   disappointing. Maybe someone can say more about these.

These are caused because by som reason troff is unable to parse the
.UR (url start) and .UE (url end) tags correctly, though I can't see
why this happens. There are a lot of other packages using these tags,
where they seem they get parsed correctly. This could be caused
by some compatibility strangeness since the source is from the BSD
camp.

> jemalloc.x86_64: W: no-manual-page-for-binary pprof
> - would be nice to have one but no blocker.

I removed pprof, since it's part of another package, see comment #3.

> [-] Package does not own all directories that are created:
>  -devel sub-package creates %{_includedir}/jemalloc but doesn't own it

Fixed.

> [-] -devel package should require the package correctly as %{name} =
> %{version}-%{release} (missed the -%{release} part)

Fixed.

> Further comments:
> - The description contains a lot of trademarks. As far as I can tell
>   they are used properly, but I feel a bit uncomfortable with
>   that. Don't you think that the users searching for this kind of
>   package know who uses it for what?  However, I _think_ it is safe
>   like that but maybe others can comment on this.

I agree that they are uneccessary. Removed.

> - A dot would be the perfect ending to the description of the devel package.

Fixed.

> - Extensive globbing in the file section is a dangerous thing; you
>   might include files and dirs you don't want to include and it gets
>   more legible if you are a bit more explicit. Especially when you
>   glob just one file like in
>   %{_mandir}/man3/*.

Fixed.

> - Would do no harm to use the %{name} macro in the SOURCE0 tag.

Fixed.


* Martin Gieseking
> Here are some additional notes:
> - the Group of the devel package should be Development/Libraries

Fixed.

> - I suggest to use the default %description for the devel package:
>   The %{name}-devel package contains libraries and header files for
>   developing applications that use %{name}.

Works for me. Fixed.

> - the man3 manual page belongs to the devel package as it contains the API
>   documentation of the library

Fixed.

* Jussi Lehtola
> (In reply to comment #3)
> The important thing here is that you should not duplicate files in
> interdependent packages. -devel requires the main package, so the
> files are going to be present anyway.

Fixed.

> btw.
>  Requires:       %{name} = %{version}
> should read
>  Requires:       %{name} = %{version}-%{release}
> unless you have an *extremely good* reason not to use a fully versioned
> Requires.

Yep, I overlooked that one. Fixed.

Thank you too, Martin and Jussi. Updated srpm and specfile at http://users.linpro.no/ingvar/jemalloc

Ingvar

Hi Ingvar,

sorry for nagging again, but there are still some things to consider in your latest spec:

- please add a short comment above Patch0 telling what the patch does

- swap the Groups of base and devel package:
  base package:  System Environment/Libraries
  devel package: Development/Libraries

- Please replace %{_libdir}/libjemalloc.so.1 with 
  %{_libdir}/libjemalloc.so.* to simplify future soname bumps and to ensure 
  that all versioned library variants (if there will be any in the future) get
  packaged.

- drop %{_includedir}/jemalloc/* in %files devel, as the preceding line 
  %{_includedir}/jemalloc already adds the directory including all its contents

- replace %{_mandir}/man3/jemalloc.3.gz with %{_mandir}/man3/jemalloc.3*
  since we should not rely on a specific compression format applied by rpmbuild

* Martin Gieseking
> sorry for nagging again, but there are still some things to consider
> in your latest spec:

NP :-)


> - please add a short comment above Patch0 telling what the patch does

Fixed.

> - swap the Groups of base and devel package:
>   base package:  System Environment/Libraries
>   devel package: Development/Libraries

Gah! Stupid me. That _was_ my intention. Fixed.

> - Please replace %{_libdir}/libjemalloc.so.1 with 
>   %{_libdir}/libjemalloc.so.* to simplify future soname bumps and to ensure 
>   that all versioned library variants (if there will be any in the future) get
>   packaged.

Fixed.

> - drop %{_includedir}/jemalloc/* in %files devel, as the preceding line 
>   %{_includedir}/jemalloc already adds the directory including all its 
>   contents

Fixed.

> - replace %{_mandir}/man3/jemalloc.3.gz with %{_mandir}/man3/jemalloc.3*
>   since we should not rely on a specific compression format applied by 
>   rpmbuild

Fixed.

Updated srpm and specfile at http://users.linpro.no/ingvar/jemalloc

Ingvar

Hi Ingvar,

you should have bumped the Release number. ;-)
Besides that, the package looks fine now, and we can finish here.

$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm
jemalloc.src: W: spelling-error Summary(en_US) malloc -> mallow, Mallorca, Mallory
jemalloc.src: W: spelling-error %description -l en_US malloc -> mallow, Mallorca, Mallory
jemalloc.x86_64: W: spelling-error Summary(en_US) malloc -> mallow, Mallorca, Mallory
jemalloc.x86_64: W: spelling-error %description -l en_US malloc -> mallow, Mallorca, Mallory
jemalloc-devel.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 65: warning: macro `UR' not defined
jemalloc-devel.x86_64: W: manual-page-warning /usr/share/man/man3/jemalloc.3.gz 67: warning: macro `UE' not defined
4 packages and 0 specfiles checked; 0 errors, 8 warnings.

The above warnings can savely be ignored:
- spelling errors are false positive
- undefined groff macros UR/UE don't harm the proper display of the manpage


---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    BSD (2- and 3-clause variants)

[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum jemalloc-2.0.1.tar.bz2*
    4687f59c073975f39375ece49c402bdc  jemalloc-2.0.1.tar.bz2
    4687f59c073975f39375ece49c402bdc  jemalloc-2.0.1.tar.bz2.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
    koji scratch build:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=2608809

[.] MUST: If the package does not successfully compile, build or work on an architecture, ...
[.] MUST: All build dependencies must be listed in BuildRequires.
    - none required

[.] MUST: The spec file MUST handle locales properly. 
[+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[+] MUST: library files with a suffix must go in a -devel package.
[+] MUST: devel packages must require the base package using a fully versioned dependency
[+] MUST: Packages must NOT contain any .la libtool archives
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) ...
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: subpackages other than devel should require the base package
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin ...
[+] SHOULD: your package should contain man pages for binaries/scripts. 

----------------
Package APPROVED
----------------

New Package SCM Request
=======================
Package Name: jemalloc
Short Description: General-purpose scalable concurrent malloc implementation
Owners: ingvar
Branches: f13 f14 el5 el6
InitialCC:

Git done (by process-git-requests).

jemalloc-2.0.1-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/jemalloc-2.0.1-2.fc14

jemalloc-2.0.1-2.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/jemalloc-2.0.1-2.fc13

jemalloc-2.0.1-2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/jemalloc-2.0.1-2.el5

jemalloc-2.0.1-2.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update jemalloc'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/jemalloc-2.0.1-2.el5

jemalloc-2.0.1-2.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

jemalloc-2.0.1-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

jemalloc-2.0.1-2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.