Bug 610079

Summary: Review Request: bindex - Bundle Manifest Header Mapper
Product: [Fedora] Fedora Reporter: Victor G. Vasilyev <victor.vasilyev>
Component: Package ReviewAssignee: Stanislav Ochotnicky <sochotni>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting, sochotni
Target Milestone: ---Flags: sochotni: fedora‑review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-07-30 20:38:45 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 610153    

Description Victor G. Vasilyev 2010-07-01 10:14:12 EDT
Spec URL: http://victorv.fedorapeople.org/files/bindex.spec
SRPM URL: http://victorv.fedorapeople.org/files/bindex-2.2-1.fc14.src.rpm
Description: 
A Java program that implements the manifest header to repository 
format mapping as described in the RFC-0112 Bundle Repository. [1]

Note, the NetBeans 6.9 [2] depends on this package.

[1] http://www.osgi.org/download/rfc-0112_BundleRepository.pdf
[2] https://fedoraproject.org/wiki/Features/NetBeans_6.9
Comment 1 Victor G. Vasilyev 2010-07-01 10:24:36 EDT
Scratch koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2286662

Results of the rpmlint tool:
----------------------------

$ rpmlint bindex-2.2-1.fc14.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint bindex-2.2-1.fc14.src.rpm
bindex.src: W: invalid-url Source0: bindex.r96.svn.tar.gz
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

The warning due to 
https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
Comment 2 Victor G. Vasilyev 2010-07-25 10:41:56 EDT
The second release is prepared for review:

Spec URL: http://victorv.fedorapeople.org/files/bindex.spec
SRPM URL: http://victorv.fedorapeople.org/files/bindex-2.2-2.fc14.src.rpm

Scratch koji build:  http://koji.fedoraproject.org/koji/taskinfo?taskID=2349592

$ rpmlint SRPMS/bindex-2.2-2.fc14.src.rpm RPMS/noarch/bindex-2.2-2.fc14.noarch.rpm
bindex.src: W: invalid-url Source0: bindex.r96.svn.tar.gz
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

The warning due to 
https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
Comment 3 Stanislav Ochotnicky 2010-07-28 08:56:57 EDT
I can do the review, but it seems like you will need to have a look into https://fedoraproject.org/wiki/Packaging:NamingGuidelines#SnapshotPackages

Maybe I am wrong and package version/release is correct but it seems like there should be some changes. I'll let you know.
Comment 4 Stanislav Ochotnicky 2010-07-28 09:48:05 EDT
OK: rpmlint must be run on every package. The output should be posted in the review.
bindex.src: W: invalid-url Source0: bindex.r96.svn.tar.gz
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

NEEDSWORK: The package must be named according to the Package Naming Guidelines .

I am not sure how you got to "Version: 2.2". I couldn't find anything
in the archive/homepage suggesting that's the last version of the
package. This seems like the snapshot pre-release of version 0. So it
should be something like:

Version: 0
Release: 0.1.svn96%{?dist}

You are welcome to prove me wrong. One way or the other it would be
nice to get in touch with upstream and get them to actually release
versioned binary release (e.g. bindex-%{version}.zip/tar.xx)

OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.  .
OK: The package must meet the Packaging Guidelines .
OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
OK: The License field in the package spec file must match the actual license. 
OK: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
OK: All independent sub-packages have License of their own (if it exists)
OK: The spec file must be written in American English. 
NEEDSWORK: The spec file for the package MUST be legible.

You define a LOT of macros that are only used once:
 * svnRev/svnURL
 * bnd
 * installJAR
 * rmFiles/rmFiles_lst

Please don't do this, it just makes the spec file harder to read
without adding any benefit. I know it can be tempting to treat spec
file as a bash script, but think of it more as a "recipe" where you
just define the ingredients and few hints how to cook it :-) Make it
as simple as possible.

Plus one more thing. Instead of creating lnSysJar macro, use
build-classpath or build-classpath-directory commands. They have been
created especially for this situation. I noticed you used
build-classpath in the spec file...so maybe you just didn't know about
-directory version of it? I know it doesn't work well with renames
when creating symlinks, so maybe you would have to patch the
bindex.bnd file or something like that...I still find it better
than custom functions that recreate those already provided. 

OK: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
OK: The package MUST successfully compile and build into binary rpms on at least one primary architecture. 
OK: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
OK: Packages must NOT bundle copies of system libraries.
OK: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
OK: A Fedora package must not list a file more than once in the spec file's %files listings. 
OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
OK: Each package must consistently use macros. 
OK: The package must contain code, or permissable content. 
OK: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK: All filenames in rpm packages must be valid UTF-8. 


That's about it for now I guess
Comment 5 Victor G. Vasilyev 2010-07-28 21:22:13 EDT
(In reply to comment #4)
> NEEDSWORK: The package must be named according to the Package Naming Guidelines.
> 
> I am not sure how you got to "Version: 2.2". I couldn't find anything
> in the archive/homepage suggesting that's the last version of the
> package. This seems like the snapshot pre-release of version 0. So it
> should be something like:
> 
> Version: 0
The bundle version 2.2 is established in the bundle descriptor:
http://www.osgi.org/svn/public/trunk/org.osgi.impl.bundle.bindex/bindex.bnd
So, I'll use "Version: 2.2".

> Release: 0.1.svn96%{?dist}
Of course, it is more closer with the guidelines.
But, seems, due to version 2.2 (i.e. it is not pre-release) I need use
Release: 1.svn96%{?dist}

> One way or the other it would be nice to get in touch with upstream 
> and get them to actually release versioned binary release
> (e.g. bindex-%{version}.zip/tar.xx)
I agree. I think, both moment and contents of the release is not clear if an archive of the upstream sources (not only versioned binary release!) is not published.
I'll send a request to authors of the bindex program.
 
> NEEDSWORK: The spec file for the package MUST be legible.
> 
> You define a LOT of macros that are only used once:
>  * svnRev/svnURL
>  * bnd
>  * installJAR
>  * rmFiles/rmFiles_lst
> 
> Please don't do this, it just makes the spec file harder to read
> without adding any benefit. I know it can be tempting to treat spec
> file as a bash script, but think of it more as a "recipe" where you
> just define the ingredients and few hints how to cook it :-) Make it
> as simple as possible.
OK. I've removed most of the macros, but I'd like to consider svnRev as a version: 
https://fedoraproject.org/wiki/Packaging:SourceURL#Using_.25.7Bversion.7D

