Bug 469471 - Review Request: skinlf - Java look and feel for swing
Review Request: skinlf - Java look and feel for swing
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On: 461407
Blocks: 464781 tvbrowser
  Show dependency treegraph
 
Reported: 2008-11-01 00:59 EDT by D Haley
Modified: 2009-01-07 04:12 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-19 10:59:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description D Haley 2008-11-01 00:59:48 EDT
Package SkinLF, 6.7

Files:
Spec URL: http://dhd.selfip.com/427e/skinlf.spec
SRPM URL: http://dhd.selfip.com/427e/skinlf-6.7-1.fc9.src.rpm  

Description:
 Skin Look And Feel is able to read GTK (The Gimp ToolKit) and KDE (The K Desktop Environment) Skins to enhance your application GUI controls such as Buttons, Checks, Radios, Scrollbars, Progress Bar, Lists, Tables, Internal Frames, Colors, Background Textures, Regular Windows. This allows for the skinning of java applications to enhance their look and feel.
Comment 1 D Haley 2008-11-16 07:21:10 EST
Files:
Spec URL: http://dhd.selfip.com/427e/skinlf-2.spec
SRPM URL: http://dhd.selfip.com/427e/skinlf-6.7-2.fc9.src.rpm  

- Fix version numbering
- Fix top level dir when building jar
Comment 2 Chitlesh GOORAH 2008-11-23 08:00:09 EST
#001: remove the copyright paragraph, else I won't review this package

#002: avoid renaming spec files : %{name}-X.spec. Always keep the spec filenme as %{name}.spec. I have seen you did the same for all your packages.

#003: rpmlint issues
chitlesh(~)[0]$rpmlint /home/chitlesh/rpmbuild/RPMS/i386/skinlf-6.7-2.fc9.i386.rpm
skinlf.i386: W: no-documentation
skinlf.i386: E: description-line-too-long Today many applications must be skinnable: your CD player, your emailnotifier, even your operating system! Now with Skin Look And Feel, your Java application is skinnable! Skin Look And Feel is able to read GTK (The Gimp ToolKit) and KDE (The K Desktop Environment) Skins to enhance your application GUI controls such as Buttons, Checks, Radios, Scrollbars, Progress Bar, Lists, Tables, Internal Frames, Colors, Background Textures, Regular Windows.
skinlf.i386: W: non-standard-group skinlf
skinlf.i386: W: invalid-license Apache
skinlf.i386: E: no-binary

Break the description paragraph into seperate lines.

004: Build errors, possible missing BuildRequires:
     [javac]                       ^                                                                             
    [javac] /home/chitlesh/rpmbuild/BUILD/skinlf-6.7/build/src/com/l2fprod/tools/msstyles/MsStylesToSkinLF.java:78: warning: com.sun.org.apache.xpath.internal.XPathAPI is Sun proprietary API and may be removed in a future release                                                                                                           
    [javac]       (Element)XPathAPI.                                                                            
    [javac]                ^
Comment 3 D Haley 2008-11-23 09:47:22 EST
Spec URL:
SRPM URL:

> #001: remove the copyright paragraph, else I won't review this package
Done.

>#002: avoid renaming spec files : %{name}-X.spec. Always keep the spec filenme
>as %{name}.spec. I have seen you did the same for all your packages.

I'm going to do it again, just to be difficult -- my local copy is always blahblah.spec, only the bugzilla copies are blahblah-rev.spec. Sorry. How do I make sure other people on the 'net can see the changes in the spec files after I have made them? Is there a good way to do this?

>#003: rpmlint issues
>skinlf.i386: W: no-documentation
Fixed.

>skinlf.i386: E: description-line-too-long
Fixed.

>skinlf.i386: W: non-standard-group skinlf
Fixed.

