Bug 985

I'll take first look; it's my first java review (go easy) ;-)

  Summary:
I think this could be expanded a bit: perhaps:
DVB mpg stream analysis, demultiplex and repair tool

  Patch:
should that be %{name}...

  BuildRequires / Requires:
I'd prefer to see at least the minimal build/requires on separate lines. This makes it easier to see that the package guidelines are being met, and when adjustments are needed, the diff from a previous version becomes clearer.

  %global
not sure what that is or does. Can you please explain ?

  %description
DVB is also used in other countries, eg Australia, so I'm not sure mentioning only Europe is important. Eg maybe "Europe and many countries...
It would also be good to spell out the Digital Video Broadcasting before use of the acronym.
Also, I think we aren't really looking at the transmissions themselves, but rather mpeg transport streams that DVB tuner device has decoded.

  %build
Is there any standard ways of doing stuff in:
http://fedoraproject.org/wiki/Packaging:Java#build-classpath
that would make sense to use with this package ?

  install & clean
rm -rf %buildroot
Is that supposed to be %{ } for most of the %xyz items.

It would be clearer to create the install script as a file that goes into cvs, rather than create it during the build.


  %files
why does %{bindir}/projectx need specific attribs set ?


  %changelog
Please fix the typo in the date and preparation

  build process:
I noticed two things while failing to build the package:
1. yum-builddep on .src:
Installing:
 jakarta-commons-net       noarch      2.0-2.fc12             fedora      187 k
 jakarta-oro               i686        2.0.8-6.3.fc12         fedora      170 k
 java-1.5.0-gcj-devel      i686        1.5.0.0-29.fc12        fedora       45 k
 recode                    i686        3.6-28.fc12            fedora      636 k
Installing for dependencies:
 ecj                       i686        1:3.4.2-6.fc12         fedora      2.4 M
 gcc-java                  i686        4.4.2-7.fc12           fedora      3.3 M
 libgcj-devel              i686        4.4.2-7.fc12           fedora      1.6 M
 libgcj-src                i686        4.4.2-7.fc12           fedora       12 M

Transaction Summary
================================================================================
Install       8 Package(s)
Upgrade       0 Package(s)

Total download size: 21 M
Is this ok [y/N]: y
Downloading Packages:
Setting up and reading Presto delta metadata
Processing delta metadata
Package(s) data still to download: 21 M
(1/8): ecj-3.4.2-6.fc12.i686.rpm                         | 2.4 MB     00:05     
(2/8): gcc-java-4.4.2-7.fc12.i686.rpm                    | 3.3 MB     00:03     
(3/8): jakarta-commons-net-2.0-2.fc12.noarch.rpm         | 187 kB     00:00     
(4/8): jakarta-oro-2.0.8-6.3.fc12.i686.rpm               | 170 kB     00:00     
(5/8): java-1.5.0-gcj-devel-1.5.0.0-29.fc12.i686.rpm     |  45 kB     00:00     
(6/8): libgcj-devel-4.4.2-7.fc12.i686.rpm                | 1.6 MB     00:01     
(7/8): libgcj-src-4.4.2-7.fc12.i686.rpm                  |  12 MB     00:19     
(8/8): recode-3.6-28.fc12.i686.rpm                       | 636 kB     00:00     
--------------------------------------------------------------------------------
Total                                           526 kB/s |  21 MB     00:39     
Running rpm_check_debug
Running Transaction Test
Finished Transaction Test
Transaction Test Succeeded
Running Transaction
  Installing     : 1:ecj-3.4.2-6.fc12.i686                                  1/8 
  Installing     : jakarta-oro-2.0.8-6.3.fc12.i686                          2/8 
  Installing     : recode-3.6-28.fc12.i686                                  3/8 
  Installing     : libgcj-devel-4.4.2-7.fc12.i686                           4/8 
  Installing     : libgcj-src-4.4.2-7.fc12.i686                             5/8 
  Installing     : jakarta-commons-net-2.0-2.fc12.noarch                    6/8 
  Installing     : gcc-java-4.4.2-7.fc12.i686                               7/8 
  Installing     : java-1.5.0-gcj-devel-1.5.0.0-29.fc12.i686                8/8 
failed to read link /usr/bin/javac: No such file or directory
failed to read link /usr/lib/jvm/java-gcj: No such file or directory
failed to read link /usr/lib/jvm/java-1.5.0: No such file or directory

Installed:
  jakarta-commons-net.noarch 0:2.0-2.fc12     jakarta-oro.i686 0:2.0.8-6.3.fc12
  java-1.5.0-gcj-devel.i686 0:1.5.0.0-29.fc12 recode.i686 0:3.6-28.fc12        

Dependency Installed:
  ecj.i686 1:3.4.2-6.fc12                 gcc-java.i686 0:4.4.2-7.fc12         
  libgcj-devel.i686 0:4.4.2-7.fc12        libgcj-src.i686 0:4.4.2-7.fc12       