> Plus one more thing. Instead of creating lnSysJar macro, use
> build-classpath or build-classpath-directory commands.
OK. The build-classpath is used instead.

> I know it doesn't work well with renames when creating symlinks, 
> so maybe you would have to patch ... or something like that...
I do not think that a patch makes the spec more clear, so I've decided to use 
%__ln_s with build-classpath instead.

The next release is prepared for review:
Spec URL: http://victorv.fedorapeople.org/files/bindex.spec
SRPM URL: http://victorv.fedorapeople.org/files/bindex-2.2-1.svn96.fc14.src.rpm

$ rpmlint SRPMS/bindex-2.2-1.svn96.fc14.src.rpm RPMS/noarch/bindex-2.2-1.svn96.fc14.noarch.rpm
bindex.src: W: invalid-url Source0: bindex.r96.svn.tar.gz
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2356481
Comment 6 Stanislav Ochotnicky 2010-07-29 04:04:51 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > NEEDSWORK: The package must be named according to the Package Naming Guidelines.
> > 
> > I am not sure how you got to "Version: 2.2". I couldn't find anything
> > in the archive/homepage suggesting that's the last version of the
> > package. This seems like the snapshot pre-release of version 0. So it
> > should be something like:
> > 
> > Version: 0
> The bundle version 2.2 is established in the bundle descriptor:
> http://www.osgi.org/svn/public/trunk/org.osgi.impl.bundle.bindex/bindex.bnd
> So, I'll use "Version: 2.2".

Sure, missed that one.

> > Release: 0.1.svn96%{?dist}
> Of course, it is more closer with the guidelines.
> But, seems, due to version 2.2 (i.e. it is not pre-release) I need use
> Release: 1.svn96%{?dist}

I guess it's hard to tell because of how upstream does "releases" so I'll leave this up to you. This seems like reasonable versioning in this case

> > One way or the other it would be nice to get in touch with upstream 
> > and get them to actually release versioned binary release
> > (e.g. bindex-%{version}.zip/tar.xx)
> I agree. I think, both moment and contents of the release is not clear if an
> archive of the upstream sources (not only versioned binary release!) is not
> published.
> I'll send a request to authors of the bindex program.

Great.

> > NEEDSWORK: The spec file for the package MUST be legible.
> > 
> > You define a LOT of macros that are only used once:
> >  * svnRev/svnURL
> >  * bnd
> >  * installJAR
> >  * rmFiles/rmFiles_lst
> > 
> > Please don't do this, it just makes the spec file harder to read
> > without adding any benefit. I know it can be tempting to treat spec
> > file as a bash script, but think of it more as a "recipe" where you
> > just define the ingredients and few hints how to cook it :-) Make it
> > as simple as possible.
> OK. I've removed most of the macros, but I'd like to consider svnRev as a
> version: 
> https://fedoraproject.org/wiki/Packaging:SourceURL#Using_.25.7Bversion.7D

Sure, I just wanted to clear-up the spec. Seemed too convoluted to me. Now it's good.

> > Plus one more thing. Instead of creating lnSysJar macro, use
> > build-classpath or build-classpath-directory commands.
> OK. The build-classpath is used instead.
> 
> > I know it doesn't work well with renames when creating symlinks, 
> > so maybe you would have to patch ... or something like that...
> I do not think that a patch makes the spec more clear, so I've decided to use 
> %__ln_s with build-classpath instead.

Agreed, good solution too.

Package is APPROVED
Comment 7 Stanislav Ochotnicky 2010-07-29 04:36:08 EDT
Actually I noticed one more thing:
> java -jar %{_javadir}/aqute-bndlib.jar ...

Another build-classpath before committing the package please.
Comment 8 Victor G. Vasilyev 2010-07-29 10:30:38 EDT
(In reply to comment #7)
> Actually I noticed one more thing:
> > java -jar %{_javadir}/aqute-bndlib.jar ...
> 
> Another build-classpath before committing the package please.    

Thanks for the hint and whole review. I'll do it before committing.
Comment 9 Victor G. Vasilyev 2010-07-29 10:36:13 EDT
New Package CVS Request
=======================
Package Name: bindex
Short Description: Bundle Manifest Header Mapper
Owners: victorv
Branches: 
InitialCC:
Comment 10 Kevin Fenzi 2010-07-30 16:25:36 EDT
GIT done (by process-cvs-requests.py).
Comment 11 Victor G. Vasilyev 2010-07-30 18:08:31 EDT
New Package CVS Request
=======================
Package Name: bindex
Short Description: Bundle Manifest Header Mapper
Owners: victorv
Branches: F-14
InitialCC:
Comment 12 Kevin Fenzi 2010-07-30 19:05:35 EDT
f14 branch added
Comment 13 Victor G. Vasilyev 2010-07-30 20:38:45 EDT
Koji builds:
dist-rawhide
 http://koji.fedoraproject.org/koji/taskinfo?taskID=2362693
dist-f14-updates-candidate
 http://koji.fedoraproject.org/koji/taskinfo?taskID=2363734