>004: Build errors, possible missing BuildRequires:
>...
>warning: com.sun.org.apache.xpath.internal.
>XPathAPI is Sun proprietary API and may be removed in a future release  
Not technically errors, these warnings arise due to my usage of the sun xpath api as a substitute for the org.apache.xpath api (patch #3), which was removed as of sun JRE1.5 [1]. I can look at this again to see if I can make it work without the patch, but that will have to wait. The package itself will still work with the sun API, it just means that I may have to revisit and update later. I'd rather continue with the current sun API if no-one knows how to solve this properly, and doesn't object to doing so. Just so we can lift the block on other bugs


>skinlf.i386: W: invalid-license Apache
Not fixed -- I am unsure how to proceed here. If you examine the LICENSE file in the source, it isn't Apache, contrary to what's on their website [2]. Its a redistributable with attribution licence, which doesn't appear up in the rpmlint -iv output. Which licence should I select??


Current RPMlint output:
$ rpmlint -i skinlf-6.7-3.fc9.src.rpm 
skinlf.src: W: invalid-license Apache 2.0


Thanks for taking a look at the package!

[1] http://java.sun.com/j2se/1.5.0/docs/guide/xml/jaxp/JAXP-Compatibility_150.html#New
[2] https://skinlf.dev.java.net/
Comment 4 D Haley 2008-11-23 09:48:55 EST
*sigh* One more time... 

URLS:
Spec: http://www.dhd.selfip.com/427e/skinlf-3.spec
SRPM: http://www.dhd.selfip.com/427e/skinlf-6.7-3.fc9.src.rpm
Comment 5 D Haley 2008-11-23 09:54:08 EST
*sigh* again. There is no www prefix. 

URLS:
Spec: http://dhd.selfip.com/427e/skinlf-3.spec
SRPM: http://dhd.selfip.com/427e/skinlf-6.7-3.fc9.src.rpm
Comment 6 Rudolf Kastl 2008-11-23 21:13:58 EST
build seems to error out aswell without laf-plugin installed so shouldnt that also have a BuildRequires on it?
Comment 7 D Haley 2008-11-24 07:13:41 EST
>build seems to error out aswell without laf-plugin installed so shouldnt that
>also have a BuildRequires on it?

Does Requires not imply BuildRequires? I was under the (maybe incorrect) assumption that buildrequires was just a weaker form of requires (ie Requires, but only at build time)
Comment 8 Mamoru TASAKA 2008-11-24 07:51:01 EST
(In reply to comment #7)
> >build seems to error out aswell without laf-plugin installed so shouldnt that
> >also have a BuildRequires on it?
> 
> Does Requires not imply BuildRequires? 

No.
Comment 9 D Haley 2008-11-28 19:09:20 EST
Spec: http://dhd.selfip.com/427e/skinlf-4.spec
SRPM: http://dhd.selfip.com/427e/skinlf-6.7-4.fc9.src.rpm

* Sat Nov 29 2008 <mycae(a!t)yahoo.com> 6.7-4
- Updated BuildRequires to inlcude laf-plugin
- Silence several rpmlint errors
	- ASL 2.0 vs Apache Source Licence 2.0
	- Fix arch
	- Fix EOL on docs.

>build seems to error out aswell without laf-plugin installed so shouldnt that
>also have a BuildRequires on it?

Fixed

rpmlint:

$ rpmlint -iv ../SRPMS/skinlf-6.7-4.fc9.src.rpm 
skinlf.src: I: checking
$ rpmlint -iv ../RPMS/noarch/skinlf-6.7-4.fc9.noarch.rpm 
skinlf.noarch: I: checking
$ rpmlint -iv skinlf.spec 

Known issues :

comment 3
>>skinlf.i386: W: invalid-license Apache
>Not fixed -- I am unsure how to proceed here. If you examine the LICENSE file
>in the source, it isn't Apache, contrary to what's on their website [2]. Its a
>redistributable with attribution licence, which doesn't appear up in the
>rpmlint -iv output. Which licence should I select??
Comment 10 Mamoru TASAKA 2008-12-02 11:53:21 EST
* Licensing
  - I guess src/examples/Clock.java is non-free. So if this file is not
    needed, please remove this file (and also .jar/.dll files) and
    repackage the zip source:
    https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

    * Then the license tag must be "ASL 1.1 and ASL 2.0 and zlib".
---------------------------------------------------------------------------
LICENSE:		ASL 1.1
LICENSE_nanoxml:	zlib

src/com/l2fprod/gui/plaf/skin/LafPluginSupport.java	ASL 2.0
src/com/l2fprod/util/WeakImageIcon.java                 ASL 2.0
src/examples/Clock.java	???
---------------------------------------------------------------------------
    * Also, add "LICENSE_nanoxml" to %doc.

  - If src/examples/Clock.java is needed, I wil ask spot whether the
    license is free or not.

* BuildRequires, Requires
  - For java depedency, please follow:
    https://fedoraproject.org/wiki/Packaging/Java#BuildRequires_and_Requires
    If this package needs Java openjdk, the BuildRequires should have
    "BuildRequires: java-devel >= 1:1.6.0".
Comment 11 D Haley 2008-12-03 08:11:12 EST
SPEC URL: http://dhd.selfip.com/427e/skinlf-5.spec
SRPM URL: http://dhd.selfip.com/427e/skinlf-6.7-5.fc9.srpm

RPMLint on SRPM, SPEC and RPM are empty

>* Also, add "LICENSE_nanoxml" to %doc.
Done.

> If src/examples/Clock.java is needed, I will ask spot whether the
> license is free or not.
Done. Removed by deletion in %prep -- the examples are only compressed into the jar and are not required at compile. 

Q: How do I provide the srpm? With Clock.java or without -- if I make it with, it still contains it (just not the RPM). If I make it without then it will require me to recompress the source tarball, making the Source0 line invalid. 

> (and also any .jar.dll files)
Not needed/implicit. The output of find ./ -name  blah, where blah is *.jar, *so and *dll, is empty. This was the case for the prior revision. The prior revision included the standard check for java files from the packaging guidelines to ensure that the package will not build if any file *.jar is located within the build tree. This check is performed at %prep time.

> BuildRequires, Requires
Done. Imported any missing java deps from previous package "laf-plugin". Specifically jpackage-utils and updated java lines for req & buildreq.
Comment 12 Sandro Mathys 2008-12-05 18:17:56 EST
just two things I noticed (I'm about to create a package that will depend on skinlf):

1) there jar should be named %{name}-%{version}.jar with a symlink %{name}.jar to it. Right now, only %{name}.jar is installed to the %{_javadir}. ls -al /usr/share/java will show you, that (most?) packages do it like that.

2) the jar should contain the class files, not the source files ;)