Complete!
----
not sure if the failed to read link stuff is important.

2. rpmbuild -ba  .spec:
+ cd ProjectX_Source_0.90.4
+ /bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ echo 'Patch (ProjectX.sysjava.patch):'
Patch (ProjectX.sysjava.patch):
+ /bin/cat /home/davidt/rpmbuild/SOURCES/ProjectX.sysjava.patch
+ /usr/bin/patch -s -p0 --fuzz=0
+ exit 0
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.VKoNfM
+ umask 022
+ cd /home/davidt/rpmbuild/BUILD
+ cd ProjectX_Source_0.90.4
+ LANG=C
+ export LANG
+ unset DISPLAY
+ sed -i '/Class-Path/I d' MANIFEST.MF
+ sh build.sh
build.sh: line 13: javac: command not found
build.sh: line 15: jar: command not found
error: Bad exit status from /var/tmp/rpm-tmp.VKoNfM (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.VKoNfM (%build)
=====
So the question is:
Do I need to configure alternatives to set a preferred javac jar ?

And if so, how would this work on the build system, where we don't have the ability to manually set such things ?

Is there anything in the mandriva spec that applies to fedora ?
http://rpm.pbone.net/index.php3/stat/26/dist/49/size/451062/name/projectx-0.90.4.00-2mdv2007.0.src.rpm
, it is pretty old though.

They have included a desktop file, so I'm guessing it's a gui app and hence needs a destop file in Fedora / RPM Fusion.

It seems that you are using lib supplied with the sources and not system ones. This is a stopper. Please read Fedora Packaging guidelines:
https://fedoraproject.org/wiki/Packaging:Java

Moreover, this is a GUI application. Therefore you must install a dekstop file:
https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

Also please consider adding the GCJ AOT bits to the package, as requested in java packaging guidelines:
   http://fedoraproject.org/wiki/Packaging/GCJGuidelines

Java SIG says we still need them:
   https://www.redhat.com/archives/fedora-devel-java-list/2009-November/msg00026.html

Thanks for all the feedback!

(In reply to comment #1)

I'll update the description and formatting of BuildRequires according to your suggestions, and fix the typos.

>   %global
> not sure what that is or does. Can you please explain ?

%global is much like %define, and is the form the packaging guidelines prescribe: http://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

> Is there any standard ways of doing stuff in:
> http://fedoraproject.org/wiki/Packaging:Java#build-classpath

I'm patching the build script already, so I could probably patch it in a different way to use build-classpath instead.

> rm -rf %buildroot
> Is that supposed to be %{ } for most of the %xyz items.

Are the curly braces mandatory?  I thought that was just different styles, where both were acceptable.  Like choosing between %buildroot versus $RPM_BUILD_ROOT.  (Or %{buildroot} versus ${RPM_BUILD_ROOT}. :-)

> It would be clearer to create the install script as a file that goes into cvs,
> rather than create it during the build.

That I don't understand.  The build script is part of the source zip file.  How do you mean "create it during the build"?

> why does %{bindir}/projectx need specific attribs set ?

Since I just create it in the %install section, it does not get executable bits on by default.  I could add them with a chmod command, but I thought I could just as well just specify them.

>   Installing     : java-1.5.0-gcj-devel-1.5.0.0-29.fc12.i686                8/8 
> failed to read link /usr/bin/javac: No such file or directory
> failed to read link /usr/lib/jvm/java-gcj: No such file or directory
> failed to read link /usr/lib/jvm/java-1.5.0: No such file or directory

That looks like a bug in the java-1.5.0-gcj-devel package.  Possibly triggered by some kind of inconsistent state when you started.  But I don't know why that would happen.  It shouldn't.


(In reply to comment #2)
> Is there anything in the mandriva spec that applies to fedora ?

I'll take a look and compare.

> , it is pretty old though.

So is ProjectX. :-)  It's not very actively developed.  But still quite useful.

> They have included a desktop file, so I'm guessing it's a gui app and hence
> needs a destop file in Fedora / RPM Fusion.

Ah, right, I forgot about that!  I've only been using it as a command line tool myself, so I didn't think about it.  But it can indeed run in graphical mode too, so I'll add that.


(In reply to comment #3)
> It seems that you are using lib supplied with the sources and not system ones.

Do I?  The sources does indeed come with precompiled jar files.  But I don't think I'm using them.  Avoiding it is the reason I patch the build script.  (Though I could probably patch in a better way, as mentioned above.)  Do I miss something? 


(In reply to comment #4)
> Also please consider adding the GCJ AOT bits to the package, as requested in
> java packaging guidelines:

I see now I misunderstood the section on GCJ in the Java guidelines.  It just says "Please refer to Packaging/GCJGuidelines for GCJ-specific guidelines".  I interpreted that as "Refer to Packaging/GCJGuidelines if you use GCJ".  And until now I've been using openJDK, so I thought that didn't apply to me.  I didn't realize GCJ was compulsory.

Again, thanks for all the feedback!  I'll be back with an improved version in a little while.

Hi Göran, thanks for submitting ProjectX it is a great tool...

I wondered have you considered using a CVS checkout of the ProjectX source for this RPM instead of the latest official release?

I've recently used ProjectX on Fedora (11) and found that the latest release version (0.90.4.00 - over 3 years old!) on which you're basing this RPM has some fairly major shortcomings. In the end I had to build my own version from the CVS source in order to successfully demux some .ts files from a Humax box: the release version would just error and fail but the problem was fixed many years ago in CVS. 

The release version also has a fairly horrible UI compared to the latest CVS checkout and is generally just a bit crappy/buggy in comparison.

Just seems that as you're going to the trouble to bring this to Fedora it would be worthwhile considering using a more recent/useful version?

Best
Chris

(In reply to comment #5)
> Do I?  The sources does indeed come with precompiled jar files.  But I don't
> think I'm using them.  Avoiding it is the reason I patch the build script. 
> (Though I could probably patch in a better way, as mentioned above.)  Do I 
> miss something? 
> 

Possibly. Please remove the precompiled binaries in %prep, as it is asked in the guidelines:

http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries

Thanks for all the comments.  Now I have an updated version of ProjectX.  I believe I've taken all comments into account.  Two things to note:

- I've based this version on a CVS snapshot from December 1 this year.

- I couldn't find any recommendation that %{...} was preferable to %..., so I kept the latter form.  (I personally find it clearer without braces when they are not needed.)

Current URL:s are:

Spec: ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
Source: ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-3.20091201cvs.src.rpm

Did I miss some comment?  Does anyone see more things that could be improved?

(In reply to comment #8)
> - I couldn't find any recommendation that %{...} was preferable to %..., so I
> kept the latter form.
(With my FPC hat on) There is no such recommendation.

> (I personally find it clearer without braces when they
> are not needed.)
The role of %{..} is comparable to "quotes", "escapes" etc. in other languagues.
Whether to use them is largely a matter of personal preference - I for one prefer the %{..} form.

One recomendation, however: Be consistent within one *.spec, i.e. exclusively either use %{xxx} or %xxx. Mixing both forms unnecessarily complicates  sed'ing, grep'ing, searching etc. of *.specs.

There was a flurry of comments in the beginning, and then nothing happened.  I'm supposed to wait until someone has time to continue this review, right?

I believe so, but because of the delay I just wanted to check this is not waiting for me to do something.

(In reply to comment #10)
> There was a flurry of comments in the beginning, and then nothing happened. 
> I'm supposed to wait until someone has time to continue this review, right?
Correct, although you can appeal for reviewers or request review swaps on the rpmfusion-devel list.

> I believe so, but because of the delay I just wanted to check this is not
> waiting for me to do something.
I forgot all about this one, sorry about that. I was really keen to get this app into rpmfusion, but have little java build experience to enable me to say much more about this package than I already noted.

That said:
1. Every time you make a published change to the spec, you need to bump the release number, for eg. the original and current spec have (almost) identical version-release. The fact that you missed doing that suggests you might need to re-read the packaging guidelines:
http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Release
I think the snapshot falls into the post release category.

This is important to help others track the package changes, and to ensure upgrade-ability with the standard tools (rpm/yum/packagekit).
eg this should be at least -4.... so that it will replace -3 the last version.

2. The changelog info was not updated, other than overwriting the date of an existing item, which doesn't make sense. See:
https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
ps: you might light to try using a visual diff tool like meld to review your changes, so that these can be placed in the changelog. It's a very different spec (over 80% larger). An item should be added (not for every line change) to describe the essence of each change:
- update to snapshot from cvs 2009-xxxx ?
- something about gcj ??

3. description: it's non-standard to double-space sentences.

4. Source: should at least include a correct upstream path. I think it would be useful to place a comment:
- showing the release download link.
- the fact that you are using a snapshot, and why (eg last release 2006, fixes only in snapshot)
- how the snapshot is generated, and preferably a script to generate the snapshot, so that others can easily verify the download.

5. It can ease reading of the spec to separate the main sections with two-new lines. At least this should be consistent through the spec.

6. %buildroot%_datadir : while Hans mentioned the reasoning behind using %{_x}, it would seem to make the spec clearer by using them eg in:
%buildroot%{_datadir}
, and is also widely used in fedora.

That's all I have for the moment. I'm happy to help further once you have a new spec to review.

> Every time you make a published change to the spec, you need to bump the 
> release number

Well I did bump from -3 to -3.20091201cvs.  That's later to rpm/yum.  Maybe it would have been more obvious if I had bumped the "3" to "4" too, though.  I'll do so in the future.

> The changelog info was not updated

Um, yes, that was sloppy.  I don't have the -3 version around any more, but I've tried to retrofit now.

> It's non-standard to double-space sentences

I once was taught to do it, and have kept the habit.  But I know it's debated, and I can switch to single-spacing if you prefer.

> Should at least include a correct upstream path

I didn't put any since there isn't any upstream path to the tar archive.  But I've added a comment on how the tar archive can be (re)made using a cvs and a tar command.  It includes the exact path to the CVS pserver.  Is this a good way to document where it comes from?

> It can ease reading of the spec to separate the main sections with two-new
> lines. At least this should be consistent through the spec.

So you like double spacing in SOME cases. :-)

There wasn't double line spacing anywhere, so in that sense it was consistent.  But I've added extra lines in some places.  (I guess it could be debated what constitutes a "main section".  If you think more or less is needed, let me know.)


> %buildroot%_datadir : while Hans mentioned the reasoning behind using %{_x},
> it would seem to make the spec clearer by using them eg in:
> %buildroot%{_datadir}

Hans?

In comment 9 the only real rule was to be consistent, and your suggestion would be contrary to that.

In the end, what is clear is of course a matter of taste.  To me, unneeded braces clutters the spec and makes it LESS clear.  If you really insist I could change it, but I WOULD like to keep it the way it is.

Updated version available at
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-4.20100801cvs.fc14.src.rpm

Created attachment 454 [details]
old ver 3 spec

(In reply to comment #12)
> > Every time you make a published change to the spec, you need to bump the 
> > The changelog info was not updated
> 
> Um, yes, that was sloppy.  I don't have the -3 version around any more, but
> I've tried to retrofit now.

(In reply to comment #12)
> > Should at least include a correct upstream path
> 
> I didn't put any since there isn't any upstream path to the tar archive.  But
> I've added a comment on how the tar archive can be (re)made using a cvs and a
> tar command.  It includes the exact path to the CVS pserver.  Is this a good
> way to document where it comes from?
Yes, it's basically OK, but has an issue that it overwrites the archive of the actual release version, because the file name is the same. That is something to avoid; eg the resultant filename should include the %{_release} part, and hence indicate it is different to the release filename. See:
http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

Also, .cvsignore are left inside, along with a heap of empty directories (but maybe they are used in the build process - i didn't notice).

You can make and include a script like:
http://fedoraproject.org/wiki/Packaging:SourceURL#When_Upstream_uses_Prohibited_Code
shows.
An example: http://cvs.rpmfusion.org/viewvc/rpms/ffmpeg/devel/?root=free
ffmpeg-snapshot.sh
and include it as part of the package, eg: Source1 or whatever.


===== Is this the startup script ? 
cat > %buildroot%_bindir/projectx << EOF
#! /bin/bash
. /usr/share/java-utils/java-functions
MAIN_CLASS=net.sourceforge.dvb.projectx.common.Start
set_classpath %name commons-net jakarta-oro
run "\$@"
EOF
=====
I think it would be clearer to make this a source that sits in cvs, rather than build time generating it. Any changes can then be easily diffed within cvs

=====build fails
+ echo 'Patch (ProjectX.sysjava.patch):'
Patch (ProjectX.sysjava.patch):
+ /bin/cat /home/davidt/rpmbuild/SOURCES/ProjectX.sysjava.patch
+ /usr/bin/patch -s -p0 --fuzz=0
1 out of 1 hunk FAILED -- saving rejects to file MANIFEST.MF.rej
error: Bad exit status from /var/tmp/rpm-tmp.RMmIat (%prep)

RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.RMmIat (%prep)

Does the patch need to be rediffed ? There was a change in fedora to use fuzz0, and hence the patch needs to more closely match the source.

=====
Are you already sponsored in fedora or rpmfusion ?
What is your fas username ?

(In reply to comment #12)
> ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
gcj standard spec items:
=====
  BuildArchitectures: noarch
seems that should be BuildArch:
http://fedoraproject.org/wiki/Packaging/GCJGuidelines

=====
  %files
...
%if %with_gcj
%_libdir/gcj/%name

  Seems to be missing some bits as described "Add the following to the %files section:"

=====
Since I'm no expert on java nor scripts, my preference would be for all these bits to be exact copy paste from the GCJGuidelines page, rather than modified versions, since I can't see what the difference would achieve (eg security risk etc).

A script is included as you suggested that does the download and tar packaging.  (And excludes empty directories, I was fooled by my .cvsrc there.)  This script makes the date part of the name.

The start script in /usr/bin is a separate source file.  That change also made the separate %attr() no longer needed.

The tag name is adjusted to exactly match the wiki page.

I'm not quite sure what other gcj bits you find missing.  There are some additional comments on the wiki page on how to do with packages that have subpackages, but that doesn't apply to this package.  Or do you mean you want an explicit %attr() although there is a %defattr()?  I'm also no expert on Java, and try to follow the instructions.  But I'm not sure what you see that I miss.

I'm really confused why your build fails.  It builds fine for me.  Also with mock, which should rule out environment influences like .cvsrc above.  Furthermore, your error message says it saves the rejects to MANIFEST.MF.rej, but ProjectX.sysjava.patch only patches build.sh.  Is the ProjectX.sysjava.patch in your build environment not the one from the latest SRPM?

Updated version available at:
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-5.20100801cvs.fc14.src.rpm

> Are you already sponsored in fedora or rpmfusion ?

Yes, I am sponsored and maintain packages in Fedora already.  (Just a couple, but still.)

My FAS name is "goeran".

(In reply to comment #16)
====
> A script is included as you suggested that does the download and tar packaging.
>  (And excludes empty directories, I was fooled by my .cvsrc there.)  This
> script makes the date part of the name.
OK.

====
> The start script in /usr/bin is a separate source file.  That change also made
> the separate %attr() no longer needed.
OK, much easier to understand.

=====
> The tag name is adjusted to exactly match the wiki page.
OK.

=====
> I'm not quite sure what other gcj bits you find missing.  There are some
> additional comments on the wiki page on how to do with packages that have
> subpackages, but that doesn't apply to this package.  Or do you mean you want
> an explicit %attr() although there is a %defattr()?  I'm also no expert on
> Java, and try to follow the instructions.  But I'm not sure what you see
> that I miss.
Changes are OK, meets the guidelines.

===== 1. patch file naming
> I'm really confused why your build fails.  It builds fine for me.  Also with
> mock, which should rule out environment influences like .cvsrc above. 
> Furthermore, your error message says it saves the rejects to MANIFEST.MF.rej,
> but ProjectX.sysjava.patch only patches build.sh.  Is the
> ProjectX.sysjava.patch in your build environment not the one from the latest
> SRPM?
Correctamundo. I guess that's another of the not so obvious things: 
if a patch is named projectx-1.3.4-fix-libdir.patch, then when the patch needs to change to suit projectx-1.3.5, then the old patch would be removed, and the new one added with it's name indicating the reference upstream version. This aids clarity, and makes it easier to see the difference between branches, eg:
http://cvs.fedoraproject.org/viewvc/rpms/audacity/F-12/

Keeping the same name while the patch changes could allow issue (like mine - where I manually extract and review all the files, but since there was no mention of a change or obvious name change for the patch I didn't copy it into rpmbuild/SOURCES). In spec/packages I have come across, it seems fairly normal to to name patches referencing the base file version, so I think we should do the same here.

=====
> Updated version available at:
> ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
> ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-5.20100801cvs.fc14.src.rpm
- Builds OK.

===== Many build warnings:
...
Type safety: The method add(Object) belongs to the raw type ArrayList. References to generic type ArrayList<E> should be parameterized
----------
1369 problems (1369 warnings)+ cp 

OK. Not a direct packaging issues, although as you find time it would be good to submit patches to the upstream to take care of them.

===== later in build:
/src/net/sourceforge/dvb/projectx/parser/Scan.java:1980,
                 from <built-in>:4:
/home/davidt/rpmbuild/BUILD/ProjectX-0.90.4.00-20100801cvs/src/net/sourceforge/dvb/projectx/video/MpvDecoder.java:0: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without

- not sure if this is significant ?

- snapshot script works.

- 2. snapshot includes some bits we might as well nuke before adding to look-aside cache, mainly the /lib/os2, and /lib windows dll and precompiled included jars.
further removals for archive create:
lib]# file *
commons-net-1.3.0.jar: Zip archive data, at least v2.0 to extract
idctref.dll:           PE32 executable for MS Windows (DLL) (GUI) Intel 80386 32-bit
idctsse.dll:           PE32 executable for MS Windows (DLL) (GUI) Intel 80386 32-bit
jakarta-oro-2.0.8.jar: Zip archive data, at least v2.0 to extract
OS2:                   directory

- source archive: included archive and local archive created using the snapshot script create identical archives (expanded both, and diff -ur).

- might like to see if there is much size reduction by using the xz compression.

- rpmlint:
=====rpmlint src
$ rpmlint /home/davidt/rpmbuild/SRPMS/ProjectX-0.90.4.00-5.20100801cvs.fc12.src.rpm
ProjectX.src: W: spelling-error Summary(en_US) demultiplexing -> de multiplexing, de-multiplexing, multiplexing
ProjectX.src: I: enchant-dictionary-not-found sv
ProjectX.src: W: spelling-error %description -l en_US analyse -> analyst, analyze, ana lyse
ProjectX.src: W: spelling-error %description -l en_US demultiplex -> de multiplex, de-multiplex, multiplexer
ProjectX.src: W: strange-permission ProjectX-snapshot.sh 0775
ProjectX.src:113: W: libdir-macro-in-noarch-package (main package) %_libdir/gcj/%name
ProjectX.src: W: no-cleaning-of-buildroot %install
ProjectX.src: W: no-cleaning-of-buildroot %clean
ProjectX.src: W: no-buildroot-tag
ProjectX.src: W: no-%clean-section
ProjectX.src: W: invalid-url Source0: ProjectX-0.90.4.00-20100801cvs.tar.bz2
1 packages and 0 specfiles checked; 0 errors, 10 warnings.

=====rpmlint bin
rpmlint /home/davidt/rpmbuild/RPMS/x86_64/ProjectX-0.90.4.00-5.20100801cvs.fc12.x86_64.rpm
ProjectX.x86_64: W: spelling-error Summary(en_US) demultiplexing -> de multiplexing, de-multiplexing, multiplexing
ProjectX.x86_64: I: enchant-dictionary-not-found sv
ProjectX.x86_64: W: spelling-error %description -l en_US analyse -> analyst, analyze, ana lyse
ProjectX.x86_64: W: spelling-error %description -l en_US demultiplex -> de multiplex, de-multiplex, multiplexer
ProjectX.x86_64: W: no-manual-page-for-binary projectx
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

=====rpmlint bin -debuginfo
rpmlint /home/davidt/rpmbuild/RPMS/x86_64/ProjectX-debuginfo-0.90.4.00-5.20100801cvs.fc12.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

===== 3. libdir-macro-in-noarch-package
- not sure about this one. I thought gcj made native arch libraries / executable, and hence on an x86_64, this would need to point to lib64.

- also, why does it think it's a no-arch package when we are building native stuff ?

===== 4. missing help file:
Help|Help
missing resource ! :
htmls/Index.html

- wonder if that is a capitalization problem needing a patch.
- or need to patch source to look for the help in the right place.

> --- Comment #17 from Göran Uddeborg <goeran@uddeborg.se>  2010-08-03
> Yes, I am sponsored and maintain packages in Fedora already.  (Just a couple,
> but still.)
> 
> My FAS name is "goeran".
OK, confirmed. Hence I could approve package.
https://admin.fedoraproject.org/community/?username=goeran&_csrf_token=90de19743069dfb67ab1512f5701d1a28cab0690#people/package_maintenance

===== application bugs:
- select file open
- change to detail view
- click a folder
  -> the dir list is not updated to the sub folder
- click the sort view button
  -> detail list headers stay in the dialog
  -> the entries move left, overwriting what was previously the icons

===== 5. Requires:
Can you check whether rpm can autodetect these Requires, eg by leaving them out of the spec, and seeing whether you get useable executable ?

===== 6. I also realize that the inline desktop patching is not very understandable (even if it works). I would suggest placing this as a patch to the included .desktop file

===== Secondary review:
I'll ask anyone who has good java skills to cast there eyes over Göran'a next spec/srpm ...

Some quick replies first.  I'll be back with an updated version later on.  (Could take a few days before I have the time.)

> ===== 3. libdir-macro-in-noarch-package
> - not sure about this one.

As I understand it, rpmlint doesn't quite understand %if clauses in SPEC files.  The noarch directive and the line with libdir macro can never both be active.  It's either one or the other, depending on the value of %with_gcj.  But, apparently, rpmlint doesn't see that.

> Can you check whether rpm can autodetect these Requires

I believe I tried, and it couldn't.  But I'll double check.

(In reply to comment #19)
> Some quick replies first.  I'll be back with an updated version later on. 
> (Could take a few days before I have the time.)
> 
> > ===== 3. libdir-macro-in-noarch-package
> > - not sure about this one.
> 
> As I understand it, rpmlint doesn't quite understand %if clauses in SPEC files.
>  The noarch directive and the line with libdir macro can never both be active. 
> It's either one or the other, depending on the value of %with_gcj.  But,
> apparently, rpmlint doesn't see that.

This review is created long ago, recently many java packages have already droped gcj support(e.g. maven2 eclipse).

(In reply to comment #20)
> This review is created long ago, recently many java packages have already
> droped gcj support(e.g. maven2 eclipse).
Given I'm a java packaging beginner, what does the above mean for this package ?

eg: if it will go in F12 will it need the gcj bits from the packaging guidelines ?

Additionally, Chen, are you able to take a look at Göran's next spec, because I think we are close to having the package ready for approval ?

Here comes the next version.

- A version number is added to the sysjava patch.

- The snapshot script excludes the lib directory and a few other files irrelevant for Fedora when creating the archive.

- The archive was some 15% smaller with xz compression, so I switched to that.

- The desktop file modification is put in a separate patch file.

- The jar dependencies are not discovered automatically, so they are still in the spec file.

- The warnings you saw were signs of a mistake of mine.  Different Java compilers gives different warnings.  I didn't see them in my regular environment, where I have Java 1.6.0 installed.  The only warnings I saw were a couple about using a deprecated API.  (The authors mention those in the ReadMe file, so they are already aware.)  But the build requirements in the SPEC file only said Java 1.2.2 was needed, which is what the source documentation says is the minimum.  And when creating a minimal environment, the java compiler from GCJ met that requirement.  With that compiler, you get all the warnings.

I'm not sure how best to handle this.  For now I've added requirements on Java 1.6.0 or later.  I looked at the spec files of a couple of other, randomly choosen, Java packages, and they had that.  But it wasn't immediately clear if that was an actual requirement of the Java source in those packages, or if it was the same situation as mine.  Some further input from some more experienced Java packager on how to handle GCJ, if GCJ is to be supported at all, would be appreciated.  If it simplifies things if I only build for F13 or later, or only F14 or later, I would do that.

- The -fvar-tracking-assignment message is still there.  But it is only a "note", so I assume it's not important.  I don't really know, though.

- There are also some warnings when it tries to create the debuginfo package, which seems to be about temporary files created by the GCJ AOT compiler but not kept.  I have ignored those too, assuming that there is nothing to do about it, but again, I don't really know.

- Finally, the failure to show the help was a bit harder.  ProjectX is apparently designed to run where it was built, not to be installed.  So it looks for "htmls/index.html" locally from where it is started.  The string "htmls" is in one place in the code, but the addition of the current directory is done elsewhere, and the latter place is reached by several paths.  So there was no immediately obvious quick fix.

Reading the code and digging a bit in my memory about Java, I finally figured out that if I put the html pages inside the jar file rather than in the file system, the code will find them.  So I've added a third patch, modifying the build script to do this.

- As for the application bugs, are you sure the first one really is a bug?  If I select a subdirectory and click "open", the directory list is updated.  From what I understand, that could be how the dialog is supposed to work.

The second I don't quite understand.  Where is the "sort view button"?

Updated version available at:
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-6.20100806cvs.fc14.src.rpm

(In reply to comment #22)
> - A version number is added to the sysjava patch.
Ack.

> - The snapshot script excludes the lib directory and a few other files
> irrelevant for Fedora when creating the archive.
Ack.

> - The archive was some 15% smaller with xz compression, so I switched to that.
And most people will have xz since rpms uses it, so good pickup.

> - The desktop file modification is put in a separate patch file.
As I understand it, desktop-file-install validates the .desktop and adds the version number. Hence we shouldn't add:
+Version=1.0
 
> - Finally, the failure to show the help was a bit harder.  ProjectX is
... 
> Reading the code and digging a bit in my memory about Java, I finally figured
> out that if I put the html pages inside the jar file rather than in the file
> system, the code will find them.  So I've added a third patch, modifying the
> build script to do this.
Ack.

=====
> - As for the application bugs
Actually, we aren't explicitly required to fix bugs, however, the following is reasonable bad.

> The second I don't quite understand.  Where is the "sort view button"?
File|Add
change a few directories, use the up directory icon near top-right.
Also click the top-right "show detail" list icon. (I called that the wrong name, but it has no hint).

Things get really weird, eg. disk has /home/1/2/3/4/5 the path shown in the top dropdown is 3, but the dialog is showing the contents of the folder 5.
=====
Given both our lack of java rpm packaging experience, perhaps you could ask on either a fedora java or jpackage list about whether the spec is clear, follows guildelines, and is there anything else that should be added/changed ?

> File|Add
> change a few directories, use the up directory icon near top-right.
> Also click the top-right "show detail" list icon.

This is getting strange.  If I point JAVA_HOME to java-1.5.0-gcj, I can manage to get the drop-down and the content out of sync as you describe.  But double-clicking on a directory opens it.

When I don't set JAVA_HOME, getting the default java-1.6.0-openjdk instead, I can't get drop-down and content out of sync.  But double-clicking on a directory doesn't seem to work.  I also have some window refresh problems, sometimes I have to hide and expose a window again to see anything.

I wonder if it's the Java implementations, ProjectX, or a combination, that is buggy.  As I said in the beginning, I have only used it as a command line tool, so I haven't seen these issues before.

(In reply to comment #24)
> > File|Add
> > change a few directories, use the up directory icon near top-right.
> > Also click the top-right "show detail" list icon.
> 
> This is getting strange.  If I point JAVA_HOME to java-1.5.0-gcj, I can manage
> to get the drop-down and the content out of sync as you describe.  But
> double-clicking on a directory opens it.
Or lets you rename the folder (for me), rather than opening it.

I saw the response from Andrew, suggesting do you want gcj ?
I don't know the answer to this, nor the best way forward.
I am testing on my F12 machine (don't have f13 at the moment).
$ alternatives --config java

There are 2 programs which provide 'java'.
  Selection    Command
-----------------------------------------------
*+ 1           /usr/lib/jvm/jre-1.6.0-openjdk.x86_64/bin/java
   2           /usr/lib/jvm/jre-1.5.0-gcj/bin/java

If there is adjustments you can make so that it works OK on F13, and you only want to support F >= 13, then that would be OK.

(By the way, I'm going to be away from pc a few days).

I have removed the requirement for Java 1.6.  This is based on advice from Java people in the thread http://lists.fedoraproject.org/pipermail/java-devel/2010-August/003810.html continued at http://lists.fedoraproject.org/pipermail/java-devel/2010-August/003817.html.  It is apparently perfectly fine to use GCJ for building, and the warnings can be ignored.  (For the purpose of packaging, at least.)

No other changes were suggested.

Updated version available at:
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-7.20100806cvs.fc14.src.rpm

I spent some time to investigate the errors we discussed.  With OpenJDK 1.6.0 I can not reproduce them.

I found double-clicking on a directory works if I do it really fast.  Doing it slightly less fast I got to edit the name instead.  Googling a bit I figured out that this is a parameter that one can change.  By merging

*multiClickTime: 1000

with xrdb, I could double-click quite slowly.  So this might be a parameter with a bad default in Java, but not a bug in ProjectX.

I can't get the confusion between the dropdown and the window content you describe with this jre.  I managed to reproduce it once with the GCJ jre, but never with OpenJDK.  So this looks more like a jre bug than a ProjectX bug.  If you can provide me with a precise test case that reliably fails, I will report it upstreams.  But as it stands now, I don't think I have anything to report, except possibly on GCJ.

There is one somewhat odd issue that the window is not always initially drawn.  But I saw that in another Java application too, so could also be something outside ProjectX.  I need to pinpoint it better before I know what and most importantly to where to report it.

I've sent the suggestion to include the help files in the jar archive upstreams: https://sourceforge.net/tracker/?func=detail&atid=670053&aid=3044988&group_id=115063

(In reply to comment #27)
> I can't get the confusion between the dropdown and the window content you
> describe with this jre.  I managed to reproduce it once with the GCJ jre, but
> never with OpenJDK.  So this looks more like a jre bug than a ProjectX bug.
Are you planning to support only openjdk ?

Which Fedora releases ?

(Just want to see a successful build that essentially works, but I'm still F12).

> Are you planning to support only openjdk ?

No, both GCJ and OpenJDK.  If I understood the Java thread correctly, if I build with GCJ, as is done now, the resulting program will work with both OpenJDK and GCJ.  And it seems to work with my tests so far.  In some of the command line uses I've tried, GCJ is considerably faster.

(The problem with the file browser we have seen is obviously an exception to the statement that it works with both runtime systems.  I've tried to figure out if it is a GCJ or ProjectX bug, but I'm still not sure about that.  The file chooser dialog is a standard Swing component.  But when I make a small standalone program using it, I can't reproduce the bug with it.  I'll try a bit more to see if I can figure out where the bug report belongs.)

> Which Fedora releases ?

Well, that depends on how long it takes before we are both happy with the package.  I think F12 is a bit old for new packages by now.  (And for that reason I've removed the %clean section, since it isn't needed from F13 on.)  If I will build for F13 or only for F14 I will decide depending on how far F14 has come by the time.

> (Just want to see a successful build that essentially works, but I'm still F12).

What about using mock?  I just checked, and mock for F12 does contain configuration for F13 and rawhide.

(In reply to comment #26)
> ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX.spec
> ftp://ftp.uddeborg.se/pub/ProjectX/ProjectX-0.90.4.00-7.20100806cvs.fc14.src.rpm
OK, I had a try to test build, install and run on F13 x86_64.

This was successfull, using:
alternatives --config java

There are 2 programs which provide 'java'.

  Selection    Command
-----------------------------------------------
   1           /usr/lib/jvm/jre-1.5.0-gcj/bin/java
*+ 2           /usr/lib/jvm/jre-1.6.0-openjdk.x86_64/bin/java

No immediate issue with runtime operation of the program.

A fresh snapshot was compared with the srpm's included cvs snapshot. After extraction, a diff -ur between the two folder structures are identical.

I believe we have covered all the packaging guidelines and the specific java ones as well. Since there hasn't been any further expression of improvments to make, I am hearby APPROVING the above package.

Updating blocker to bug 4 to indicate ProjectX has been approved (RF_ACCEPT).

Thanks for beating me to packaging ProjectX !

Than YOU David for the review!

Now, could I have some help from a CVS admin, please:

Package CVS request
======================
Package Name: ProjectX
Short Description: DVB video editing and demultiplexing tool
Owners: goeran
Branches: F13
InitialCC:
----------------------
License tag: free

@Göran Uddeborg
You have to create an account in fas.rpmfusion.org separated from the fedora account. Please use an email address that match your bugzilla account.
Thx
Please reblock #33 once done

The separate RPM Fusion FAS account is there now.  I'm not yet in the “cvsextras” group, but I assume that will happen soon, since I'm already sponsored in Fedora.  (Or should I have waited for that too, before I reblocked bug 33?)

The package is now in the repositories.  Thanks everyone for your help!