Bug 532203 - Review Request: jgraph - Java-based Diagram Component and Editor
Summary: Review Request: jgraph - Java-based Diagram Component and Editor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: F13FeatureIDEA
: 252084 472793 (view as bug list)
Depends On:
Blocks: 532205
TreeView+ depends on / blocked
 
Reported: 2009-10-31 13:09 UTC by Lubomir Rintel
Modified: 2009-11-24 00:13 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-11-17 11:29:11 UTC
Type: ---
Embargoed:
mycae: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Lubomir Rintel 2009-10-31 13:09:44 UTC
SPEC: http://v3.sk/~lkundrak/SPECS/jgraph.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/jgraph-5.12.2.1-2.fc12.src.rpm

Description:

JGraph is the a lightweight and feature-rich graph component
for Java. It provides automatic 2D layout and routing for diagrams.
Object and relations can be displayed in any Swing UI via provided
zoomable compontent. It is accompanied by diagram editor, JGraphpad.

Comment 1 D Haley 2009-11-01 09:57:40 UTC
Replying to some comments from previous (deadreview) bug (Bug 472793)

Notes
===
$ rpmlint jgraph.spec
jgraph.spec:68: W: non-standard-group Development/Documentation

I believe this can be ignored, as it matches the template spec files in the java packaging guidelines. 

jgraph.spec:135: W: libdir-macro-in-noarch-package (main package) %attr(-,root,root) %{_libdir}/gcj/%{name}
Rpmlint is incorrect, rpmlint is failing to detect the %if conditional around the noarch. So no problems here

So rpmlint output is OK.
===


Items to be addressed:
===

> > https://bugzilla.redhat.com/show_bug.cgi?id=472292

>This is pretty much irrelevant to this package review.

I believe it may be relevant, as otherwise your -debuginfo package is broken. There are workarounds shown in both that bug (bug 472292) and in bug 191014. Please fix this.

I think this package is almost done. Please post a spec fixing the above and I will review the package.
===

Random comments:
===
>Umm, no that's definitely not needed for review, feel free to do that yourself
>(technically, one architecture-release combination is needed and there's no
>clear reason why would they fail, nor indication that packager intends to
>create branches for these).

When doing your CVS request, please create a branch for F-11 - its is definitely not EOL, and is hence *supported*. F-10 is no longer required as it will very soon be EOL. 

It is not always clear why a given version will fail (sometimes deps are missing, sometimes compiler crashes, documentation build failure, or whatever may be the case. 

Providing scratch builds provides assurance that this is not the case. I prefer not to review packages which may fail to work under any given F- distribution that is not near EOL.

Note the following SHOULD from the package review checklist:
"SHOULD: The package should compile and build into binary rpms on all supported architectures."

>I prefer not using dos2unix for endline conversion
>This is a matter of taste and I'd prefer to follow packager's one, thus no
>change here.

Fair enough.

> > Fully standards-compliant (What standard? ISO 9001? Why do I (user) care?)

> Interoperability?

Apologies for the following rant, feel free to skip it as the current description is good :)

