Bug 254008 - Review Request: objectweb-asm - Version 3.0 of the ObjectWeb ASM
Summary: Review Request: objectweb-asm - Version 3.0 of the ObjectWeb ASM
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fernando Nasser
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 435601 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-08-23 16:10 UTC by Fernando Nasser
Modified: 2008-05-20 15:57 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-12-04 22:07:30 UTC
Type: ---
Embargoed:
loganjerry: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
upgraded and with tabs converted to spaces (3.68 KB, application/octet-stream)
2007-11-24 06:24 UTC, Fernando Nasser
no flags Details

Description Fernando Nasser 2007-08-23 16:10:55 UTC
Spec URL: http://fnasser.fedorapeople.org/objectweb-asm.spec

SRPM URL: http://fnasser.fedorapeople.org/objectweb-asm-3.0-1jpp.src.rpm

Description: Version 3.0 of the ObjectWeb ASM

ASM is a code manipulation tool to implement adaptable systems.

Comment 1 Fernando Nasser 2007-08-23 16:14:02 UTC
This version is needed by the newer versions of findbugs, which someone wants to
contribute.

P.S.: The old versions of ASM are named 'asm' and 'asm2' respecitively for 1.x
and 2.x versions.  ObjectWeb has requested JPackage.org to add the 'objectweb-'
prefix to the name of the package, so I prefered to do this to call it 'asm3'. 
The APIs have changed by these are widely used and each usptream project seems
to need a specific one.  So we need 1.x, 2.x and 3.x !

Comment 2 Jerry James 2007-11-21 18:40:03 UTC
The someone is me.  As it turns out, recent versions of findbugs use features of
asm3 that were added to CVS after the release of 3.0.  I was hoping to find a
way of hacking the findbugs source to work around this.  However, version 3.1 is
now out and resolves the problem for me nicely.  If you can update your SRPM to
3.1, I'll do the review.

Incidentally, the current spec file mixes spaces and tabs, which makes rpmlint
complain.  Also, the license should be BSD instead of BSD-style.  Thanks!


Comment 3 Fernando Nasser 2007-11-21 19:40:20 UTC
Alright, will do.

Comment 4 Fernando Nasser 2007-11-24 06:24:17 UTC
Created attachment 267961 [details]
upgraded and with tabs converted to spaces

Comment 5 Fernando Nasser 2007-11-24 06:25:29 UTC
http://fnasser.fedorapeople.org/objectweb-asm-3.1-1jpp.src.rpm

Revised and upgraded SRPM

Comment 6 Jerry James 2007-11-27 20:03:21 UTC
I'm getting an HTTP 404 on the SRPM.  I tried navigating from
http://fnasser.fedorapeople.org/ as well, but all I can find is the old version.


Comment 7 Fernando Nasser 2007-11-28 03:53:34 UTC
Sorry, my bad, try this:

http://people.redhat.com/fnasser/objectweb-asm-3.1-1jpp.src.rpm

Comment 8 Jerry James 2007-11-30 20:56:14 UTC
This package fails 3 MUST items.  They are as follows:

[1] LICENSE.txt must be included as a documentation file.
[2] README.txt and LICENSE.txt both must have their end-of-line encodings fixed.
[3] The package xml-commons-apis (or maybe just jaxp) is both a BuildRequires
and a Requires, due to its use in the org.objectweb.asm.xml package.

Here is the rpmlint output for this package:

[jamesjer@localhost ~]$ rpmlint objectweb-asm
objectweb-asm.noarch: W: wrong-file-end-of-line-encoding
/usr/share/doc/objectweb-asm-3.1/README.txt
objectweb-asm.noarch: W: non-standard-group Development/Libraries/Java
objectweb-asm.noarch: W: class-path-in-manifest
/usr/share/java/objectweb-asm/asm-xml-3.1.jar
[jamesjer@localhost ~]$ rpmlint objectweb-asm-javadoc
objectweb-asm-javadoc.noarch: W: non-standard-group Development/Documentation

MUST items:

- rpmlint output: FAIL, see #2 above
- package naming: OK
- spec file name: OK
- packaging guidelines: OK
- licensing guidelines: OK
- license matches: OK
- package contains license file: FAIL, see #1 above
- spec file in American English: OK
- spec file legible: OK
- sources match upstream: OK
- package compiles and builds into binary RPMs: OK
- appropriate ExcludeArch tags: OK
- all build requirements listed: FAIL, see #3 above
- locales: OK
- correct use of ldconfig: OK
- relocatable package: OK
- package owns directories: OK
- no duplicate files in %files: OK
- file permissions: OK
- spec file has %clean section: OK
- use of macros: OK
- package contains code or permissible content: OK
- large documentation files in a separate package: OK
- files in %doc do no affect runtime: OK
- header files in a -devel package: OK
- static libraries in a -static package: OK
- proper handling of pkgconfig files: OK
- handling of .so and .so.version files: OK
- -devel packages require the base package: OK
- no libtool archives: OK
- desktop file for GUI applications: OK
- package does not own files or directories owned by others: OK
- the buildroot is cleaned at the top of %install: OK
- all filenames are UTF-8: OK

SHOULD items:

- query upstream for a missing license file: OK
- description and summary translations, if available: OK
- package builds in mock: FAIL, see #3 above
- package builds on all supported architectures: unable to test
- package functions as described: OK
- sane scriptlets: OK
- subpackages require the base package with a full version: NO

The javadoc subpackage does not do this, but this appears to be common practice.

- placement of pkgconfig files: OK
- require packages instead of files: OK

