[Bug 653682] Review Request: jemalloc - General-purpose scalable concurrent malloc(3) implementation

[bugzilla at redhat.com]

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=653682

--- Comment #5 from Ingvar Hagelund <ingvar at linpro.no> 2010-11-18 05:21:07 EST ---
* 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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.