Bug 620177

Taking for review.

> License:        GPLv2+
How did you determine that it is GPLv2+? uprof.c, uprof.h, and uprof-private.h all have LGPLv2+ headers and there's an LGPL COPYING file too.

> find $RPM_BUILD_ROOT -name '*.a' -exec rm -f {} ';'
It'd be nicer to pass --disable-static to configure so that the .a files don't get built in the first place.

> BuildRequires:  gtk-doc
> BuildRequires:  gnome-doc-utils
Are these buildrequires necessary? There doesn't appear to be anything built with gtk-doc in the final rpms.

The COPYING file is included in both base package and in the -devel subpackage. As per https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Subpackage_Licensing you can omit the COPYING file in -devel as it's dependant upon the base package.

(In reply to comment #1)
> Taking for review.

Thanks!

> > License:        GPLv2+
> How did you determine that it is GPLv2+? uprof.c, uprof.h, and uprof-private.h
> all have LGPLv2+ headers and there's an LGPL COPYING file too.

D'oh .. your right it is indeed LGPLv2+

> > find $RPM_BUILD_ROOT -name '*.a' -exec rm -f {} ';'
> It'd be nicer to pass --disable-static to configure so that the .a files don't
> get built in the first place.

Well yeah, but as the current build system is broken I'd had to patch it for "--disable-static" to work; and I'd rather avoid non upstream patches unless when possible.

> > BuildRequires:  gtk-doc
> > BuildRequires:  gnome-doc-utils
> Are these buildrequires necessary? There doesn't appear to be anything built
> with gtk-doc in the final rpms.

Missing configure switch; docs are now being built. (it complains about missing gtk-doc even when no docs are being built).

> The COPYING file is included in both base package and in the -devel subpackage.
> As per
> https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Subpackage_Licensing
> you can omit the COPYING file in -devel as it's dependant upon the base
> package.    

Good point.

New spec / srpm :
http://193.200.113.196/apache2-default/rpm/uprof.spec
http://193.200.113.196/apache2-default/rpm/uprof-0.2-0.2.b620fb7f9.fc13.src.rpm

(In reply to comment #2)
> (In reply to comment #1)
> > > find $RPM_BUILD_ROOT -name '*.a' -exec rm -f {} ';'
> > It'd be nicer to pass --disable-static to configure so that the .a files don't
> > get built in the first place.
> 
> Well yeah, but as the current build system is broken I'd had to patch it for
> "--disable-static" to work; and I'd rather avoid non upstream patches unless
> when possible.

"avoid non upstream patches"? Is upstream not accepting patches? Anyway, I tried it out with the following changes to the spec file:
-%configure --enable-gtk-doc
+%configure --disable-static --enable-gtk-doc

 find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';'
-find $RPM_BUILD_ROOT -name '*.a' -exec rm -f {} ';'

... and the scratch build succeeded, so I don't really believe there's something wrong with the way the build system handles --disable-static:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2374115

Also, I'd like to comment on two added requires in the devel package (no need to change anything, I just feel verbose today):
> Requires:       gtk-doc
This is fine right now, but keep an eye on
https://bugzilla.redhat.com/show_bug.cgi?id=604169#c28

> Requires:       pkgconfig
This line is only needed if you plan to use the same spec file in EPEL 4 or EPEL 5; in current Fedora releases rpm adds the pkgconfig dep automatically.

Fedora review uprof-0.2-0.2.b620fb7f9.fc13.src.rpm 2010-08-05

+ OK
! needs attention

rpmlint output:
$ rpmlint uprof uprof-devel uprof-0.2-0.2.b620fb7f9.fc15.src.rpm uprof-debuginfo-0.2-0.2.b620fb7f9.fc15.i686.rpm
uprof.i686: W: unused-direct-shlib-dependency /usr/lib/libuprof-0.2.so.0.0.0 /lib/librt.so.1
uprof.src: W: invalid-url Source0: uprof-0.2-b620fb7f9.tar.bz2
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

+ Rpmlint warnings are harmless and can be ignored
! The Release tag doesn't follow Package Naming Guidelines.
Instead of "0.2.b620fb7f9" you should use "0.2.YYYYMMDDgitb620fb7f9" or just plain "0.2.YYYYMMDDgit" as per https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

+ Spec file name matches the base package name
! Actual compiler commands aren't visible in the build logs
Please turn off libtool "shave" mode so that it'd be possible to check compiler flags. It's usually enough to pass V=1 to make command.

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains license file (COPYING)
+ Spec file is written in American English
+ Spec file is legible
+ Following instructions in the spec file to check out sources
  from upstream git repo produce matching tarball. md5sum:
  c6d435870591f04202a3c262c46df5d0  uprof-0.2-b620fb7f9.tar.bz2
  c6d435870591f04202a3c262c46df5d0  uprof.git/uprof-0.2-b620fb7f9.tar.bz2

+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file MUST handle locales properly
+ ldconfig is properly called in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all directories it creates
+ No duplicate files in %files
+ Permissions are properly set and %files has %defattr
+ Consistent use of macros
+ The package must contain code, or permissable content.
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc don't affect the package
+ Header files are in -devel
n/a Static libraries should be in -static
+ Library files that end in .so are in -devel package
+ -devel requires the fully versioned base
+ Package doesn't contain any libtool .la files
n/a Packages containing GUI apps must include %{name}.desktop file
+ Directory ownership sane
+ Filenames are valid UTF-8


It would also be nice if you could avoid building the static library by using --disable-static as mentioned in comment #3, but this isn't a review blocker.

Please address the issues mentioned in this comment and I'll evaluate the package for final approval.

Sorry for the late response; I have addressed the noted issues.

> (In reply to comment #3)
> ... and the scratch build succeeded, so I don't really believe there's
> something wrong with the way the build system handles --disable-static:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=2374115

Indeed not sure what was going on before, so your right it works fine just confirmed it.


New spec / srpm :
http://193.200.113.196/apache2-default/rpm/uprof.spec
http://193.200.113.196/apache2-default/rpm/uprof-0.2-0.3.b620fb7f9.fc13.src.rpm

Looks like you missed this paragraph in comment #4:
> ! The Release tag doesn't follow Package Naming Guidelines.
> Instead of "0.2.b620fb7f9" you should use "0.2.YYYYMMDDgitb620fb7f9" or just
> plain "0.2.YYYYMMDDgit" as per
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

(In reply to comment #6)
> Looks like you missed this paragraph in comment #4:
> > ! The Release tag doesn't follow Package Naming Guidelines.
> > Instead of "0.2.b620fb7f9" you should use "0.2.YYYYMMDDgitb620fb7f9" or just
> > plain "0.2.YYYYMMDDgit" as per
> > https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages    

Neither makes sense .... the git hash is an unique identifier so it tells exactly which upstream snapshot that is.

Adding the date does not add any value here neither does not using the hash at all.

I'd argue that using the date in release field gives the user a better sense how fresh the snapshot is. The git hash is only useful if one can look it up in the git repo; without the repo the hash isn't very informative.

In any case, it's the official guidelines which suggest to include date in the release field if you are packaging a snapshot. I think it makes sense to keep naming consistent in the distro. Even if you think that including the date + the word "git" in release field is redundant, I believe it is useful to keep naming consistent with other packages.

(In reply to comment #8)
> I'd argue that using the date in release field gives the user a better sense
> how fresh the snapshot is. The git hash is only useful if one can look it up in
> the git repo; without the repo the hash isn't very informative.

True but that is what the changelog is for; we don't add the date to "foo-2.0-1" either even thought it would tell the user when "2.0" has been released.

> In any case, it's the official guidelines which suggest to include date in the
> release field if you are packaging a snapshot. I think it makes sense to keep
> naming consistent in the distro. Even if you think that including the date +
> the word "git" in release field is redundant, I believe it is useful to keep
> naming consistent with other packages.    

Anyway yeah you are right; changing this is beyond the scope of a package review, so I have added the date and "git" to the release.

New srpm / spec:
http://193.200.113.196/apache2-default/rpm/uprof.spec
http://193.200.113.196/apache2-default/rpm/uprof-0.2-0.4.20100808gitb620fb7f9.fc13.src.rpm

Looks good.

APPROVED

New Package SCM Request
=======================
Package Name: uprof
Short Description: Profiling toolkit
Owners: drago01
Branches: f12 f13 f14
InitialCC:

Git done (by process-git-requests).