Bug 516343

Super fast Spec-only comments. I promise to look at this more closely in the next 24 hrs:

*Why are the unit tests disabled?
*Please consider including %{name}.jar symlinks to %{name}-%{version}.jar

(In reply to comment #1)
> Super fast Spec-only comments. I promise to look at this more closely in the
> next 24 hrs:
> 
> *Why are the unit tests disabled?

Because if I enable it I get the following:

[...]
test:

BUILD FAILED
/home/andrea/devel/prg/metadata-extractor/build.xml:48: Problem:
failed to create task or type junit
Cause: the class
org.apache.tools.ant.taskdefs.optional.junit.JUnitTask was not found.
       This looks like one of Ant's optional components.
Action: Check that the appropriate optional JAR exists in
       -/usr/share/ant/lib
       -/home/andrea/.ant/lib
       -a directory added on the command line with the -lib argument

Do not panic, this is a common problem.
The commonest cause is a missing JAR.

This is not a bug; it is a configuration problem


BTW, this is what Debian does too. I'm not a Java boy and I do not know how to work this out.

> *Please consider including %{name}.jar symlinks to %{name}-%{version}.jar  

I'm inexperienced, can you please tell me what are the pro's of doing this? Is this something mandatory? Or is this something suggested but strongly advised? I haven't seen anything about this in the guidelines. 

Thanks for your comments!

(In reply to comment #2)
> (In reply to comment #1)
> > Super fast Spec-only comments. I promise to look at this more closely in the
> > next 24 hrs:
> > 
> > *Why are the unit tests disabled?
> 
> Because if I enable it I get the following:
> 
> [...]
> test:
> 
> BUILD FAILED
> /home/andrea/devel/prg/metadata-extractor/build.xml:48: Problem:
> failed to create task or type junit
> Cause: the class
> org.apache.tools.ant.taskdefs.optional.junit.JUnitTask was not found.
>        This looks like one of Ant's optional components.
> Action: Check that the appropriate optional JAR exists in
>        -/usr/share/ant/lib
>        -/home/andrea/.ant/lib
>        -a directory added on the command line with the -lib argument
> 
> Do not panic, this is a common problem.
> The commonest cause is a missing JAR.
> 
> This is not a bug; it is a configuration problem
> 
> 
> BTW, this is what Debian does too. I'm not a Java boy and I do not know how to
> work this out.
> 

Did you try to BuildRequire: ant-junit ? 
It seems you lack the jar providing junit functions to ant

>can you please tell me what are the pro's of doing this? Is
>this something mandatory?

It means that anyone trying to link to your jar can find it quite easily with build-classpath, and they wont have to update their package with the new jar version every time you update your version of this jar.

(In reply to comment #3)
> Did you try to BuildRequire: ant-junit ? 
> It seems you lack the jar providing junit functions to ant  

Yes, that's what I get:

[...]
test:
    [junit] WARNING: multiple versions of ant detected in path for junit 
    [junit]          jar:file:/usr/share/java/ant-1.7.1.jar!/org/apache/tools/ant/Project.class
    [junit]      and jar:file:/usr/share/java/ant.jar!/org/apache/tools/ant/Project.class
    [junit] Running com.drew.metadata.test.AllTests
    [junit] Tests run: 79, Failures: 1, Errors: 0, Time elapsed: 0,269 sec

BUILD FAILED
/home/andrea/devel/prg/metadata-extractor/build.xml:48: Test com.drew.metadata.test.AllTests failed

Total time: 1 second

(In reply to comment #4)
> >can you please tell me what are the pro's of doing this? Is
> >this something mandatory?
> 
> It means that anyone trying to link to your jar can find it quite easily with
> build-classpath, and they wont have to update their package with the new jar
> version every time you update your version of this jar.  

I'll wait for more comments from your part to publish a new release. I'll include this improvement too.

Thanks!

(In reply to comment #5)
> (In reply to comment #3)
> > Did you try to BuildRequire: ant-junit ? 
> > It seems you lack the jar providing junit functions to ant  
> 
> Yes, that's what I get:
> 
> [...]
> test:
>     [junit] WARNING: multiple versions of ant detected in path for junit 
>     [junit]         
> jar:file:/usr/share/java/ant-1.7.1.jar!/org/apache/tools/ant/Project.class
>     [junit]      and
> jar:file:/usr/share/java/ant.jar!/org/apache/tools/ant/Project.class
>     [junit] Running com.drew.metadata.test.AllTests
>     [junit] Tests run: 79, Failures: 1, Errors: 0, Time elapsed: 0,269 sec
> 
> BUILD FAILED
> /home/andrea/devel/prg/metadata-extractor/build.xml:48: Test
> com.drew.metadata.test.AllTests failed
> 
> Total time: 1 second
> 

You can ignore the warning (/usr/share/java/ant/ant-junit.jar is a symlink to /usr/share/java/ant/ant-junit-1.7.1.jar); however the package fails in one of the tests, that's probably why junit build was excluded from debian folks too. 

Which locale do you run the jvm into? The code calls a decimal number formatter which returns "x,xx" (or "x.xx" depending on locale) instead of the expected  "x.xx" (should work in us locales, it's bad written code however because it makes a comparison between a value processed depending on locale with a static string).

You can patch build.xml in the top directory:
----
48a49
>             <jvmarg value="-Duser.language=en -Duser.country=US"/>
----

to make junit tests complete successfully, and send the patch upstream.

(In reply to comment #6)

> Which locale do you run the jvm into? The code calls a decimal number formatter
> which returns "x,xx" (or "x.xx" depending on locale) instead of the expected 
> "x.xx" (should work in us locales, it's bad written code however because it
> makes a comparison between a value processed depending on locale with a static
> string).  

I'm using an Italian locale.

(In reply to comment #7)
> You can patch build.xml in the top directory:
> ----
> 48a49
> >             <jvmarg value="-Duser.language=en -Duser.country=US"/>
> ----
> 
> to make junit tests complete successfully, and send the patch upstream.  

Thanks for the patch. I can successfully compile metadata-extractor outside
rpmbuild. In rpmbuild I still have an error in the junit tests, although
different:

[...]
 Testcase: testUserCommentDescription_AsciiHeaderExtendedAsciiEncoding took
0.002
 sec
        FAILED
expected:<...extended characters [??? ???]> but was:<...extended characters
[???
 ???]>
junit.framework.ComparisonFailure: expected:<...extended characters [??? ???]>
b
ut was:<...extended characters [??? ???]>
        at
com.drew.metadata.exif.test.ExifDescriptorTest.testUserCommentDescrip
tion_AsciiHeaderExtendedAsciiEncoding(Unknown Source)

This is quite strange. I diff'ed the sources and they seem the same. I need to
investigate this further.

Can you provide location for your latest specfile and SRPM?

(In reply to comment #9)
> Can you provide location for your latest specfile and SRPM?  

Sure. Here they are:
Spec URL: http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor.spec
SRPM URL:
http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor-2.3.1-2.fc10.src.rpm

Changelog:
* Mon Aug 10 2009 Andrea Musuruane <musuruan> 2.3.1-2
- Created JAR alias
- Added a patch to make junit tests complete successfully

Mmh, rpmmacro exporting LANG seems to take precedence against my patch, so please forget it; what you need is:
LANG=en_US.UTF-8 ant dist-binaries javadoc
in you specfile, but i cant say whether it's following packaging guidelines.

Your uploaded spec and SRPM do not appear to match. Please do not modify the uploaded spec without posting a matching SRPM. 


Please  repost an updated SRPM with the changes suggested by Guido (Comment 11). Also I don't think you need Requires: jpackage-utils in the javadoc package, as it depends on the main, which also requires jpackage-utils.

> Read upsteam homepage for license information
Upstream says "The code is protected by copyright, though only to avoid people selling it unmodified or copyrighting it themselves." Which is not quite PD, but close. Someone else may need to confirm the licencing here. All the source headers appear to be in place.




$ diff -uh  metadata-extractor.spec metadata-extractor.spec-orig 
--- metadata-extractor.spec	2009-08-11 04:58:42.000000000 +1000
+++ metadata-extractor.spec-orig	2009-08-22 21:32:37.000000000 +1000
@@ -11,15 +11,13 @@
 # Patch provided by Gabriel Ebner to remove all references to the 
 # com.sun classes. Package builds with a free java implementation now.
 Patch0:         %{name}-2.3.1-nosun.patch
-# Patch provided by Guido Grazioli to make junit tests complete successfully
-Patch1:         %{name}-2.3.1-buildxml.patch
 BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 BuildArch:      noarch
 BuildRequires:  jpackage-utils
 BuildRequires:  java-devel
 BuildRequires:  ant
-BuildRequires:  ant-junit
+BuildRequires:  junit
 Requires:       jpackage-utils
 Requires:       java
 
@@ -44,7 +42,6 @@
 %prep
 %setup -q -c
 %patch0 -p1
-%patch1 -p0
 
 # Remove pre-built JAR and class files
 find -name '*.jar' -exec rm -f '{}' \;
@@ -56,6 +53,9 @@
 # Fix end-of-line encoding
 sed -i 's/\r//' ChangeLog.txt
 
+# Do not execute unit test
+sed -i 's:clean, compile, test:clean, compile:' build.xml
+
 
 %build
 ant dist-binaries javadoc
@@ -96,9 +96,8 @@
 
 
 %changelog
-* Mon Aug 10 2009 Andrea Musuruane <musuruan> 2.3.1-2
+* Sun Aug 09 2009 Andrea Musuruane <musuruan> 2.3.1-2
 - Created JAR alias
-- Added a patch to make junit tests complete successfully
 
 * Sun Aug 02 2009 Andrea Musuruane <musuruan> 2.3.1-1
 - First release

(In reply to comment #12)
> Please  repost an updated SRPM with the changes suggested by Guido (Comment
> 11). Also I don't think you need Requires: jpackage-utils in the javadoc
> package, as it depends on the main, which also requires jpackage-utils.

Spec URL: http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor.spec
SRPM URL:
http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor-2.3.1-3.fc10.src.rpm

Changelog:
* Sat Aug 22 2009 Andrea Musuruane <musuruan> 2.3.1-3
- Used a different workaround as suggested by Guido Grazioli to make 
  junit tests complete successfully


Please note that I didn't removed the "Requires: jpackage-utils" because this exactly what is used in the ant template you can find in the Java Packaging Guidelines:
http://fedoraproject.org/wiki/Packaging:Java

> > Read upsteam homepage for license information
> Upstream says "The code is protected by copyright, though only to avoid people
> selling it unmodified or copyrighting it themselves." Which is not quite PD,
> but close. Someone else may need to confirm the licencing here. All the source
> headers appear to be in place.

Upstream, in the "About this library" paragraph in its homepage, also states this:
"This metadata library is available with Java source code for usage in the public domain." 

Moreover, as you noted, all source files contain the following text:

 * This class is public domain software - that is, you can do whatever you want
 * with it, and include it software that is licensed under the GNU or the
 * BSD license, or whatever other licence you choose, including proprietary
 * closed source licenses.  Similarly, I release this Java version under the
 * same license, though I do ask that you leave this header in tact.
 *
 * If you make modifications to this code that you think would benefit the
 * wider community, please send me a copy and I'll post it on my site.
 *
 * If you make use of this code, I'd appreciate hearing about it.
 *   drew.noakes
 * Latest version of this software kept at
 *   http://drewnoakes.com/

Or the following text:

 * This is public domain software - that is, you can do whatever you want
 * with it, and include it software that is licensed under the GNU or the
 * BSD license, or whatever other licence you choose, including proprietary
 * closed source licenses.  I do ask that you leave this header in tact.
 *
 * If you make modifications to this code that you think would benefit the
 * wider community, please send me a copy and I'll post it on my site.
 *
 * If you make use of this code, I'd appreciate hearing about it.
 *   drew
 * Latest version of this software kept at
 *   http://drewnoakes.com/

Does not build in koji:

F10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1626400
F11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1626402

(In reply to comment #14)
> Does not build in koji:
> 
> F10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1626400
> F11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1626402  

I have really enough of junit test failings. Can we just disable them as I did when I submitted the package? After all, there should be a reason why Debian disabled them too.

For the record it fails this test:

[..]
Testcase: testUserCommentDescription_AsciiHeaderExtendedAsciiEncoding took 0 sec
        FAILED
expected:<... extended characters[  ]> but was:<... extended characters[]>
junit.framework.ComparisonFailure: expected:<... extended characters[  ]> but wa
s:<... extended characters[]>
   at com.drew.metadata.exif.test.ExifDescriptorTest.testUserCommentDescription_
AsciiHeaderExtendedAsciiEncoding(ExifDescriptorTest.java:79)

I don't know enough about the Exif specification and the current code to understand why this test fails. If you can explain why this test will not ever affect downstream users, then disable it with a comment, otherwise please identify why the code does not work, then patch it and send patches upstream.

I know it kinda sucks when you want to get a package done, but packagers stand between bugs and users!

(In reply to comment #16)
> I don't know enough about the Exif specification and the current code to
> understand why this test fails. If you can explain why this test will not ever
> affect downstream users, then disable it with a comment, otherwise please
> identify why the code does not work, then patch it and send patches upstream.
>
> I know it kinda sucks when you want to get a package done, but packagers stand
> between bugs and users!  

Junit test fails because they assume:
1. You are using an en-US locale
2. You are using an UTF-8 locale

For some reason, the workaround Guido found does not work with mock (and therefore koji). It still fails because of 2.

Unluckily I'm not a Java programmer and I cannot patch the code myself. External help is always welcome :)

I can look at this, but not for another week. I suggest emailing upstream in the mean time for assistance.

(In reply to comment #18)
> I can look at this, but not for another week. I suggest emailing upstream in
> the mean time for assistance.  

I mailed upstream on Aug 23rd but I haven't received yet an answer.

Bye,

Andrea.

I agree with Andrea, probably best deal is to not run unit tests as debian folks have done; the failures in the tests are due to bad coding in the tests themselves, setting up a workaround to let it compile would be silly while patching the code would require a pretty big patch.

>the failures in the tests are due to bad coding in the tests themselves,

I still haven't had time to look at this. As long as you are convinced that it is a test failure, and not a failure in the main code, I am happy for it to be disabled with a comment.

http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor.spec
http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor-2.3.1-4.fc10.src.rpm

Changelog:
* Sat Sep 12 2009 Andrea Musuruane <musuruan> 2.3.1-4
- Disabled junit tests because of bad coding in the tests themselves

http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor.spec
http://www.webalice.it/musuruan/RPMS/reviews/metadata-extractor-2.3.1-5.fc10.src.rpm

Changelog:
* Sun Oct 18 2009 Andrea Musuruane <musuruan> 2.3.1-5
- Fixed javadoc package requires
- Created javadoc directory alias

OK - rpmlint output
3 packages and 1 specfiles checked; 0 errors, 0 warnings.
OK - The package must be named according to the Package Naming Guidelines.
OK - The spec file name must match the base package %{name}
OK - The package must meet the Packaging Guidelines
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 (no license file)
OK - The package must be licensed with a Fedora approved license and
meet the Licensing Guidelines (license is Public Domain)
NA - Every binary RPM package which stores shared library files must
call ldconfig in %post and %postun 
OK - The package MUST successfully compile and build
http://koji.fedoraproject.org/koji/taskinfo?taskID=1798401
OK - The spec file MUST be written in American English.
OK - The spec file for the package MUST be legible.
OK - The sources used to build the package MUST match the upstream source, as
provided in the spec URL. 
b2f8d9ade3cc8008ed41ad62c1e80bc2  metadata-extractor-2.3.1-src.jar
NA - The spec file MUST handle locales properly (no translations)
NA - package not relocatable
OK - A package MUST own all directories that it creates
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
OK - Each package MUST have a %clean section
OK - Each package MUST consistently use macros
OK - The package MUST contain code, or permissable content 
OK - Large documentation files MUST go in a -doc subpackage (javadocs in -javadoc subpackage)
OK - IF a package includes something as %doc, it MUST not affect the runtime of
the application
NA - Header files MUST be in a -devel package (java package)
NA - Static libraries MUST be in a -static package (no static package)
NA - Packages containing pkgconfig(.pc) files MUST 'Requires: pkgconfig' 
OK - Packages MUST NOT contain any .la libtool archives
NA - Packages containing GUI applications MUST include a .desktop file 
OK - No file conflicts with other packages and no general names.
OK - At the beginning of %install, each package MUST run rm -rf %{buildroot}
OK - All filenames in rpm packages MUST be valid UTF-8
OK - The package does not yet exist in Fedora. The Review Request is not a
duplicate.
OK - %{?dist} tag is used in release
OK - Jar file naming (versioned jar file and unversioned symlink)
OK - BuildRequires and Requires (Java and Ant Rs and BRs used consistently with guidelines)
OK - Pre-built JAR files / Other bundled software (not present)
NEEDSWORK - Java Directory structure: all jars in %{_javadir} and all javadocs in %{_javadocdir}
OK - Javadoc scriptlets not present

NOTES:
For -javadoc subpackage, you dont want a versioned dir and an unversioned link pointing to that. (See https://fedoraproject.org/wiki/Packaging/Java#ant_2 ).

With that fixed, i will approve the package.

(In reply to comment #24)
> NOTES:
> For -javadoc subpackage, you dont want a versioned dir and an unversioned link
> pointing to that. (See https://fedoraproject.org/wiki/Packaging/Java#ant_2 ).
> 
> With that fixed, i will approve the package.  

Are you sure about this? I ask because during the review of gettext-commons (#515136) the reviewer required me to do so. I'm a newbie at Java packaging and I'm really confused now.

(In reply to comment #25)
> (In reply to comment #24)
> > NOTES:
> > For -javadoc subpackage, you dont want a versioned dir and an unversioned link
> > pointing to that. (See https://fedoraproject.org/wiki/Packaging/Java#ant_2 ).
> > 
> > With that fixed, i will approve the package.  
> 
> Are you sure about this? I ask because during the review of gettext-commons
> (#515136) the reviewer required me to do so. I'm a newbie at Java packaging and
> I'm really confused now.  

I could not find any reference about that convention in the guidelines, not even in the old jpackage guidelines which only ask (it's "shall", not "must") to symlink unversioned jars to versioned ones. Maybe there were talks about it in the -packaging list, however I can confirm that most java packages installed in my system adhere to that convention; the ant and maven specfile templates found in the wiki should be updated though.
That said, package is APPROVED, as it is.

New Package CVS Request
=======================
Package Name: metadata-extractor
Short Description: JPEG metadata extraction framework
Owners: musuruan
Branches: F-10 F-11 F-12
InitialCC:

cvs done.

metadata-extractor-2.3.1-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/metadata-extractor-2.3.1-5.fc12

metadata-extractor-2.3.1-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/metadata-extractor-2.3.1-5.fc11

metadata-extractor-2.3.1-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/metadata-extractor-2.3.1-5.fc10

Built. Closing.

metadata-extractor-2.3.1-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

metadata-extractor-2.3.1-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

metadata-extractor-2.3.1-5.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.