Bug 485000 - Review Request: libbsr - Barrier Synchronization Register access library
Summary: Review Request: libbsr - Barrier Synchronization Register access library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-02-11 03:03 UTC by Tony Breeds
Modified: 2009-03-05 16:32 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-02-17 00:47:46 UTC
Type: ---
Embargoed:
bugs.michael: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Tony Breeds 2009-02-11 03:03:18 UTC
Spec URL: http://bakeyournoodle.com/~tony/fedora/libbsr/libbsr.spec
SRPM URL: http://bakeyournoodle.com/~tony/fedora/libbsr/libbsr-0.1-1.fc10.src.rpm
Description:
This is a library to expose the functionality of the Barrier Synchronization
Register (BSR) on IBM POWER Systems in Linux. This facility helps speed up
synchronization across large SMP systems

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1118653
RPMlint:
[tony@mango fedora]$ rpmlint -iv RPMS/*/libbsr* SRPMS/libbsr*
libbsr.ppc64: I: checking
libbsr-debuginfo.ppc64: I: checking
libbsr-devel.ppc64: I: checking
libbsr.src: I: checking
4 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 1 manuel wolfshant 2009-02-11 03:54:16 UTC
Please do not hardcode a specific sourceforge mirror in the Source0 tag. the recommended address is downloads.sourceforge.net. See https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

According to  https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment any patch should be submitted upstream for evaluation/inclusion (+ a comment with the URL of the bug report or whatever applies).

Comment 2 Tony Breeds 2009-02-11 04:06:42 UTC
(In reply to comment #1)
> Please do not hardcode a specific sourceforge mirror in the Source0 tag. the
> recommended address is downloads.sourceforge.net. See
> https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

Ahh thank you.  I have corrected the SPEC file at the same URL.
 
> According to 
> https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
> any patch should be submitted upstream for evaluation/inclusion (+ a comment
> with the URL of the bug report or whatever applies).

I'd updated the SPEC fil to indicate that I've asked for upstream to include the patch in the next release for this.  Next I'll try and decode sourceforge to file a real bug.

Comment 3 manuel wolfshant 2009-02-11 04:38:54 UTC
Another small fix is needed:
 "warning: File listed twice: /usr/share/doc/libbsr-0.1/examples"

Comment 4 Tony Breeds 2009-02-11 07:20:34 UTC
(In reply to comment #3)
> Another small fix is needed:
>  "warning: File listed twice: /usr/share/doc/libbsr-0.1/examples"

Hmm okay.  I can see how to fix that.  I  clearly need to read the rpmbuild output more clearly.

Comment 5 Tony Breeds 2009-02-11 22:57:32 UTC
(In reply to comment #4)

> Hmm okay.  I can see how to fix that.  I  clearly need to read the rpmbuild
> output more clearly.

Fixed, new scratch build here:
  https://koji.fedoraproject.or/koji/taskinfo?taskID=1120493

new SRPM and SPEC file at:
   http://bakeyournoodle.com/~tony/fedora/libbsr/

rpmlint is still clear:
[tony@mango fedora]$ rpmlint -vi SRPMS/libbsr*3.fc10.src.rpm RPMS/ppc64/libbsr*3.fc10.ppc64.rpm SPECS/libbsr.spec 
libbsr.src: I: checking
libbsr.ppc64: I: checking
libbsr-debuginfo.ppc64: I: checking
libbsr-devel.ppc64: I: checking
4 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 6 Tony Breeds 2009-02-12 02:49:01 UTC
Upstream have released v0.2 including the patch from the RPM.  I've respun with 0.2

New scratch build here:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1120621

new SRPM and SPEC file at:
   http://bakeyournoodle.com/~tony/fedora/libbsr/

rpmlint is still clear:
[tony@mango fedora]$ rpmlint -vi SRPMS/libbsr*0.2*rpm RPMS/ppc64/libbsr*0.2*rpm SPECS/libbsr.spec
libbsr.src: I: checking
libbsr.ppc64: I: checking
libbsr-debuginfo.ppc64: I: checking
libbsr-devel.ppc64: I: checking
4 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 7 Michael Schwendt 2009-02-12 12:22:23 UTC
The new guideline about comments on patches is a "SHOULD" item and not a MUST.

Even the SourceForge URLs are not mandatory. No reason to insist on getting them fixed prior to approval. It's not as if the url pointed to the interactive web page. Direct links to a mirror are just fine, since the various wget/curl compatible mirror-selection or round-robin dns urls don't always work for everyone either (and a reviewer ought to visit the upstream site anyway). Additionally, some SF.net projects publish their tarballs with different urls. The guidelines don't cover all cases, as they are more of a hint/recommendation than a strict rule.

The redundant %dir didn't cause any files to be mispackaged and didn't cause a build failure either.

[...]

The packaging is fine: APPROVED


[...]

Hints and minor issues that can be changed in pkg cvs:

* For %setup the "-n %name-%version" parameter is the default.

* You mix %_libdir and %_prefix/%_lib in %install section.

* Instead of

| %dir %{_defaultdocdir}/%{name}-%{version}/
| %{_defaultdocdir}/%{name}-%{version}/LGPL-2.1
| %{_defaultdocdir}/%{name}-%{version}/README

you could also use:

| %{_defaultdocdir}/%{name}-%{version}/
| %exclude %{_defaultdocdir}/%{name}-%{version}/examples/

Doesn't change much for this package, but could be helpful with other packages where more than two doc files shall be included.

Comment 8 manuel wolfshant 2009-02-12 12:43:47 UTC
http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles:
    A Fedora package must not contain any duplicate files in the %files listing.

Comment 9 Michael Schwendt 2009-02-12 13:07:29 UTC
And that really refers only to the rpmbuild "warning: File listed twice ..."? I have doubts about that (and I've done packaging guidelines stuff long enough). Let me explain:

1.) That guideline would only be somewhat important for actual files (and not directories). The rationale is simple: By mistake one can include a file more than once in the same %files section (which does not do any harm!) and not notice this when moving the file to the %files section of another subpackage. Then you end up with the same file in multiple packages. That's bad, as it can cause side-effects.

2.) That guideline would be more important if it referred to files listed in multiple subpackages. Because that is something rpmbuild does NOT detect.

So, once again, consider it a hint/documentation on what packagers might want to know about when creating packages.


Still no reason to require a package submitter to spend extra time on building fresh rpms just for this. Here it was a simple redundant %dir entry. It can be pointed out and fixed in cvs. It's not a blocker-criterion.

Comment 10 Tony Breeds 2009-02-13 00:22:46 UTC
New Package CVS Request
=======================
Package Name: libbsr
Short Description: Barrier Synchronization Register access library
Owners: tbreeds
Branches: F-9 F-10 EL-5
InitialCC:

Comment 11 manuel wolfshant 2009-02-13 04:30:02 UTC
Michael, since you did the review, could you please assign this bug to yourself ?

Comment 12 Kevin Fenzi 2009-02-13 06:32:18 UTC
cvs done.

Comment 14 Fedora Update System 2009-03-05 16:31:04 UTC
libbsr-0.2-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Fedora Update System 2009-03-05 16:32:04 UTC
libbsr-0.2-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.