-- BEGIN RANT --
ISO9000 and friends have nothing to do with interoperability (http://en.wikipedia.org/wiki/ISO_9001). They are workplace quality systems ISOs, and really have nothing to do with software (or anything really.) They were popular a few years ago amongst marketing and manager types. It is entirely possible that upstream is simply being facetious here, as actual accreditation is a long and complex (and quite boring) procedure, which individual developers would be unlikely to undertake.
-- END RANT --
===

Comment 2 D Haley 2009-11-01 09:59:23 UTC
Oh sorry, one more comment.

Please re-instate the changelog entry by Mary, which looks to have been inadvertently deleted
+- Added gcj stuff

This should be inserted after line 160.

Comment 3 Lubomir Rintel 2009-11-01 12:29:13 UTC
(In reply to comment #1)
> Replying to some comments from previous (deadreview) bug (Bug 472793)
> 
> Notes
> ===
> $ rpmlint jgraph.spec
> jgraph.spec:68: W: non-standard-group Development/Documentation
> 
> I believe this can be ignored, as it matches the template spec files in the
> java packaging guidelines. 

Might be. I replaced this with Documentation, just to be on safe side.

> >This is pretty much irrelevant to this package review.
> 
> I believe it may be relevant, as otherwise your -debuginfo package is broken.
> There are workarounds shown in both that bug (bug 472292) and in bug 191014.
> Please fix this.

Ah, sorry, you're right. I thought this was common to all Java packages, not this package's fault. Addressed in new version.

> I think this package is almost done. Please post a spec fixing the above and I
> will review the package.
> ===
> 
> Random comments:
> ===
> >Umm, no that's definitely not needed for review, feel free to do that yourself
> >(technically, one architecture-release combination is needed and there's no
> >clear reason why would they fail, nor indication that packager intends to
> >create branches for these).
> 
> When doing your CVS request, please create a branch for F-11 - its is
> definitely not EOL, and is hence *supported*. F-10 is no longer required as it
> will very soon be EOL.

I do not need this anywhere < Fedora 13. Common practice is that if anyone else needs this, he maintains it there. Therefore, if you need the package in F-12, F-11 or even F-10, feel free to maintain it there yourself (once it's in), but the maintenance burden is on your shoulders :) That means -- it's waste of bandwidth and builders cpu cycles from my point of view, since I don't care :)

> > > Fully standards-compliant (What standard? ISO 9001? Why do I (user) care?)
> 
> > Interoperability?
> 
> Apologies for the following rant, feel free to skip it as the current
> description is good :)

I won't pretend I knew what ISO 9001 is :)

New package:

SPEC: http://v3.sk/~lkundrak/SPECS/jgraph.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/jgraph-5.12.2.1-3.fc12.src.rpm
scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1780977

Comment 4 Jason Tibbitts 2009-11-02 20:42:10 UTC
*** Bug 252084 has been marked as a duplicate of this bug. ***

Comment 5 Jason Tibbitts 2009-11-02 20:42:28 UTC
*** Bug 472793 has been marked as a duplicate of this bug. ***

Comment 6 D Haley 2009-11-07 02:01:03 UTC
Hello,

I started reviewing this, and noticed that upstream have just undergone some changes, which I think we need to track. In particular a project called "jgraphx" (aka jgraph-6 apparently) has been released by the author. 

For now, please update to jgraph-5.13. Also we may need to package jgraphx at a later date (this is jgraph5 + namespace move + some changes?) -- it is unclear if upstream plan to maintain jgraph-5.13 as a separate library. However this (jgraphx) I believe should be handled separately (but would appreciate comments).

Unfortunately I am still getting those debuginfo errors with your latest SRPM.

Comment 7 D Haley 2009-11-07 02:01:55 UTC
Oh and note that the author has relicenced under BSD for 5.13.

Comment 8 Lubomir Rintel 2009-11-09 10:15:08 UTC
(In reply to comment #6)
> For now, please update to jgraph-5.13.

Good catch, thank you.
Done.

> Also we may need to package jgraphx at a
> later date (this is jgraph5 + namespace move + some changes?) -- it is unclear
> if upstream plan to maintain jgraph-5.13 as a separate library. However this
> (jgraphx) I believe should be handled separately (but would appreciate
> comments).

You're completely right. Given how big is the tendency of Java programmers to often completely redesign APIs and namespace hierarchies it's not uncommon (in fact it is very common) to keep older versions of packages as long as they are being depended on and package new ones with another name (see junit - junit4, saxon - saxon8 (jpackage), etc. not even bothering to follow the -compat package naming). I currently have no need to package jgraphx, but as you correctly noted that would really be handled separately.

> Unfortunately I am still getting those debuginfo errors with your latest SRPM.  

Did -debuginfo generate correctly for you? For me and the scratch build as well, it did. Please don't get confused with messages about complaints about problems finding files with dollar sign characters ("$") in their file names -- they're really not to be found, it's just the result of how does find-debuginfo determine the .java files paths from .class-es embedded in jars.

(In reply to comment #7)
> Oh and note that the author has relicenced under BSD for 5.13.  

Changed.

New package:

SPEC: http://v3.sk/~lkundrak/SPECS/jgraph.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/jgraph-5.13.0.0-1.fc12.src.rpm
scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1795473

Comment 9 D Haley 2009-11-10 10:30:21 UTC
Key:

[+] - OK
[N] - Not applicable
[!] - Attention required

[+] MUST: rpmlint must be run on every package. The output should be posted in the review.
==
$ cat tmp
Wrote: /home/makerpm/rpmbuild/RPMS/i386/jgraph-5.13.0.0-1.fc10.i386.rpm
Wrote: /home/makerpm/rpmbuild/RPMS/i386/jgraph-javadoc-5.13.0.0-1.fc10.i386.rpm
Wrote: /home/makerpm/rpmbuild/RPMS/i386/jgraph-debuginfo-5.13.0.0-1.fc10.i386.rpm

$ rpmlint `cat tmp | awk '{print $2}'`
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint jgraph.spec 
jgraph.spec:139: W: libdir-macro-in-noarch-package (main package) %attr(-,root,root) %{_libdir}/gcj/%{name}
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

$ sudo rpm -i ../RPMS/i386/jgraph-5.13.0.0-1.fc10.i386.rpm 
$ sudo rpm -i ../RPMS/i386/jgraph-javadoc-5.13.0.0-1.fc10.i386.rpm 
$ rpmlint jgraph jgraph-javadoc
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


==
rpmlint is wrong here, as discussed earlier. So OK

[+] MUST: The package must be named according to the Package Naming Guidelines .
[+] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[+] MUST: The License field in the package spec file must match the actual license. 
[+] MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English. 
[+] MUST: The spec file for the package MUST be legible. 
[+] 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.

Upstream:
$ md5sum jgraph-5.13.0.0-bsd-src.jar 
16b0e3af6c5ac3e776d9c95e9a1f54fe  jgraph-5.13.0.0-bsd-src.jar
SRPM:
16b0e3af6c5ac3e776d9c95e9a1f54fe  jgraph-latest-bsd-src.jar

OK

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. 
[N] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. 
[!] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
Am I being a bit dense, or is there a "Requires: java" missing?

[N] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[N] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
[+] MUST: Packages must NOT bundle copies of system libraries.
[N] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 
[+] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
[+] MUST: A Fedora package must not list a file more than once in the spec file's %files listings. 
[+] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
[+] MUST: Each package must consistently use macros. 
[+] MUST: The package must contain code, or permissable content. 
[+] MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
[N] MUST: Header files must be in a -devel package. 
[N] MUST: Static libraries must be in a -static package. 
[N] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). 
[N] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
[N] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
[N] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
[N] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. 
[+] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
[+] MUST: All filenames in rpm packages must be valid UTF-8. 



[N] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. 
[N] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. 
[+] SHOULD: The reviewer should test that the package builds in mock. 
	Koji:
	F11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1798071
	F12: http://koji.fedoraproject.org/koji/taskinfo?taskID=1795473
		
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures. 
	We have koji builds for F11, F12. 
[!] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
	The examples do not compile. This requires the "l2fprod-common" jar. This is in the repos, so please create a README.fedora describing how to install deps for and run the examples. You may wish to compile the examples directory with ant compile-examples during build time -- obviously you need to then add BuildRequires: l2fprod-common.

[N] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. 
[+] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. 
[N] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. 
[N] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.  


If you fix these issues, then I will approve the package.

Comment 10 D Haley 2009-11-10 12:20:09 UTC
>If you fix these issues, then I will approve the package.  
Just to clarify, I am not a sponsor. I will be happy with the review, however.

Comment 11 Lubomir Rintel 2009-11-10 14:46:34 UTC
Thanks for your time and review.

(In reply to comment #9)
> [!] MUST: All build dependencies must be listed in BuildRequires, except for
> any that are listed in the exceptions section of the Packaging Guidelines ;
> inclusion of those as BuildRequires is optional. Apply common sense.
> Am I being a bit dense, or is there a "Requires: java" missing?

You're right. In fact, I the package should depend on jpackage-utils since it owns /usr/share/java this package used. Added a dependency on jpackage-utils, it also depends on java so it's not necessary to list it twice.

> [!] SHOULD: The reviewer should test that the package functions as described. A
> package should not segfault instead of running, for example.
>  The examples do not compile. This requires the "l2fprod-common" jar. This is
> in the repos, so please create a README.fedora describing how to install deps
> for and run the examples. You may wish to compile the examples directory with
> ant compile-examples during build time -- obviously you need to then add
> BuildRequires: l2fprod-common.

I'm quite reluctant to do this since it's extra work for virtually no benefit (and just a SHOULD, not MUST). It's not a common practice either. If you insist on verifying functionality you insist on doing so I suggest you try to build something that depends on it (say, microba, see bug #532205). It would be rather uncommon for a Java library not to function once it compiles though.

(In reply to comment #10)
> >If you fix these issues, then I will approve the package.  
> Just to clarify, I am not a sponsor. I will be happy with the review, however.  

I am already sponsored (in fact, I'm a sponsor), so I don't need a sponsor to review packages. Anyone who's in the packager group (e.g. you) can review my packages (this ticket would block FE_NEEDSPONSOR if I needed a sponsor).

New package:

SPEC: http://v3.sk/~lkundrak/SPECS/jgraph.spec
SRPM: http://v3.sk/~lkundrak/SRPMS/jgraph-5.13.0.0-2.fc12.src.rpm

Comment 12 D Haley 2009-11-13 14:33:31 UTC
I've managed to get the examples to work in jgraph, with your current SRPM, so this package is APPROVED.

Comment 13 Lubomir Rintel 2009-11-13 14:47:01 UTC
Thank you! (I'm also creating F-11 and F-12 branches as you requested and will orphan them as soon as they are created. Feel free to take them in pkgdb then).

By the way, you seem to have forgotten to set the review flag to '+'. Please do so when you approve the package.

New Package CVS Request
=======================
Package Name: jgraph
Short Description: Java-based Diagram Component and Editor
Owners: lkundrak
Branches: F-11 F-12 EL-5

Comment 14 Jason Tibbitts 2009-11-13 16:23:16 UTC
Sorry, but this ticket isn't assigned to anyone, is still in NEW state, and the fedora-review flag is unset.  It doesn't look like it's quite time for CVS.

Comment 15 Jason Tibbitts 2009-11-14 16:39:46 UTC
CVS done.

Comment 16 Lubomir Rintel 2009-11-17 11:29:11 UTC
Imported and built.
Thank you for Review and CVS.
(orphaning in F-11 and F-12, feel free to pick it up)


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