Bug 464781 - Review Request: flexdock - Java docking UI element. First package.
Review Request: flexdock - Java docking UI element. First package.
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dominik 'Rathann' Mierzejewski
Fedora Extras Quality Assurance
:
Depends On: 469471
Blocks: RussianFedoraRemix 472639 492203
  Show dependency treegraph
 
Reported: 2008-09-30 09:53 EDT by D Haley
Modified: 2009-05-02 00:45 EDT (History)
9 users (show)

See Also:
Fixed In Version: 0.5.1-11.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-23 11:50:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dominik: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Suggested specfile fixes (4.32 KB, patch)
2009-02-15 19:19 EST, Dominik 'Rathann' Mierzejewski
no flags Details | Diff

  None (edit)
Description D Haley 2008-09-30 09:53:06 EDT
Spec URL: http://dhd.selfip.com/427e/flexdock.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-1.fc9.src.rpm 
Description: flexdock java class files & JNI generated binary. Provides Swing GUIs with a dock user interface element. Uploading as library is required for (later) packaging of Scilab.
Comment 1 D Haley 2008-09-30 09:56:46 EDT
I had to fiddle the %install section to get it to work, as the apache ant build.xml that is provided doesn't seem to have any facility for *installing* the package.
Comment 2 Nicolas Chauvet (kwizart) 2008-10-12 17:17:04 EDT
Please read http://fedoraproject.org/wiki/Packaging/Guidelines
and specially http://fedoraproject.org/wiki/Packaging/Java
You have to use macro for the spec file (that what miss most to "start" review).

Please also read http://fedoraproject.org/wiki/PackageMaintainers
So you can get sponsored.

For license problem, it is the one on top of each source file (if any) that matter.

I have matio ready if you want to save time on scilab5 packaging...(i will submit it soon).
Comment 3 Dominik 'Rathann' Mierzejewski 2008-10-12 18:02:25 EDT
(In reply to comment #0)
> Spec URL: http://dhd.selfip.com/427e/flexdock.spec
> SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-1.fc9.src.rpm 
> Description: flexdock java class files & JNI generated binary. Provides Swing
> GUIs with a dock user interface element. Uploading as library is required for
> (later) packaging of Scilab.

Are you going to package Scilab, too, then? That's great. Let me know if I can help either with packaging or reviewing. I can also sponsor you, but please do a couple of reviews and point me to them.
Comment 4 D Haley 2008-10-13 03:45:26 EDT
Yes, I am aiming to get a scilab package uploaded. I have a (private) basic scilab package all up and running (minus GUI), but I wanted to work out the approval/packaging process. The only thing its missing to get the scilab working in GUI mode is flexdock :)

