Bug 717337

This is my first Fedora package, so I'll need a sponsor.

Some comments:

1. The spec file should be named after the package (so it should be "liburcu.spec").

2. The License should be "LGPLv2+" instead.

3. The Release tag should be in the format "1%{?dist}".

4. The build should be done with "make %{?_smp_mflags}".

5. The libraries seem to include rpaths (http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath).

(In reply to comment #2)
> Some comments:

Thank you for the feedbacks,

I've posted an update spec and srpm:

http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/liburcu.spec
http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/liburcu-0.6.3-1.fc15.src.rpm

An alternative way to fix the rpath problem is to rebuild autoconf-generated files at RPM build time.  This has the advantage of incorporating bugfixes to autoconf/automake/libtool automatically.

So that would be

BuildRequires:  pkgconfig, autoconf, automake, libtool
...
%build
autoreconf -fvi
%configure

without the sed lines mentioned in the packaging guidelines.  I'll let you and other reviewers decide the preferred resolution.

(In reply to comment #4)
> 
> without the sed lines mentioned in the packaging guidelines.  I'll let you and
> other reviewers decide the preferred resolution.

Looks cleaner. I'll see if there is other opinions on the subject

I've put a new SPEC and SRPM with small fixes

Spec URL: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/urcu.spec 
SRPM URL:
http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/liburcu-0.6.3-2.fc15.src.rpm

Several of the findings in comment 2 have not been added to the spec file and have not been commented on either. Please respond to reviewers' comments even if you disagree with them.


> License:        LGPL v2 or later

The correct license identifier really is "LGPLv2+" as pointed out in comment 2. The related guidelines are these:

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Valid_License_Short_Names


Writing this comment I noticed the linked spec file is out-of-date and doesn't match the latest src.rpm. Hmmm... continueing with the src.rpm then:


> License:        LGPLv2

So, same comment as above applies. ;)


> Name:           liburcu
> Group:          Development/Libraries

Dunno whether or when RPM will get rid of these Group tags (if at all), but library base packages typically belong into

  Group: System Environment/Libraries


> %description
> Userspace RCU (Read-Copy-Update) Implementation from the LTTng project.

Very brief and reads more like a summary. The top lines at http://lttng.org/urcu/ contain a somewhat more detailed description that could be copied and modified slightly to build a more detailed description:

| This package contains liburcu, a userspace RCU (read-copy-update)
| library. This data synchronization library provides read-side access
| which scales linearly with the number of cores. It does so by allowing
| multiples copies of a given data structure to live at the same time,
| and by monitoring the data structure accesses to detect grace periods
| after which memory reclamation is possible.

What do you think?


> ExclusiveArch:  %ix86 x86_64 ppc ppc64 s390 s390x

Based on
http://fedoraproject.org/wiki/Architectures#ExcludeArch_.26_ExclusiveArch
I recommend dropping this, especially since no spec file comment gives a strong rationale.


> %package -n liburcu-devel
> Requires:       liburcu = %{version}-%{release}

Be aware of %{?_isa} having entered the guidelines as a MUST item:
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> autoreconf -fvi

No strong feelings here. Just know that depending on what versions of the GNU Autotools may be required by the liburcu build files, a full autoreconf may cause broken builds. Sometimes without terminating the RPM package build job.


> make  %{?_smp_mflags}

For more verbose build.log output, this one works:

  V=1 make %{?_smp_mflags}


> %files -n liburcu-devel
> %{_prefix}/include/*

Note that %{_includedir} exists, too, and is the one set by the %configure macro.

As convenient as wildcards may be, with some packages, it can also be beneficial to be a little bit more specific about what file names to include, e.g.

  %{_includedir}/urcu*

or even

  %{_includedir}/urcu/
  %{_includedir}/urcu*.h

would implicitly protect against unexpected renames during package version upgrades. You would learn about substantial changes below %_includedir due to the build failing. Not mandatory, of course.


> %{_libdir}/*.a

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> # rpmlint *

> liburcu.x86_64: W: shared-lib-calls-exit /usr/lib64/liburcu-qsbr.so.1.0.0
> exit.5

and several more. Please find out why/when it calls exit and whether you can get rid of this.


> liburcu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/userspace-rcu-0.6.3/urcu/list.h
> liburcu-devel.x86_64: E: incorrect-fsf-address /usr/include/urcu/rcuhlist.h
> liburcu-devel.x86_64: E: incorrect-fsf-address /usr/include/urcu/rculist.h
> liburcu-devel.x86_64: E: incorrect-fsf-address /usr/include/urcu/list.h

Please try to get this fixed in the upstream tarball. 0.6.4 is available, btw.


> %doc README LICENSE

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

>> liburcu.x86_64: W: shared-lib-calls-exit /usr/lib64/liburcu-qsbr.so.1.0.0
>> exit.5
>
> and several more. Please find out why/when it calls exit and whether you can
> get rid of this.

Looks like it's called if pthread_mutex_{lock,unlock} fails, which shouldn't happen.  Not nice, though.

(In reply to comment #7)
> Several of the findings in comment 2 have not been added to the spec file and
> have not been commented on either. Please respond to reviewers' comments even
> if you disagree with them.

Sorry, it seems I've missed them. Will address those and your new comments with a new package soon. 


> > liburcu.x86_64: W: shared-lib-calls-exit /usr/lib64/liburcu-qsbr.so.1.0.0
> > exit.5
> 
> and several more. Please find out why/when it calls exit and whether you can
> get rid of this.

These have been reported upstream and is being worked on. 
 
> 
> > liburcu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/userspace-rcu-0.6.3/urcu/list.h
> 
> Please try to get this fixed in the upstream tarball. 0.6.4 is available, btw.

These have been reported and fixed upstream.

Here is the updated version following your comments. Including the new release. 

http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/liburcu.spec

http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/liburcu-0.6.5-1.fc15.src.rpm

The exits call are still present, but they should be fixed in the next upstream release. 

I've decided to put back the sed to fix the rpath as they seems less risky.

I've just discovered that Bill Nottingham (notting) has sponsored you already, so I'm giving back the tickets. :-/

[ Note that I haven't found the first package approval, so something about the process might not be right here.
  https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored 
  https://fedoraproject.org/wiki/Packager_sponsor_responsibilities
]

- Package meets naming and packaging guidelines ***

Upstream tarball is named userspace-rcu. Package is named liburcu.

I'm inclined to let it go, though.

- Spec file matches base package name. - OK
- Spec has consistant macro usage.  - OK
- Meets Packaging Guidelines. - OK
- License - LGPLv2+
- License field in spec matches - OK
- License file included in package - ***

License file (LICENSE) isn't in package's %doc.

- Spec in American English - OK
- Spec is legible. - OK
- Sources match upstream md5sum:  - OK
a455ea20ca7fc4f259f7b7fd92f0e975da8f0f19  userspace-rcu-0.6.5.tar.bz2

- Package needs ExcludeArch - has it for mips, which we don't support -> OK
- BuildRequires correct - OK
- Spec handles locales/find_lang - N/A
- Package is relocatable and has a reason to be. - N/A
- Package has %defattr and permissions on files is good. ***

Permissions are fine. %defattr not included, not needed unless you're backporting to older ELs.

- Package has a correct %clean section. ***

%clean not included, but not needed unless you're backporting to older ELs.

- Package is code or permissible content. - N/A
- Doc subpackage needed/used. - N/A
- Packages %doc files don't affect runtime. - OK

- Headers/static libs in -devel subpackage. - OK
- Spec has needed ldconfig in post and postun - OK
- .pc files in -devel subpackage/requires pkgconfig - ***

liburcu-devel should have Requires: pkgconfig

- .so files in -devel subpackage. - OK
- -devel package Requires: %{name} = %{version}-%{release}  - OK
- .la files are removed. - OK

- Package compiles and builds on at least one arch. - OK (tested x86_64)
- Package has no duplicate files in %files. - OK
- Package doesn't own any directories other packages own. - OK
- Package owns all the directories it creates. - ***

Package should own %{includedir}/urcu

- No rpmlint output.

liburcu.x86_64: W: spelling-error Summary(en_US) Userspace -> User space, User-space, Users pace
Can be ignored (can also be fixed if you're bored).

liburcu.x86_64: W: shared-lib-calls-exit /usr/lib64/liburcu.so.1.0.0 exit.5

Does not need fixed, but can be impolite.

- final provides and requires are sane: - OK, modulo pkgconfig item above

SHOULD Items:

- Should build in mock. - OK
- Should build on all supported archs - tested x86_64
- Should function as described. - didn't test
- Should have sane scriptlets. - OK
- Should have subpackages require base package with fully versioned depend. - OK
- Should have dist tag - OK
- Should package latest version  ***

Latest is 0.6.7.

Issues:

1. Package license file
2. liburcu-devel should have Requires: pkgconfig
3. Package should own %{includedir}/urcu
4. Should upgrade to 0.6.7.
5. %defattr/%clean can be included if you want, but not required.

#2 (liburcu-devel requiring pkgconfig) is picked up already, not an issue.

Updated version available following your comments

http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/liburcu.spec
http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/liburcu-0.6.7-1.fc15.src.rpm

Is there anything missing to finish this review?

*** This bug has been marked as a duplicate of bug 529861 ***