Bug 462521 - Review Request: simplyhtml - Application and a java component for rich text processing
Summary: Review Request: simplyhtml - Application and a java component for rich text p...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mary Ellen Foster
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-09-16 20:59 UTC by John Guthrie
Modified: 2009-04-23 19:39 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-05 08:10:44 UTC
mefoster: fedora-review+


Attachments (Terms of Use)

Description John Guthrie 2008-09-16 20:59:17 UTC
Spec URL: http://www.guthrie.info/RPMS/f9/simplyhtml.spec
SRPM URL: http://www.guthrie.info/RPMS/f9/simplyhtml-0.12.3-2.fc9.src.rpm
Description: SimplyHTML is an application and a java component for rich text
processing. It stores documents as HTML files in combination with
Cascading Style Sheets (CSS). SimplyHTML is not intended to be used
as an editor for web pages.

Comment 1 John Guthrie 2008-09-16 21:11:52 UTC
This package has a dependency on javahelp2 which currently exists in the devel tree.  Thus, this package would be targeted only for rawhide and F10.

(I'm not certain how does a case like this get handled.)

Comment 2 Mary Ellen Foster 2009-02-06 15:28:00 UTC
(Using Jason Tibbitts' review template from
http://fedoraproject.org/wiki/User:Tibbs/Review_Template)

[-] source files match upstream:
    I followed the instructions in the spec file and ended up with
    something with a different md5sum than the file in the SRPM. The
    readme.txt and gpl.txt files do match though

    You could just use the upstream .zip file directly, though. Just change
    your %setup line to 
        %setup -q -c %{name}-%{version}
    and it'll create the directory when it needs to.

[+] package meets naming and versioning guidelines.
[+] specfile is properly named, is cleanly written and uses macros consistently.
[+] dist tag is present.
[+] build root is correct.
[+] license field matches the actual license.
[+] license is open source-compatible.
[+] license text included in package.
[-] latest version is being packaged.
    It looks like upstream has released 0.12.5

[+] BuildRequires are proper.
[+] compiler flags are appropriate.
[+] %clean is present.
[-] package builds in mock.
    error: %patch without corresponding "Patch:" tag
    You should have "%patch0", not "%patch", on line 62
    For the remainder of this review I made this change

[+] package installs properly.
[+] debuginfo package looks complete.
[-] rpmlint is silent.
    One warning to deal with:
    simplyhtml.src: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 20)
    The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
    annoyance.  Use either spaces or tabs for indentation, not both.

    Other warnings are about Group tags or confusion with the gcj
    noarch-ness

[+] final provides and requires are sane:
    Provides:
        simplyhtml-0.12.3.jar.so
        simplyhtml = 0:0.12.3-2.fc10
        simplyhtml(x86-32) = 0:0.12.3-2.fc10
    Requires:
        /bin/sh
        gnu-regexp
        java
        java-gcj-compat >= 1.0.31
        javahelp2
        jpackage-utils
        libc.so.6
        libc.so.6(GLIBC_2.1.3)
        libdl.so.2
        libgcc_s.so.1
        libgcc_s.so.1(GCC_3.0)
        libgcj_bc.so.1
        libm.so.6
        libpthread.so.0
        librt.so.1
        libz.so.1
        rpmlib(CompressedFileNames) <= 3.0.4-1
        rpmlib(PayloadFilesHavePrefix) <= 4.0-1
        rtld(GNU_HASH)

[+] no shared libraries are added to the regular linker search paths.
[+] owns the directories it creates.
[+] doesn't own any directories it shouldn't.
[+] no duplicates in %files.
[+] file permissions are appropriate.
[+] gcj scriplets are correct
[+] code, not content.
[+] documentation is small, so no -docs subpackage is necessary.
[+] %docs are not necessary for the proper functioning of the package.
[+] no headers.
[+] no pkgconfig files.
[+] no libtool .la droppings.

[-] Package doesn't run
    I recommend creating a small shell script to run the program with the
    correct CLASSPATH -- if you just try to run the jar file, it doesn't
    find the gnu-regexp classes.

    Something like this:

    #!/bin/sh

    exec java -cp `build-classpath gnu-regexp javahelp2 simplyhtml` \
        com.lightdev.app.shtm.App

[-] Other source files are included:
    Please check the status and the necessity of using the files in
    src/com/sun and de/calcom as they appear to come from other projects.

Comment 3 John Guthrie 2009-02-06 20:44:20 UTC
(In reply to comment #2)
> (Using Jason Tibbitts' review template from
> http://fedoraproject.org/wiki/User:Tibbs/Review_Template)
> 
> [-] source files match upstream:
>     I followed the instructions in the spec file and ended up with
>     something with a different md5sum than the file in the SRPM. The
>     readme.txt and gpl.txt files do match though
> 
>     You could just use the upstream .zip file directly, though. Just change
>     your %setup line to 
>         %setup -q -c %{name}-%{version}
>     and it'll create the directory when it needs to.

That would probably make my life noticeably easier.

> [-] latest version is being packaged.
>     It looks like upstream has released 0.12.5

That's not too surprising since I first posted this review request 4.5 months ago.  Let me download the new source and rebuild.

> [-] Package doesn't run
>     I recommend creating a small shell script to run the program with the
>     correct CLASSPATH -- if you just try to run the jar file, it doesn't
>     find the gnu-regexp classes.
> 
>     Something like this:
> 
>     #!/bin/sh
> 
>     exec java -cp `build-classpath gnu-regexp javahelp2 simplyhtml` \
>         com.lightdev.app.shtm.App

When and where would I want to do this?  During compile-time perhaps?

(There are other blockers that I haven't addressed in this message.  I was just picking a few blockers to deal with at first.)

I will post a new SRPM and spec soon.

Comment 4 John Guthrie 2009-02-08 05:02:52 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > [-] Package doesn't run
> >     I recommend creating a small shell script to run the program with the
> >     correct CLASSPATH -- if you just try to run the jar file, it doesn't
> >     find the gnu-regexp classes.
> > 
> >     Something like this:
> > 
> >     #!/bin/sh
> > 
> >     exec java -cp `build-classpath gnu-regexp javahelp2 simplyhtml` \
> >         com.lightdev.app.shtm.App
> 
> When and where would I want to do this?  During compile-time perhaps?

Never mind this comment.  I was totally mis-parsing what you were trying to say.  I figured it out. ;-)

Comment 5 John Guthrie 2009-02-08 07:34:43 UTC
(In reply to comment #2)
I have posted a new spec file and SRPM.  Here are the URLs:
Spec: http://www.guthrie.info/RPMS/f10/simplyhtml.spec
SRPM: http://www.guthrie.info/RPMS/f10/simplyhtml-0.12.5-2.fc10.src.rpm

> [-] source files match upstream:
>     I followed the instructions in the spec file and ended up with
>     something with a different md5sum than the file in the SRPM. The
>     readme.txt and gpl.txt files do match though
> 
>     You could just use the upstream .zip file directly, though. Just change
>     your %setup line to 
>         %setup -q -c %{name}-%{version}
>     and it'll create the directory when it needs to.

For the 0.12.5 version, the upstream is now offering the source in .tar.gz format.  I am now using that unmodified.

> [-] latest version is being packaged.
>     It looks like upstream has released 0.12.5

This is fixed.  0.12.5 is now packaged.

> [-] package builds in mock.
>     error: %patch without corresponding "Patch:" tag
>     You should have "%patch0", not "%patch", on line 62
>     For the remainder of this review I made this change

This is fixed.

> [-] rpmlint is silent.
>     One warning to deal with:
>     simplyhtml.src: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line
> 20)
>     The specfile mixes use of spaces and tabs for indentation, which is a
> cosmetic
>     annoyance.  Use either spaces or tabs for indentation, not both.

This is fixed.

> [-] Package doesn't run
>     I recommend creating a small shell script to run the program with the
>     correct CLASSPATH -- if you just try to run the jar file, it doesn't
>     find the gnu-regexp classes.
> 
>     Something like this:
> 
>     #!/bin/sh
> 
>     exec java -cp `build-classpath gnu-regexp javahelp2 simplyhtml` \
>         com.lightdev.app.shtm.App

I did this almost verbatim except that I added simplyhtml-help to the build-classpath command.

> [-] Other source files are included:
>     Please check the status and the necessity of using the files in
>     src/com/sun and de/calcom as they appear to come from other projects.

The short story is that from what I can tell, these files actually are imported into the files for this project.  I will talk more about this later when it is not 2AM and I am needing sleep. ;-)

Comment 6 John Guthrie 2009-02-09 04:37:09 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > [-] Other source files are included:
> >     Please check the status and the necessity of using the files in
> >     src/com/sun and de/calcom as they appear to come from other projects.
> 
> The short story is that from what I can tell, these files actually are imported
> into the files for this project.  I will talk more about this later when it is
> not 2AM and I am needing sleep. ;-)

This could end up being a bit of a mess.  Just looking at the file com/sun/demo/ExampleFileFilter.java, the class defined by this file gets imported into three different files under com.lightdev.  Moreover, these files use methods that are defined in ExampleFileFilter.java.  It would be difficult to extricate this class from the com.lightdev tree without making semi-substantial changes to the code.  On further inspection, one also sees that the ExampleFileFilter class extends the FileFilter class.  The documentation for that class can be found here:  http://java.sun.com/j2se/1.4.2/docs/api/javax/swing/filechooser/FileFilter.html  According to that page, the FileFilter class is in fact an abstract class, so it is expected that a subclass will be written in order to implement this class.  It appears that this particular subclass, ExampleFileFilter, was written to, well, be an example implementation.  However, I am guessing that the example code did everything that the simplyhtml upstream wanted, and so they just used it directly, rather than writing their own implementation.  Fortunately, ExampleFileFilter.java has a 3-clause BSD license with no advertising clause.

Similar comments apply to ElementTreePanel.java.

All of the de.calcom code has been placed under GPL just like the rest of the com.lightdev code.  IT turns out that the lightdev.com and calcom.de domains are owned by the same person, one Ulrich Hilger.  Moreover, looking at the calcom.de web site, it would appear that calcom.de was a previous name for lightdev.com.  Based on this, it would seem safe to assume that the de.calcom code is part of the "same" project as the com.lightdev code.

Is there anything that we would need to worry about with the com.sun.demo code having a BSD license vs. the rest of the code having a GPL license?

Comment 7 Mary Ellen Foster 2009-02-09 16:29:25 UTC
I asked on fedora-devel-java-list about the com.sun.* classes, and unfortunately, it looks like since the license of these files includes
    * You acknowledge that this software is not designed, licensed or intended
    * for use in the design, construction, operation or maintenance of any
    * nuclear facility.
they can't be used in Fedora. Here's the mailing list thread:
    https://www.redhat.com/archives/fedora-devel-java-list/2009-February/msg00008.html

For the ExampleFileFilter, it doesn't look like it would be too difficult to patch the code to use the javax.swing.filechooser.FileNameExtensionFilter that was introduced in JDK 1.6 and that provides similar functionality.

For the ElementTreePanel, I'm not sure if there's an alternative implementation, but I haven't looked too hard. In the worst case, I guess you could patch out the use of the ElementTreePanel entirely, and then there would be some functionality missing from simplyhtml.

Comment 8 John Guthrie 2009-02-09 22:33:41 UTC
(In reply to comment #7)
> I asked on fedora-devel-java-list about the com.sun.* classes, and
> unfortunately, it looks like since the license of these files includes
>     * You acknowledge that this software is not designed, licensed or intended
>     * for use in the design, construction, operation or maintenance of any
>     * nuclear facility.
> they can't be used in Fedora. Here's the mailing list thread:
>    
> https://www.redhat.com/archives/fedora-devel-java-list/2009-February/msg00008.html
> 
> For the ExampleFileFilter, it doesn't look like it would be too difficult to
> patch the code to use the javax.swing.filechooser.FileNameExtensionFilter that
> was introduced in JDK 1.6 and that provides similar functionality.
> 
> For the ElementTreePanel, I'm not sure if there's an alternative
> implementation, but I haven't looked too hard. In the worst case, I guess you
> could patch out the use of the ElementTreePanel entirely, and then there would
> be some functionality missing from simplyhtml.

I'm sitting here chuckling to myself a little bit because I saw that clause, and I actually thought to myself,  "Yeah, I can understand why someone might want to keep Java code out of a nuclear facility.  <sarcasm>Mmmmm... Java code and nuclear power plants.  Two tastes that go great together.</sarcasm>"  I can also understand now why this might be a bad thing from Fedora's point of view.

Thanks for the pointer on FileNameExtensionFilter.  I'll see what I can do with that for now.

I'll also have to keep that "no nukes" clause in mind as well. ;-)

Comment 9 Mary Ellen Foster 2009-02-11 15:40:32 UTC
Good news -- looks like there's a newer version of ElementTreePanel included in the java-1.6.0-openjdk-demo package inside /usr/lib/jvm/java-1.6.0-openjdk-1.6.0.0/demo/jfc/Notepad/src.zip. According to the fedora-devel-java mailing list (same thread as before), you should just be able to delete the included version and copy this one in instead. The license will have to become something like "GPLv2+ and BSD" (http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios).

Comment 10 John Guthrie 2009-02-13 04:06:15 UTC
(In reply to comment #9)
> Good news -- looks like there's a newer version of ElementTreePanel included in
> the java-1.6.0-openjdk-demo package inside
> /usr/lib/jvm/java-1.6.0-openjdk-1.6.0.0/demo/jfc/Notepad/src.zip. According to
> the fedora-devel-java mailing list (same thread as before), you should just be
> able to delete the included version and copy this one in instead. The license
> will have to become something like "GPLv2+ and BSD"
> (http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios).

That is SOOOO cool.  I was very seriously considering deleting the class and removing the code from the package.  Thank you so much for finding this replacement.

In any event, I have successfully replaced ExampleFileFilter with FileNameExtensionFilter.  The result can be seen here:  http://www.guthrie.info/RPMS/f10/simplyhtml-0.12.5-3.fc10.src.rpm.  The new spec file has the same URL as given above.

Comment 11 John Guthrie 2009-02-13 04:18:21 UTC
What would be the proper/preferred way to include this newer ElementTreePanel.java file?  Would I want to just extract the file myself and include it in the SRPM as another source file, or would I want to specify a java-1.6.0-openjdk-demo as a BuildRequirement, and then extract the java file from that package during the %prep phase?  (The java file would not be included with the SRPM in the latter option, of course.)

Comment 12 Mary Ellen Foster 2009-02-23 16:54:47 UTC
Sorry, $DAYJOB exploded on me for a couple of weeks there. :)

It's more elegant to BuildRequire the demo package and move the file where you need it (+ change package, whatever), but I don't think it would be wrong to just include the file itself as a separate Source: file in the SRPM. Whichever you prefer, I'd say.

Comment 13 John Guthrie 2009-02-24 06:42:01 UTC
(In reply to comment #12)
> Sorry, $DAYJOB exploded on me for a couple of weeks there. :)

No problem.  I suppose that it's good that $DAYJOB != "".  ;-)

> It's more elegant to BuildRequire the demo package and move the file where you
> need it (+ change package, whatever), but I don't think it would be wrong to
> just include the file itself as a separate Source: file in the SRPM. Whichever
> you prefer, I'd say.

That's what I was thinking.  I didn't know if there was any downside though.  Here is the new SRPM URL with the new BuildRequire:

http://www.guthrie.info/RPMS/f10/simplyhtml-0.12.5-4.fc10.src.rpm

The Spec file URL is still the same, but updated.

Comment 14 Mary Ellen Foster 2009-02-24 08:35:54 UTC
Looks good. Just one tiny rpmlint warning from the SRPM:

simplyhtml.src: W: strange-permission simplyhtml.sh 0755
A file that you listed to include in your package has strange permissions.
Usually, a file should have 0644 permissions.

Since you have the %attr line in the %files section, I don't think that the source file needs any special permissions itself. This is APPROVED (but please change the permissions on that one source file before importing, just for neatness. :) )

Comment 15 John Guthrie 2009-02-25 05:37:20 UTC
Thank you very much.  That last permission change can be seen at http://www.guthrie.info/RPMS/f10/simplyhtml-0.12.5-5.fc10.src.rpm
for reference.

Onto the CVS procedure now...

Again, thank you for all of your help.

Comment 16 John Guthrie 2009-02-25 05:45:15 UTC
New Package CVS Request
=======================
Package Name: simplyhtml
Short Description: Application and a java component for rich text processing
Owners: guthrie
Branches: F-9 F-10
InitialCC:

Comment 17 Kevin Fenzi 2009-02-27 00:22:45 UTC
cvs done.

Comment 18 John Guthrie 2009-03-05 08:10:44 UTC
It now builds successfully in CVS.  Closing.

Comment 19 John Guthrie 2009-04-23 19:36:05 UTC
Package Change Request
======================
Package Name: simplyhtml
New Branches: F-11
Owners: guthrie

Comment 20 John Guthrie 2009-04-23 19:39:00 UTC
Never mind.  I was trying to check out the new branch incorrectly.  I figured it out now.


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