Also, I note that the find command in the %prep section appears to be
unnecessary.  The upstream distribution contains no jar files.


Comment 9 Fernando Nasser 2007-11-30 23:38:23 UTC
[1] LICENSE.txt must be included as a documentation file.

Nice catch, I wonder why their dist target does not copy it over.  Anyway, I am
adding it as %doc in the next spin.

[2] README.txt and LICENSE.txt both must have their end-of-line encodings fixed.

I inspected both files and they don't have any EOL problems.  Seem to be fine ix
terminated lines.  Why do you think there is a problem?

[3] The package xml-commons-apis (or maybe just jaxp) is both a BuildRequires
and a Requires, due to its use in the org.objectweb.asm.xml package.

I am 100% sure it builds without it, so it is not a build requires.  I did not
see any target doing conditional compilation of classes based on its presence
either.  In any case, 'ant' itself brings jaxp, so it could be assumed (I
removed with nodeps for testing).

I checked the ASM web site http://asm.objectweb.org/ and could not find any
reference about the need of xml-commons for anything.  Yet you say it should be
a run-time dependency.  Is it something optional?  But if it is, unless it is
used by reflection, we should have needed it at build time too.

An optional dependency perhaps?  We don't add Requires for those.

I will wait for your comments on the above before uploading the one with [1]
fixed as I may be missing something.

Thanks again for the reviewing.


Comment 10 Jerry James 2007-12-03 18:15:56 UTC
For [2], did you notice that rpmlint complains about README.txt?  When I open
the files with a hex editor, I see 0x0d 0x0a sequences in both of them.

As for [3], xml-commons-apis is Required by ant as you note, so when you
BuildRequire ant, it gets pulled in.  Then the rpm scripts add everything in
/usr/share/java to your CLASSPATH, so it is magically available.  Try building
without ant; i.e., explicitly invoke javac on all of the source files.  You'll
see that the compile fails with a message about being unable to resolve a bunch
of imports that all start with org.xml.sax.

It is my understanding that every build requirement, even those that are
satisfied transitively, must be listed in the BuildRequires unless they appear
on the exceptions list:

http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions

So even though the BuildRequires of ant pulls it in, it should be listed anyway,
in case some future version of ant stops Requiring it.

The classes in src/org/objectweb/asm/xml call methods provided by
xml-commons-apis / jaxp, so for asm-xml.jar to be useable, xml-commons-apis must
be installed at runtime, hence the Requires.  It isn't using reflection, nor is
it something optional.  That jar will do nothing but throw
ClassNotFoundExceptions if xml-commons-apis is not installed.  The other jars
don't need it, so this is possibly a candidate for a subpackage.

See the reference to JAXP here:
http://asm.objectweb.org/current/doc/javadoc/user/org/objectweb/asm/xml/package-summary.html

Comment 12 Jerry James 2007-12-03 22:28:39 UTC
Looks great.  I'll just note that the BuildRequires on dos2unix isn't necessary.
 You could do this:

sed -i 's/\r//' README.txt LICENSE.txt

since sed is in the default build root, but that's a minor point.  I'll mark
this as passing review (in 2 steps, since I didn't remember to mark this as
under review, yikes!).  Thanks for all your work!


Comment 13 Fernando Nasser 2007-12-04 19:35:55 UTC
You're welcome.  And thanks for the review, suggestions, clarifications etc.

Talking about suggestions, I will replace dos2unix with sed as per your
suggestion in the .1 build.

Requesting a cvs branch now.


Comment 14 Kevin Fenzi 2007-12-04 19:52:07 UTC
Can you add a CVS template here to indicate what branches you want, etc?

See: http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Reset the fedora-cvs flag when you are ready. 

Comment 15 Fernando Nasser 2007-12-04 20:08:07 UTC
Package Name: objectweb-asm
Short Description: A code manipulation tool to implement adaptable systems
Owners: fnasser
Branches: devel
InitialCC: fnasser
Cvsextras Commits: no

Comment 16 Kevin Fenzi 2007-12-04 20:12:51 UTC
Thanks. cvs done.

Comment 17 Fernando Nasser 2007-12-04 21:26:37 UTC
274354 build (dist-f9, devel:objectweb-asm-3_1-2jpp_1_fc9) completed successfully


Comment 18 Mamoru TASAKA 2008-03-06 18:35:10 UTC
*** Bug 435601 has been marked as a duplicate of this bug. ***

Comment 19 Conrad Meyer 2008-05-09 21:00:08 UTC
Package Change Request
======================
Package Name: objectweb-asm
New Branches: F-8

Comment 20 Kevin Fenzi 2008-05-10 04:04:05 UTC
Conrad: Does Fernando want to maintain this F-8 branch as well?

Fernando: Can you ack the request for a F-8 branch?

Comment 21 Conrad Meyer 2008-05-10 21:22:21 UTC
If not I can co-maintain. I don't expect it to be difficult to get working on 
F-8.

Comment 22 Fernando Nasser 2008-05-20 14:13:07 UTC
I am OK with it and maintain it in an additional branch should not be a problem.
 But...

Are you sure I can do that?  This package was submitted and approved for F-9, I
am not certain I can add it to a previous release by myself.

In the meanwhile I will check how one goes about creating a branch for a
previous release.  This is a new case for me.

Regards.



Comment 23 Fernando Nasser 2008-05-20 14:41:21 UTC
OK, waiting for F-8 bramch creation to try and build.

Comment 24 Kevin Fenzi 2008-05-20 15:57:56 UTC
cvs done.


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