Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: URCU - Userspace RCU Implementation
https://bugzilla.redhat.com/show_bug.cgi?id=717337
Summary: Review Request: URCU - Userspace RCU Implementation Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: unspecified Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: greenscientist@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Story Points: ---
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-1.src.rpm Description: Userspace RCU (read-copy-update) Implementation from LTTng UST. 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.
Needed by the UST - the LTTng Userspace tracer that will be posted shortly.
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=717337
--- Comment #1 from Yannick Brosseau greenscientist@gmail.com 2011-06-28 11:33:52 EDT --- This is my first Fedora package, so I'll need a sponsor.
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=717337
Yannick Brosseau greenscientist@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
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=717337
Yannick Brosseau yannick.brosseau@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |717748
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=717337
Veeti Paananen veeti.paananen@rojekti.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |veeti.paananen@rojekti.fi
--- Comment #2 from Veeti Paananen veeti.paananen@rojekti.fi 2011-06-29 15:25:27 EDT --- 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).
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=717337
--- Comment #3 from Yannick Brosseau yannick.brosseau@gmail.com 2011-06-29 16:38:39 EDT --- (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.sr...
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=717337
Paolo Bonzini pbonzini@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |pbonzini@redhat.com
--- Comment #4 from Paolo Bonzini pbonzini@redhat.com 2011-07-01 08:21:33 EDT --- 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.
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=717337
--- Comment #5 from Yannick Brosseau yannick.brosseau@gmail.com 2011-07-05 17:19:00 EDT --- (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
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=717337
--- Comment #6 from Yannick Brosseau yannick.brosseau@gmail.com 2011-09-01 15:51:57 EDT --- 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.sr...
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=717337
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mschwendt@gmail.com
--- Comment #7 from Michael Schwendt mschwendt@gmail.com 2011-09-18 08:00:59 EDT --- 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_S...
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_Librari...
# rpmlint *
liburcu.x86_64: W: shared-lib-calls-exit /usr/lib64/liburcu-qsbr.so.1.0.0 exit@GLIBC_2.2.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
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=717337
--- Comment #8 from Paolo Bonzini pbonzini@redhat.com 2011-09-19 05:42:32 EDT ---
liburcu.x86_64: W: shared-lib-calls-exit /usr/lib64/liburcu-qsbr.so.1.0.0 exit@GLIBC_2.2.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.
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=717337
--- Comment #9 from Yannick Brosseau yannick.brosseau@gmail.com 2011-09-19 16:06:53 EDT --- (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@GLIBC_2.2.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.
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=717337
--- Comment #10 from Yannick Brosseau yannick.brosseau@gmail.com 2011-10-11 17:16:34 EDT --- 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.sr...
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.
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=717337
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |mschwendt@gmail.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=717337
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW CC|mschwendt@gmail.com | Blocks|177841(FE-NEEDSPONSOR) | AssignedTo|mschwendt@gmail.com |nobody@fedoraproject.org
--- Comment #11 from Michael Schwendt mschwendt@gmail.com 2011-11-19 08:59:06 EST --- 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_S... https://fedoraproject.org/wiki/Packager_sponsor_responsibilities ]
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=717337
--- Comment #12 from Bill Nottingham notting@redhat.com 2012-02-28 13:13:42 EST ---
- 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@GLIBC_2.2.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.
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=717337
Bill Nottingham notting@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |notting@redhat.com Flag| |fedora-review?
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=717337
--- Comment #13 from Bill Nottingham notting@redhat.com 2012-02-28 13:55:36 EST --- #2 (liburcu-devel requiring pkgconfig) is picked up already, not an issue.
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=717337
--- Comment #14 from Yannick Brosseau yannick.brosseau@gmail.com 2012-03-05 16:30:19 EST --- 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.sr...
package-review@lists.fedoraproject.org