Nicolas:
As of scilab 5, I believe matio is not directly required and scilab can (optionally) use its own code (or embedded code) to support these operations. (reference: http://wiki.scilab.org/Dependencies_of_Scilab_5.X). 

The java guidelines are a bit out of date, and keep talking about using the non sun java components (IcedTea, gcj); it's hard to tell what does and does not apply here.

> You have to use macro for the spec file
By macro I assume you mean that i should be installing the JNI .so files to %{_libdir}/%{name}? I can update the spec file accordingly. Otherwise if you are referring to the general format of the spec file, the original spec file was generated using vim, as per (my interpretation of) the packaging guidelines. Is something incorrect?

Dominik:
I read the guidelines a few times, for me the process is still a little fuzzy. I am following the procedure outlined here: http://fedoraproject.org/wiki/PackageMaintainers/Join, and am up to the part " Watch for Feedback" -- How do I fix the FE-NEEDSPONSOR -- It seems a little bit of a chicken and egg problem, I have to submit packages and get approval to get a sponsor, but can't do that until I have a sponsor... ?

The guidelines say "When the package is APPROVED by the reviewer, you must separately obtain member sponsorship in order to check in and build your package." -- so shouldn't I be trying to get approval now??? Confused.


Thanks for the help
Comment 5 Dominik 'Rathann' Mierzejewski 2008-10-13 06:16:55 EDT
(In reply to comment #4)
> Dominik:
> I read the guidelines a few times, for me the process is still a little fuzzy.
> I am following the procedure outlined here:
> http://fedoraproject.org/wiki/PackageMaintainers/Join, and am up to the part "
> Watch for Feedback" -- How do I fix the FE-NEEDSPONSOR -- It seems a little bit
> of a chicken and egg problem, I have to submit packages and get approval to get
> a sponsor, but can't do that until I have a sponsor... ?

You can submit packages and do non-authoritative reviews before getting sponsored. In fact, that's the best way to convince a someone (me, for example) to sponsor you. After that person approves you for membership in the "packager" group in the Fedora Account System, you will be able to import and build any approved package.

> The guidelines say "When the package is APPROVED by the reviewer, you must
> separately obtain member sponsorship in order to check in and build your
> package." -- so shouldn't I be trying to get approval now??? Confused.

You should find someone to sponsor you. Usually, sponsorship happens at the same time as your first package is approved. As I said, I can do that after I'm satisfied that you have learned to follow our packaging guidelines.

I'll take the review of this package now.
Comment 6 D Haley 2008-10-26 04:43:48 EDT
Hello,

Sorry about that, but the spec file I uploaded was completely bogus: don't bother officially reviewing the package at this stage :(. 

I have modified the spec file to create a jar file and install that, rather than installing all the .class files everywhere. Unfortunately my examination of the package has shown more problems than just .class files.

Problem:
Flexdock depends on several other libraries:
*Java media framework 
http://java.sun.com/javase/technologies/desktop/media/jmf/
       ./lib/jmf/lib/jmf.jar 
       ./lib/jmf/lib/mediaplayer.jar
       ./lib/jmf/lib/multiplayer.jar
       ./lib/jmf/lib/customizer.jar -- 

*"Looks" project
 https://looks.dev.java.net/
       ./lib/looks-2.1.1.jar

*skinlf "Skin look and Feel" project
 https://skinlf.dev.java.net/ 
       ./lib/skinlf.jar

Each of which don't appear to have fedora packages at this stage. Unless I am allowed to use the .jar files supplied with flexdock, flexdock cannot build. I have uploaded the new .spec file in case anyone has any comments.

Note that the ant-jmf seems to cause a build failure, and thus cannot be used:
compile:
    [javac] Compiling 229 source files to /home/makerpm/rpmbuild/BUILD/flexdock-0.5.1/build/bin
    [javac] Note: Some input files use or override a deprecated API.
    [javac] Note: Recompile with -Xlint:deprecation for details.
    [javac] Compiling 29 source files to /home/makerpm/rpmbuild/BUILD/flexdock-0.5.1/build/bin-demo
    [javac] /home/makerpm/rpmbuild/BUILD/flexdock-0.5.1/src/java/demo/org/flexdock/demos/raw/jmf/MediaPanel.java:21: package javax.media does not exist
    [javac] import javax.media.ControllerEvent;
    [javac]                   ^


Am I supposed to now create packages for each of the missing deps? If someone can tell me what I am supposed to do now, that would be great! 

Thanks.
Comment 7 Mamoru TASAKA 2008-10-26 05:32:45 EDT
Well,
https://fedoraproject.org/wiki/Packaging/Java#Pre-built_JAR_files_.2F_Other_bundled_software

Removing all pre-compiled .jar files (.class files etc) at %prep is _mandatory_
on Fedora to make it sure that all files are correctly built from FOSS file.

If this package needs some external jar files you must package them in advance
(example: bug 428798 , especially the discussion from bug 428798 comment 11 )
Comment 8 D Haley 2008-10-26 06:05:50 EDT
Looks like this might be dead in the water.

Unless this is out of date, the JMF is available under the "Sun Community Source Licensing", which is not allowed in fedora per http://fedoraproject.org/wiki/Licensing.

Might have to raise this as a bug at scilab.
Comment 9 D Haley 2008-10-26 06:06:31 EDT
Sorry about the dup, forgot the JMF FAQ link:
http://java.sun.com/javase/technologies/desktop/media/jmf/reference/faqs/index.html#jmfsource
Comment 10 Mamoru TASAKA 2008-10-26 07:57:57 EDT
Well, while I cannot find out yet where to download JMF source file
itself, as far as I read the link in your comment 9 jmf cannot be
included in Fedora.
Comment 11 D Haley 2008-10-26 08:52:08 EDT
This has been reported as a bug at scilab, as this package is intended to support scilab-5.0.1

http://bugzilla.scilab.org/show_bug.cgi?id=3696
Comment 12 Dominik 'Rathann' Mierzejewski 2008-10-28 18:01:42 EDT
Looks like you got an answer from the Debian maintainer of flexdock along with some patches to fix the problem. Please go ahead and incorporate them into your package and post a new spec+source rpm set for review.
Comment 13 D Haley 2008-11-01 02:07:07 EDT
I have uploaded the new spec & src rpm. Now pending upon deps.
Comment 14 Dominik 'Rathann' Mierzejewski 2008-11-01 12:37:25 EDT
You are supposed to increase the revision of the rpm (and document any changes in the %changelog section) and post the complete urls for spec and srpm in the bugzilla comment.
Comment 15 Mamoru TASAKA 2008-11-02 08:38:19 EST
mycae, if you modified your spec/srpm, would you change the release number and
post the URLs of your new spec/srpm?
Comment 16 D Haley 2008-11-16 07:28:05 EST
Spec URL: http://dhd.selfip.com/427e/flexdock-4.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-4.fc9.src.rpm 

- Update package revision number
- Fix Top level dir
Comment 17 Chitlesh GOORAH 2008-11-23 07:30:21 EST
#
# Copyright  (c) 2008  Daniel Haley
#
# This file and all modifications and additions to the pristine 
#  package are under the same license as the package itself.  
#
# please send bugfixes or comments to mycae@yahoo.com
#


Hmm isn't this a blocker for review ? There should be no other copyright/license on any fedora spec file except fedora's.
Comment 18 Chitlesh GOORAH 2008-11-23 07:33:58 EST
#001: Missing BuildRequires: ant-commons-logging

#002: Rpmlint issues;

chitlesh(~)[0]$rpmlint /home/chitlesh/rpmbuild/RPMS/i386/flexdock-0.5.1-4.fc9.i386.rpm
flexdock.i386: W: no-documentation
flexdock.i386: W: non-standard-group Development/Java
flexdock.i386: W: incoherent-version-in-changelog 0.1-4 ['0.5.1-4.fc9', '0.5.1-4']
flexdock.i386: W: no-soname /usr/lib/libRubberBand.so
flexdock.i386: E: shlib-with-non-pic-code /usr/lib/libRubberBand.so
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

chitlesh(~)[0]$rpmlint /home/chitlesh/rpmbuild/RPMS/i386/flexdock-debuginfo-0.5.1-4.fc9.i386.rpm
flexdock-debuginfo.i386: E: empty-debuginfo-package
Comment 19 Chitlesh GOORAH 2008-12-01 13:35:19 EST
#001: missing ant-commons-logging as Build Requires

#002: add -verbose to ant
Comment 20 D Haley 2008-12-03 09:56:07 EST
Spec URL: http://dhd.selfip.com/427e/flexdock-6.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-6.fc9.src.rpm 

>Hmm isn't this a blocker for review ? There should be no other
>copyright/license on any fedora spec file except fedora's.
Gone. I thought this was part of the package licence's requirements -- but its not.

>#001: Missing BuildRequires: ant-commons-logging
Fixed.

>#002: Rpmlint issues;
rpmlint for SRPM,spec and RPM are empty.

>#001: missing ant-commons-logging as Build Requires
Fixed.
>#002: add -verbose to ant
Added, but this produces an extremely long output -- significantly greater than the scrollback on my terminal. Not good for building unless you (1) log or (2) redirect or both.


Known issues:
*I am not actually removing the skinlf jar, as the code to do this is commented out. I discussed this with akurtakov previously & will revisit this soon.


* Thu Dec 04 2008 <mycae(a!t)yahoo.com> 0.5.1-6
- Use ant to build jar, rather than straight from bash.
- Fix build patch to correct native .so building
- Thanks goes to akurtakov for assitance.
- Fixed dos->unix line ending for doc files
- Added dos2unix buildrequires
- Fix arch flag
- Added libX11 dep for native so
- Added symlink for so
Comment 21 Jason Tibbitts 2008-12-03 12:47:44 EST
For the record, it is acceptable for specfiles to be explicitly licensed as long as that license is accepted for software in Fedora:
  http://fedoraproject.org/wiki/Licensing#License_of_Fedora_SPEC_Files
Comment 22 D Haley 2008-12-19 19:10:27 EST
Spec URL: http://dhd.selfip.com/427e/flexdock-7.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-7.fc9.src.rpm 

*Sat Dec 20 2008  <mycae(a!t)yahoo.com> 0.5.1-7
- Re-enable system skinlf link & jar check.

spec file, SRPM and RPM give empty rpmlint output

This was updated as skinlf has now been fixed & flexdock now builds correctly.
Comment 23 Dominik 'Rathann' Mierzejewski 2009-01-19 17:55:25 EST
Some comments before full review:

Why do you have:
BuildArch: i386
BuildArch: x86-64
BuildArch: ppc
in the spec? BuildArch: can only be specified once, so merge these three. Otherwise it fails with:

$ rpmbuild -bb flexdock.spec
error: No compatible architectures found for build

x86-64 is not a valid arch designation, by the way. You should use x86_64.
I think you should use ExcludeArch: ppc64 to disable the unsupported arch instead and open a bug for that (blocking FE-ExcludeArch-ppc64 tracker bug).

Fails to build in mock on rawhide/i386:
[...]
++ build-classpath jgoodies-looks skinlf
/usr/bin/build-classpath: error: Could not find jgoodies-looks Java extension for this JVM
/usr/bin/build-classpath: error: Could not find skinlf Java extension for this JVM
/usr/bin/build-classpath: error: Some specified jars were not found
[...]

You seem to have forgotten to add these as BuildRequires as well.

Once the above are fixed, it builds in mock, but rpmlint output is not clean:
$ rpmlint /var/lib/mock//fedora-rawhide-i386/result
flexdock-debuginfo.i386: E: empty-debuginfo-package
flexdock.src:157: E: files-attr-not-set
flexdock.src:158: E: files-attr-not-set
flexdock.src:159: E: files-attr-not-set
3 packages and 0 specfiles checked; 4 errors, 0 warnings.

You need to fix that.

According to Java Packaging Guidelines, you're installing the files in the wrong location. Please fix that.

Also, the specfile has lots of trailing whitespace and inconsistent usage of spaces and tabs for indentation. Please fix that, too.
Comment 24 Dominik 'Rathann' Mierzejewski 2009-01-19 17:57:01 EST
Removing FE-NEEDSPONSOR, as you are already sponsored.
Comment 25 D Haley 2009-01-20 09:08:07 EST
Hopefully this package is near completion. I must be close to winning a most revised package award.

SPEC URL: http://dhd.selfip.com/427e/flexdock-8.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-8.fc10.src.rpm 


Now that deps are satisified, I have provided a scratch build.

Koji scratch:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1068723flexdock-0.5.1-8.fc10.src.rpm 

I swear that the rpmlint is emty. really!
[makerpm@box SPECS]$ rpmlint ../SRPMS/flexdock-0.5.1-8.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[makerpm@box SPECS]$ rpmlint ../RPMS/i386/flexdock-0.5.1-8.fc10.i386.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[makerpm@box SPECS]$ rpmlint flexdock.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

>$ rpmbuild -bb flexdock.spec
>error: No compatible architectures found for build

Now I'm not sure why I have not left this out. I see no reason that PPC64 is special, at least not considering the changes to the sdk.home.

>You seem to have forgotten to add these as BuildRequires as well.

Added

>$ rpmlint /var/lib/mock//fedora-rawhide-i386/result
>flexdock-debuginfo.i386: E: empty-debuginfo-package
>flexdock.src:157: E: files-attr-not-set[makerpm@spiderbox SPECS]$ rpmlint -i ../RPMS/i386/flexdock-0.5.1-8.fc10.i386.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[makerpm@spiderbox SPECS]$ rpmlint -i ../SRPMS/flexdock-0.5.1-8.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[makerpm@spiderbox SPECS]$ rpmlint -i flexdock.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

>flexdock.src:158: E: files-attr-not-set
>flexdock.src:159: E: files-attr-not-set
>3 packages and 0 specfiles checked; 4 errors, 0 warnings.
Defattr set, strip command modified to only unneeded, rather than all.


>According to Java Packaging Guidelines, you're installing the files in the
>wrong location. Please fix that.

Re-Reading:
http://fedoraproject.org/wiki/Packaging/Java#Packaging_JAR_files_that_use_JNI

I have moved them into their own subfolder /%{_libdir}/%{name}/. The origin of this particular rationale is somewhat unclear to me. Having examined a long  thread from March 2007 (http://osdir.com/ml/linux.redhat.fedora.java/2007-03/msg00044.html), consensus was at best unclear. Nevertheless, they are moved.

>Also, the specfile has lots of trailing whitespace and inconsistent usage of
>spaces and tabs for indentation. Please fix that, too.
Trailing whitespace is now gone -- I dont see any indentation issues (grep), as such this has not been fixed. Also rpmlint warns mixed tab/space indenting -- am I not understanding something?

$  grep -E '^\ ' flexdock.spec 
 
$

There was a trailing endline, and I got rid of it.
Comment 26 Dominik 'Rathann' Mierzejewski 2009-01-20 15:40:41 EST
Some more comments:

Requires:	libX11 >= 1.1.4	

Why do you need a specific libX11 version? Please add a comment with an explanation.

Also, please sort both Requires: and BuildRequires: alphabetically for better readability.


%description
	FlexDock is a Java docking framework for use in cross-platform
	Swing applications

Why is the description indented? Also, it's missing the end period.


#Modify the jni dir that is hardcoded in the patch
cp -pf %{PATCH0} %{PATCH0}.tmp
sed -i 's!%%{_libdir}/%%{name}/!%{_libdir}/%{name}/!' %{PATCH0}

Why don't you just modify the patch?


		if [ `uname -m` == "x86_64" ] || [ `uname -m` == "ppc64" ] ; then
			JDK_DIR=`echo %{_jdkdir} | sed 's!/$!!'`.`uname -m` 

I suggest using a case statement here. It'll be easy to add other arches later. For example sparc64.


#OK
# Apache commons Logging component
# http://commons.apache.org/logging/
#	./lib/commons-logging-1.1.jar
rm -f ./lib/commons-logging-1.1.jar
ln -s %{_javadir}/commons-logging.jar ./lib/commons-logging-1.1.jar

#remove jmf, as it is only used in a demo,
#which is unused after patching
rm -rf ./lib/jmf/

#" Looks" project
# https://looks.dev.java.net/
rm -f	./lib/looks-2.1.1.jar
ln -s %{_javadir}/jgoodies-looks.jar ./lib/looks-2.2.1.jar

#skinlf "Skin look and Feel" project
# https://skinlf.dev.java.net/ 
rm -f ./lib/skinlf.jar
ln -s %{_javadir}/skinlf.jar ./lib/skinlf.jar

Manually making symlinks is fragile, especially for versioned JARs. It'll break when their version changes. I think you should use build-jar-repository for that:
https://fedoraproject.org/wiki/Packaging/Java#build-jar-repository


	dos2unix -d2u $i;

What does -d2u option do? It's not documented in --help or in the manpage.


flexdock has funny arch flags, such as "libRubberBand-linux-x86.so" on i386
SOFILE=`find ./ -name libRubberBand*so`

Couldn't you just patch the build system not to put arch in lib filename?


strip --strip-unneeded $SOFILE

Calling strip from the specfile is not allowed. rpm post-build scripts do that while building the debuginfo package.


%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

These are not needed, as the library is not in ld.so search path (and not directly linkable).


*Tue Jan 20 2009 <mycae(a!t)yahoo.com> 0.5.1-8
- Set defattr
- Fix arch (again)
- Change install dir from %%libdir to %%libdir/%%name 
- Update patch0 to match changed so dir + make dynamic
*Sat Dec 20 2008  <mycae(a!t)yahoo.com> 0.5.1-7
- Re-enable system skinlf link & jar check.

Changelog entries must have a space between * and date.
Comment 27 Dominik 'Rathann' Mierzejewski 2009-01-20 15:53:24 EST
(In reply to comment #26)
[...]
> strip --strip-unneeded $SOFILE
> 
> Calling strip from the specfile is not allowed. rpm post-build scripts do that
> while building the debuginfo package.

Although in this case they seem to fail. If I remove that strip call, I get:
$ rpmlint /var/lib/mock//fedora-rawhide-i386/result
flexdock-debuginfo.i386: E: empty-debuginfo-package
flexdock.i386: W: unstripped-binary-or-object /usr/lib/flexdock/libRubberBand-0.so
Comment 28 Dominik 'Rathann' Mierzejewski 2009-01-20 16:31:36 EST
(In reply to comment #27)
> (In reply to comment #26)
> [...]
> > strip --strip-unneeded $SOFILE
> > 
> > Calling strip from the specfile is not allowed. rpm post-build scripts do that
> > while building the debuginfo package.
> 
> Although in this case they seem to fail. If I remove that strip call, I get:
> $ rpmlint /var/lib/mock//fedora-rawhide-i386/result
> flexdock-debuginfo.i386: E: empty-debuginfo-package
> flexdock.i386: W: unstripped-binary-or-object
> /usr/lib/flexdock/libRubberBand-0.so

Actually it's simple. Just don't chmod the .so to 644. It has to be executable for rpmbuild debuginfo scripts to find it. rpmlint is silent afterwards.

Some more nitpicks:

You don't need
mkdir -p %{buildroot}/usr/share

There's still trailing whitespace in the following lines:
Name:           flexdock

# This patch is fedora specific -- System.loadLibrary fix to help locate JNI components

#Licence is MIT on their website, Apache in their LICENSE.txt

patch3:         flexdock-skinlfTitlebarui-path.patch
patch4:         flexdock-skinlfPainter-path.patch

BuildRequires:  java-1.6.0-openjdk-devel

Requires:       libX11 >= 1.1.4

                        JDK_DIR=`echo %{_jdkdir} | sed 's!/$!!'`.`uname -m`

                echo "Relying on spec file buildpath: $JDK_DIR " >> tmpLog
                echo "sdk.home=$JDK_DIR"  > workingcopy.properties

# https://skinlf.dev.java.net/

ant  -Dbuild.sysclasspath=first build.with.native jar
Comment 29 D Haley 2009-01-22 06:09:42 EST
SPEC URL: http://dhd.selfip.com/427e/flexdock-9.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-9.fc10.src.rpm 

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

$ rpmlint flexdock.spec  ../RPMS/i386/flexdock-0.5.1-9.fc10.i386.rpm ../SRPMS/flexdock-0.5.1-9.fc10.src.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

>There's still trailing whitespace in the following lines
I re-ran the whitespace removal  through vim. What is so bad about trailing whitespace anyway - provided it isn't multi-kilobyte?

>mkdir -p %{buildroot}/usr/share
Gone.

>Why don't you just modify the patch?
Because of lib64 vs lib.

>-d2u

It forces dos to unix conversion mode. I now just use sed instead, its neater and is one less require.

>Couldn't you just patch the build system not to put arch in lib filename?

The way they do this, from memory is a bit odd. Patching would require more work than simply locating the SOFILE -- locating the SOFILE is easier and the project is not changing that much, and the SOFILE name wont change a lot, so it is maintainable. Either way the end result is the same -- we locate the SOFILE. Using find is marginally slower, but I am not writing my spec files for speed.

>strip --strip-unneeded $SOFILE

Calling strip is not allowed because you need the debugging symbols, however calling strip-unneeded does not intefere with the debugging symbols. However to make it neater I have changed this (see next item)

> Actually it's simple. Just don't chmod the .so to 644. It has to be executable
> for rpmbuild debuginfo scripts to find it. rpmlint is silent afterwards.

Done.

>Manually making symlinks is fragile, especially for versioned JARs.

Versioned jars were not being symlinked, although named ones were. The version number is to satisfy the flexdock buildsys. I have switched it to using build-jar-repository, but I feel that the new method is less straightforward, and more likely to break. I personally think the new method is more obscure.

>These are not needed, as the library is not in ld.so search path (and not
directly linkable).

Dropped post/postun

>I suggest using a case statement here. It'll be easy to add other arches later.
>For example sparc64.

Done. 

> Changelog entries must have a space between * and date.
Done.

>Why do you need a specific libX11 version?
Gone. I cant seem to track down any reason for this.
Comment 30 Dominik 'Rathann' Mierzejewski 2009-02-15 19:19:09 EST
Created attachment 331996 [details]
Suggested specfile fixes

Full review:

rpmlint is clean
$ rpmlint .
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

    * MUST: The package must meet the Packaging Guidelines.

%define _jdkdir %{_jvmdir}/java-1.6.0-openjdk-1.6.0.0

I suggest using simply

%{_jvmdir}/java-1.6.0

instead, which will allow you to drop the

JDK_DIR=`echo %{_jdkdir} | sed 's!/$!!'`.`uname -m`

hack later (see attached patch). Is java-1.6 (not older and not newer) strictly required?

Patch0: flexdock-jni-patch.patch

+        File file = new File("%{_libdir}/%{name}/");^

That '/' at the end is not necessary. Also the patch file name has a redundant 'patch' in it, same for
others.


BuildRequires: jpackage-utils

is listed twice.


#Override the build file's default hard-coded paths
if [ x"$JAVACMD" != x"" ] ; then
        echo "using RPM jnidir"  >> tmpLog
        echo sdk.home=%{_jnidir}-`$JAVACMD -version` > workingcopy.properties
else
        if [ x"$JAVA_HOME" != x"" ] ; then
                echo "Using JAVA_HOME env. var. :" $JAVA_HOME >> tmpLog
                echo "sdk.home=$JAVA_HOME" > workingcopy.properties
        else
                JDK_DIR=%{_jdkdir}
                echo "Relying on spec file buildpath: $JDK_DIR " >> tmpLog
                echo "sdk.home=$JDK_DIR"  > workingcopy.properties
        fi
fi

Why is the above necessary instead of:
echo "sdk.home=%{_jvmdir}/java-1.6.0" > workingcopy.properties

You could lose the %define _jdkdir at the beginning of the specfile then, too.


Also see the attached patch for more cosmetic fixes.

    * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.

Most files have no licencing information while others are licenced under MIT licence. Given the presence
of LICENSE.txt, this is OK, but please ask upstream to include licencing information at the top of each
source file

    * MUST: The License field in the package spec file must match the actual license.

#Licence is MIT on their website, Apache in their LICENSE.txt 
License: MIT and ASL 2.0

Wrong. LICENSE.txt is actually word-for-word MIT: https://fedoraproject.org/wiki/Licensing/MIT#Modern_Style_with_sublicense

    * MUST: 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.

88fd43d7d8db92e9480200c316e55056  flexdock-0.5.1-src.zip

    * SHOULD: The reviewer should test that the package builds in mock.

    * SHOULD: The package should compile and build into binary rpms on all supported architectures.

Doesn't build on F-9/x86_64 and F-9/i386 (java bug?).
Comment 31 D Haley 2009-02-19 08:52:29 EST
Apologies for delay in the response, haven't had a chance to look at this in the past few days.

SPEC URL: http://dhd.selfip.com/427e/flexdock-10.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-10.fc10.src.rpm 

Scratch:
F9:http://koji.fedoraproject.org/koji/taskinfo?taskID=1139730
F10:http://koji.fedoraproject.org/koji/taskinfo?taskID=1139731

$ rpmlint ../SRPMS/flexdock-0.5.1-10.fc10.src.rpm flexdock.spec ../RPMS/i386/flexdock-0.5.1-10.fc10.i386.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.


>#Licence is MIT on their website, Apache in their LICENSE.txt 
>License: MIT and ASL 2.0
>Wrong. LICENSE.txt is actually word-for-word MIT:
>https://fedoraproject.org/wiki/Licensing/MIT#Modern_Style_with_sublicense

Fixed.

>Doesn't build on F-9/x86_64 and F-9/i386 (java bug?).
Between changes from my earlier f-9 srpms to now, a new buildrequires was needed. Added:

BuildRequires:	ant-apache-regexp

Hence the build for F9 is now fixed.

>That '/' at the end is not necessary. Also the patch file name has a redundant
>'patch' in it, same for others.
I don't see the problem with having such things there, but to aid review process I have removed these.

>Why is the above necessary instead of:
>echo "sdk.home=%{_jvmdir}/java-1.6.0" > workingcopy.properties

Changed. This was based upon a jpackage script.

>BuildRequires: jpackage-utils
>is listed twice.
>Also see the attached patch for more cosmetic fixes.
Applied, with thanks.

>Is java-1.6 (not older and not newer) strictly required?
No, this was part of the hack. Changed to java-devel, java and %{_jvmdir}/java
Comment 32 Dominik 'Rathann' Mierzejewski 2009-02-23 15:51:58 EST
Very nice, however... the %changelog entry you've just added contains an empty line:
-

Yes, I know I'm being awfully picky. ;)

Of course you can fix it after importing, because this package is now APPROVED.
Great work and thanks for bearing with me.
Comment 33 Dominik 'Rathann' Mierzejewski 2009-02-23 16:38:38 EST
Looks like I was a bit too hasty. Please remove the jmf library from the original source archive. We can't distribute that, it's non-free. While you're at it, you could also remove the other binaries, since you have to repackage the source archive anyway.
Comment 34 D Haley 2009-02-24 03:35:51 EST
SPEC URL: http://dhd.selfip.com/427e/flexdock-11.spec
SRPM URL: http://dhd.selfip.com/427e/flexdock-0.5.1-11.fc10.src.rpm

Koji scratch:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1154545

rpmlint:
$ rpmlint ../SRPMS/flexdock-0.5.1-11.fc10.src.rpm  ../RPMS/i386/flexdock-0.5.1-11.fc10.i386.rpm flexdock.spec
2 packages and 1 specfiles checked; 0 errors, 0 warnings.


>Please remove the jmf library from the original source archive
Done.

> you could also remove the other binaries
Done. The JMF has the nice side effect of reducing size of package. ;)

>Very nice, however... the %changelog entry you've just added contains an empty
>line:
Removed.

>Thanks for bearing with me.
Whatever makes for good packages is a good thing.
Comment 35 Dominik 'Rathann' Mierzejewski 2009-02-24 20:45:24 EST
One last small nitpick:
BuildRequires: jpackage-utils
is listed twice. Looks like it slipped in after I asked you to fix it in comment #30. Please fix it again upon import.

This package is APPROVED.
Comment 36 D Haley 2009-02-24 22:49:05 EST
New Package CVS Request
=======================
Package Name: flexdock
Short Description: Docking framework for Java Swing GUI apps
Owners: mycae
Branches: F-9 F-10
InitialCC:
Comment 37 Kevin Fenzi 2009-02-26 19:14:02 EST
cvs done.
Comment 38 Fedora Update System 2009-02-28 22:23:50 EST
flexdock-0.5.1-11.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/flexdock-0.5.1-11.fc10
Comment 39 Fedora Update System 2009-02-28 22:24:58 EST
flexdock-0.5.1-11.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/flexdock-0.5.1-11.fc9
Comment 40 Fedora Update System 2009-03-04 11:26:10 EST
flexdock-0.5.1-11.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update flexdock'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-2321
Comment 41 Fedora Update System 2009-03-04 11:28:30 EST
flexdock-0.5.1-11.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update flexdock'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-2342
Comment 42 Dominik 'Rathann' Mierzejewski 2009-03-22 20:22:06 EDT
Could this package be moved to stable and the review bug closed as NEXTRELEASE?
Comment 43 Fedora Update System 2009-03-23 11:50:13 EDT
flexdock-0.5.1-11.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 44 Fedora Update System 2009-03-23 11:59:45 EDT
flexdock-0.5.1-11.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 45 Orcan Ogetbil 2009-03-25 12:18:37 EDT
Could you please add an unversioned symlink to the versioned jar file inside %{_libdir} ?

Something like 
   ln -s %{name}-%{version}.jar %{buildroot}%{_libdir}/%{name}/%{name}.jar

Most java packages carry these symlinks and they provide convenience on depending packages (Have a look at your /usr/share/java/ for instance).

Thanks.
Comment 46 D Haley 2009-03-25 18:12:34 EDT
>Could you please add an unversioned symlink to the versioned jar file inside
>%{_libdir} ?

Not for a JNI component as I understand.
https://fedoraproject.org/wiki/Packaging/Java#Packaging_JAR_files_that_use_JNI
Comment 47 Orcan Ogetbil 2009-03-25 18:51:43 EDT
I don't get that implication from those guidelines. 

I'm packaging frinika (bug 492203). For this one, I had to put your jar file in the classpath. But what will happen if there's a minor update in flexdock? Let's say upstream makes a minor bugfix release 0.5.1.1. In this case your jar file will be renamed, and I have to change my classpath and rebuild my package. But I shouldn't have to update my package for such a case.
Comment 48 Orcan Ogetbil 2009-03-25 18:57:27 EDT
For instance, take a look at our libreadline-java package.
Comment 49 D Haley 2009-03-25 21:01:03 EDT
OK, but could you explain where I am misunderstanding? The bit that I thought was important is this:

>Second, placing the JAR file in %{_javadir} causes the build-classpath script to >always load it, even when running on a runtime environment of the wrong arch, >meaning that the System.loadLibrary line would fail. 

I will have a look at libreadline-java in the next few days
Comment 50 Orcan Ogetbil 2009-03-25 22:21:31 EDT
Yes, but that has nothing to do with what I am saying.

I say: create a symlink
   /usr/lib64/flexdock/flexdock.jar
that points to
   /usr/lib64/flexdock/flexdock-0.5.1.jar

This has nothing to do with %{_javadir}, which is /usr/share/java. So the 
build-classpath script will still not see it.
Comment 51 D Haley 2009-04-14 07:12:19 EDT
Hi Orcan,

Yes, just to clarify, I pushed that in as an update. Sorry I misread your original post as asking for a link in /usr/share/java/
Comment 52 Orcan Ogetbil 2009-04-14 13:21:25 EDT
No problem, thanks for taking care of this :)

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