instead of:
pushd .
cd build/src
jar cf %{name}-%{version}.jar .
popd

do:
pushd build/classes
jar cf %{name}-%{version}.jar .
popd

of course, you'll need to change the path in the install cmd after that, too.

I've tested that change and it works fine.
Comment 13 D Haley 2008-12-05 21:18:35 EST
I'm actually away for the better part of a week, however I have access to my webserver, but not my build machine (aka my desktop). As such I have uploaded a modified "tentative" spec file. Be warned that I cannot even test for approximate correctness.
URL: http://dhd.selfip.com/427e/skinlf-6-tentative.spec


>1) there jar should be named %{name}-%{version}.jar with a symlink %{name}.jar
>to it. Right now, only %{name}.jar is installed to the %{_javadir}. ls -al
>/usr/share/java will show you, that (most?) packages do it like that.

Changed name & linked. I did look at this and in my java dir, IIRC, it was split between %{name}-%{version} and %{name} for the primary file.

>2) the jar should contain the class files, not the source files ;)
Fixed. Well that was stupid. 

I hope this package is almost there. My packages seem to have a nasty tendency of being a mess at the start -- something I need to attend to in the future.

Thanks.
Comment 14 Mamoru TASAKA 2008-12-07 10:16:07 EST
Sorry for delay...

Seems good, however as I wrote in my comment 10 (and as you wondered)
since Clock.java must be removed from source zip file, the zip file
must be re-packaged. Please follow:

https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code
Comment 15 D Haley 2008-12-10 04:43:59 EST
SPEC URL: http://dhd.selfip.com/427e/skinlf-6.spec
SRPM URL: http://dhd.selfip.com/427e/skinlf-6.7-6.fc9.src.rpm 

>Seems good, however as I wrote in my comment 10 (and as you wondered)
>since Clock.java must be removed from source zip file, the zip file
>must be re-packaged.
Done.

rpmlint output is empty for spec, SRPM and RPM. I set the permissions on the tarball generator to 644, so if you wish to use it, you have to set execute permissions.
Comment 16 Mamoru TASAKA 2008-12-11 12:00:13 EST
Seems good, approved.
---------------------------------------------------------
   This package (skinlf) is APPROVED by mtasaka
---------------------------------------------------------
Comment 17 D Haley 2008-12-11 17:02:55 EST
New Package CVS Request
=======================
Package Name: skinlf
Short Description: Java look and feel for swing 
Owners: mycae
Branches: F-9 F-10
InitialCC:
Comment 18 Kevin Fenzi 2008-12-13 23:54:53 EST
cvs done.
Comment 19 Fedora Update System 2008-12-14 05:35:29 EST
skinlf-6.7-6.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/skinlf-6.7-6.fc9
Comment 20 Fedora Update System 2008-12-14 05:36:13 EST
skinlf-6.7-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/skinlf-6.7-6.fc10
Comment 21 Mamoru TASAKA 2008-12-15 09:42:27 EST
Please also rebuild for dist-f11.
Comment 22 D Haley 2008-12-16 06:56:17 EST
Do I need to repeat my CVS req?
Comment 23 Mamoru TASAKA 2008-12-16 10:28:36 EST
No. "devel" branch is already created on Fedora CVS and
you can already rebuild this package for dist-f11.
Comment 24 Fedora Update System 2008-12-17 19:36:40 EST
skinlf-6.7-6.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 skinlf'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11321
Comment 25 Fedora Update System 2008-12-17 19:42:14 EST
skinlf-6.7-6.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 skinlf'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-11295
Comment 26 Fedora Update System 2009-01-07 04:06:53 EST
skinlf-6.7-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2009-01-07 04:12:01 EST
skinlf-6.7-6.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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