Bug 483205 - Review Request: eclipse-systemtapgui - GUI interface for SystemTap
Summary: Review Request: eclipse-systemtapgui - GUI interface for SystemTap
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:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-01-30 07:26 UTC by Anithra
Modified: 2009-04-06 17:05 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Enhancement
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-02-26 14:28:54 UTC
Type: ---
Embargoed:
overholt: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Anithra 2009-01-30 07:26:49 UTC
Spec URL: https://sourceforge.net/project/downloading.php?group_id=169234&use_mirror=&filename=eclipse-SystemTapGui.spec&51805119
SRPM URL: https://sourceforge.net/project/downloading.php?group_id=169234&use_mirror=&filename=eclipse-SystemTapGui-1.0-1.fc10.src.rpm&66576057
Description: This package contains eclipse plugins which provide an IDE and a visualization perspective for the systemtap scripting language. This requires the SystemTapGui server to be installed in order to run the SystemTap scripts.

Comment 1 Parag AN(पराग) 2009-01-30 08:33:02 UTC
I have not done any java or eclipse related package review yet but let me try
this package for preliminary review.

1) you should use links for sourceforge as
http://nchc.dl.sourceforge.net/sourceforge/stapgui/eclipse-SystemTapGui.spec
http://downloads.sourceforge.net/stapgui/eclipse-SystemTapGui-1.0-1.fc10.src.rpm

2) Source0 should be
Source0:
http://downloads.sourceforge.net/stapgui/org.eclipse.SystemTapGui.src.tar.gz

3) From
http://fedoraproject.org/wiki/Packaging/EclipsePlugins#Specfile_Template, you
should have %install as

%install
rm -rf $RPM_BUILD_ROOT
install -d -m 755 $RPM_BUILD_ROOT%{_datadir}/eclipse/dropins/SystemTapGui
unzip -q -d $RPM_BUILD_ROOT%{_datadir}/eclipse/dropins/SystemTapGui \
build/rpmBuild/org.eclipse.SystemTapGui.systemtap.feature.zip


3) From
http://fedoraproject.org/wiki/Packaging/EclipsePlugins#Specfile_Template, you
should have %files as
%files
%defattr(-,root,root,-)
%{_datadir}/eclipse/dropins/SystemTapGui

See package built successfully with above changes
http://koji.fedoraproject.org/koji/taskinfo?taskID=1092915

Comment 2 Andrew Overholt 2009-01-30 16:12:53 UTC
Is the camel casing necessary in the name?  Just curious ...

Comment 3 Andrew Overholt 2009-01-30 16:23:55 UTC
Also, there's something weird happening with the feature not being included in the zips resulting from the build.  I suspect something odd in the feature's build.properties or feature.xml.  The "os=linux" thing may be it ... try a build without it and see if the feature is included in the zip.

Comment 4 Anithra 2009-01-31 11:39:18 UTC
(In reply to comment #1)
Thanks for the build results and review.

I've uploaded the spec file and SRPM with all changes to SourceForge
SPEC URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/eclipse-SystemTapGui.spec
SRPM URL: http://downloads.sourceforge.net/stapgui/eclipse-SystemTapGui-1.0-2.fc10.src.rpm

Comment 5 Anithra 2009-01-31 11:41:24 UTC
(In reply to comment #2)
> Is the camel casing necessary in the name?  Just curious ...

Its not really necessary, but is there just for the sake of uniformity as SystemTap also has camel casing.

Comment 6 Anithra 2009-01-31 11:45:19 UTC
(In reply to comment #3)
> Also, there's something weird happening with the feature not being included in
> the zips resulting from the build.  I suspect something odd in the feature's
> build.properties or feature.xml.  The "os=linux" thing may be it ... try a
> build without it and see if the feature is included in the zip.

removing the "os=linux" solved the problem. Thanks!. The modified source tar ball has been uploaded to sourceforge. 
SRPM URL:
http://downloads.sourceforge.net/stapgui/eclipse-SystemTapGui-1.0-2.fc10.src.rpm

Comment 7 Anithra 2009-01-31 14:36:30 UTC
This is my first package to Fedora and I need a sponsor

Comment 8 Andrew Overholt 2009-02-02 14:23:52 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > Is the camel casing necessary in the name?  Just curious ...
> 
> Its not really necessary, but is there just for the sake of uniformity as
> SystemTap also has camel casing.

$ rpm -q systemtap
systemtap-0.8-1.fc10.x86_64

Comment 9 Anithra 2009-02-03 16:27:16 UTC
(In reply to comment #8)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > Is the camel casing necessary in the name?  Just curious ...
> > 
> > Its not really necessary, but is there just for the sake of uniformity as
> > SystemTap also has camel casing.
> 
> $ rpm -q systemtap
> systemtap-0.8-1.fc10.x86_64

Point taken. Should i be changing it to systemtapgui?. We have been using the camel casing in the sourceforge rcp packages and ive just continued it here. I can change it if needed.

Comment 10 Andrew Overholt 2009-02-03 19:02:55 UTC
I like systemtapgui 'cause it's consistent with the rest of the Eclipse packages to have all lower case but I won't raise a stink since it's not in the guidelines :)

Comment 11 Andrew Overholt 2009-02-04 15:47:12 UTC
I will sponsor Anithra.  I will also review this package sometime before Friday.

Comment 12 Anithra 2009-02-04 18:41:27 UTC
(In reply to comment #11)
> I will sponsor Anithra.  I will also review this package sometime before
> Friday.

Thanks Andrew.

Comment 13 Anithra 2009-02-05 13:38:06 UTC
(In reply to comment #10)
> I like systemtapgui 'cause it's consistent with the rest of the Eclipse
> packages to have all lower case but I won't raise a stink since it's not in the
> guidelines :)

changed the package name to lowercase

SPEC URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/eclipse-systemtapgui.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/eclipse-systemtapgui-1.0-3.fc10.src.rpm

Should I be including a shell script for fetching the source?.

Comment 14 Andrew Overholt 2009-02-05 14:44:47 UTC
(In reply to comment #13)
> (In reply to comment #10)
> > I like systemtapgui 'cause it's consistent with the rest of the Eclipse
> > packages to have all lower case but I won't raise a stink since it's not in the
> > guidelines :)
> 
> changed the package name to lowercase

Thanks.

> Should I be including a shell script for fetching the source?.

Yes, please do.

Comment 15 Andrew Overholt 2009-02-05 14:45:54 UTC
Oh(In reply to comment #14)
> (In reply to comment #13)
> > Should I be including a shell script for fetching the source?.
> 
> Yes, please do.

Oh, I didn't realize you made a source tarball available upstream.  That is preferable and you don't need a shell script in this case.

Comment 16 Andrew Overholt 2009-02-05 15:02:38 UTC
The package looks good.  There are just a few small things that need to be taken care of:

- the summary should be more generic.  Something like:  "Eclipse plugins for SystemTap"
- the description expand upon the summary.  Something like:   "Eclipse plugins providing IDE integration and visualization tools for SystemTap"
- you need a 1: epoch prefix for the BR: java-devel.  It should be >= 1:1.5.0
- this is an upstream issue so it doesn't really affect this review but I don't think the namespace and source tarball should be named "org.eclipse..." if it's not hosted at eclipse.org
- add a line in %files for marking the license as documentation:
%doc org.eclipse.systemtapgui.systemtap.feature/epl-v10.html
- it seems odd to me that there's no BuildRequires on jsch but there is a Requires: jsch
- looking in the error log, I see that there are some issues with the plugins' key bindings.  These should be fixed but are upstream issues and won't block this review.
- it would be nice if we could fix the NPEs that happen when double-clicking on a probe point in the Probe Alias view:

java.lang.NullPointerException
	at org.eclipse.systemtapgui.generic.editor.actions.file.NewFileAction.queryFile(NewFileAction.java:28)
	at org.eclipse.systemtapgui.generic.editor.actions.file.OpenFileAction.run(OpenFileAction.java:50)
	at org.eclipse.systemtapgui.systemtap.ide.actions.hidden.ProbeAliasAction.run(ProbeAliasAction.java:97)
	at org.eclipse.systemtapgui.systemtap.ide.views.ProbeAliasBrowserView$1.doubleClick(ProbeAliasBrowserView.java:71)
	at [...]

Aside from these issues, everything looks good from a packaging standpoint.  It won't take much to get this into shape for acceptance into Fedora.  Thanks!

Comment 17 Anithra 2009-02-06 21:20:45 UTC
(In reply to comment #16)

Thanks Andrew. I was able to resolve the NPE and key binding problems and have renamed the package to com.ibm.systemtapgui.* 

But adding a 1: epoch prefix for java-devel throws a dependency error when i try to build the package. I use java-gcj. Should i be moving to some other java?.

Comment 18 Andrew Overholt 2009-02-06 21:31:02 UTC
(In reply to comment #17)
> (In reply to comment #16)
> 
> Thanks Andrew. I was able to resolve the NPE and key binding problems and have
> renamed the package to com.ibm.systemtapgui.* 

Cool.

> But adding a 1: epoch prefix for java-devel throws a dependency error when i
> try to build the package. I use java-gcj. Should i be moving to some other
> java?.

java-1.5.0-gcj-compat-devel should Provide: java-devel = 1:1.5.0.  Gah, it looks like it doesn't.  Okay, remove the epoch and hopefully people using OpenJDK won't end up with gcj if they don't already have it.

Comment 19 Andrew Overholt 2009-02-06 21:33:28 UTC
Anithra:  on the website there is a screenshot showing graphing.  Is that functionality included here?  Does it use Draw2D, GEF, BIRT, or something else?

Comment 20 Anithra 2009-02-07 10:52:08 UTC
(In reply to comment #19)
> Anithra:  on the website there is a screenshot showing graphing.  Is that
> functionality included here?  Does it use Draw2D, GEF, BIRT, or something else?

graphing functionality is included, it uses eclipse-swt, not sure if that needs to be included in BuildRequires , added it anyway. 

SPEC URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/eclipse-systemtapgui.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/eclipse-systemtapgui-1.0-4.fc10.src.rpm

Comment 21 Anithra 2009-02-07 10:54:38 UTC
The above rpm , spec file contains all the other changes as well. 

Repeating the link:

SPEC URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/eclipse-systemtapgui.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/eclipse-systemtapgui-1.0-4.fc10.src.rpm

Comment 22 Andrew Overholt 2009-02-09 14:21:59 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > Anithra:  on the website there is a screenshot showing graphing.  Is that
> > functionality included here?  Does it use Draw2D, GEF, BIRT, or something else?
> 
> graphing functionality is included, it uses eclipse-swt

Okay, I wouldn't have guessed that, but cool :)

> not sure if that needs
> to be included in BuildRequires , added it anyway. 

It's not because it'll be brought in by eclipse-{platform,pde,jdt,etc.}.

Comment 23 Andrew Overholt 2009-02-09 14:53:33 UTC
Okay, here's the review.  Just 3 small things and we'll be good to go (lines beginning with non-* need attention; others are fine and just listed for brevity).  Thanks!

X BuildRequires are proper
 - do we need a BR/R on SystemTap itself?  What about kernel-devel?
X make sure lines are <= 80 characters
 - please wrap line 29 (the pdebuild call) with '\' characters
? macros fine
 - it would be nice if you s/$RPM_BUILD_ROOT/%{buildroot}/ but it's not the
   end of the world

* package is named appropriately
* it is legal for Fedora to distribute this
* license field matches the actual license.
* license is open source-compatible.
* specfile name matches %{name}
* md5sum matches upstream
* skim the summary and description for typos, etc.
* correct buildroot
* %{?dist} used correctly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
* rpmlint on <this package>.srpm gives no output
* changelog format okay
* Summary tag does not end in a period 
* no PreReq
* specfile is legible
* package successfully compiles and builds on x86_64 (but is correctly noarch)
* summary and description fine
* specfile written in American English
* no -doc sub-package necessary
* not native, so no rpath, static linking, etc.
* no config files
* not a GUI app
* no -devel necessary
* install section begins with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no translations so no locale handling
* no Requires(pre,post) 
* package not relocatable
* package contains code
* package owns all directories and files
* no %files duplicates
* file permissions fine
* %clean present
* %doc files do not affect runtime
* not a web app
* verify the final provides and requires of the binary RPMs
  - these look good to me
* run rpmlint on the binary RPMs => no output
* package includes license text in the package and marks it with %doc

Comment 24 Anithra 2009-02-10 09:25:58 UTC
(In reply to comment #23)
> X BuildRequires are proper
>  - do we need a BR/R on SystemTap itself?  What about kernel-devel?
We wont need it as this can run on machines without systemtap(even on windows.). The probes and functions wont be listed in the IDE but apart from that it should work fine as the script is sent to the server for execution.
> X make sure lines are <= 80 characters
>  - please wrap line 29 (the pdebuild call) with '\' characters
done.
> ? macros fine
>  - it would be nice if you s/$RPM_BUILD_ROOT/%{buildroot}/ but it's not the
>    end of the world
done.

SPEC URL:
http://nchc.dl.sourceforge.net/sourceforge/stapgui/eclipse-systemtapgui.spec
SRPM URL:
http://downloads.sourceforge.net/stapgui/eclipse-systemtapgui-1.0-5.fc11.src.rpm

Comment 25 Anithra 2009-02-10 10:26:49 UTC
(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Anithra:  on the website there is a screenshot showing graphing.  Is that
> > > functionality included here?  Does it use Draw2D, GEF, BIRT, or something else?
> > 
> > graphing functionality is included, it uses eclipse-swt
> 
> Okay, I wouldn't have guessed that, but cool :)

:). Planning to replace with something more sophisticated in the next release. Will discuss this by mail

Comment 26 Andrew Overholt 2009-02-10 15:41:27 UTC
Thanks for the changes.  It looks good.  This package is approved.

Now you need to apply for sponsorship (I'll sponsor you -- see https://fedoraproject.org/wiki/PackageMaintainers/Join) and do the fedora-cvs stuff (see https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure). Then you'll need to close this bug as per https://fedoraproject.org/wiki/Package_Review_Process .  Thanks, Anithra!

Comment 27 Anithra 2009-02-11 10:54:31 UTC
Thanks Andrew.

Comment 28 Anithra 2009-02-12 18:14:35 UTC
New Package CVS Request
=======================
Package Name: eclipse-systemtapgui
Short Description: GUI interface for SystemTap
Owners: anithra
Branches: F-9 F-10
InitialCC:

Comment 29 Kevin Fenzi 2009-02-13 06:55:12 UTC
cvs done.

Comment 30 William Cohen 2009-02-17 15:44:16 UTC
I built the package locally on F-10 and F-11. I looked through the output of the build and saw a number of messages about generic types should be parameterized and "Discouraged access" like the following examples:
 
    [javac] ----------
    [javac] 1. WARNING in /home/wcohen/rh-rpms/BUILD/eclipse-systemtapgui-1.0/bu
ild/plugins/com.ibm.systemtapgui.generic.editor/src/com/ibm/systemtapgui/generic
/editor/ColorManager.java (at line 29)
    [javac] 	Iterator e = fColorTable.values().iterator();
    [javac] 	^^^^^^^^
    [javac] Iterator is a raw type. References to generic type Iterator<E> shoul
d be parameterized


    [javac] ----------
    [javac] 13. WARNING in /home/wcohen/rh-rpms/BUILD/eclipse-systemtapgui-1.0/b
uild/plugins/com.ibm.systemtapgui.generic.consolelog/src/com/ibm/systemtapgui/ge
neric/consolelog/actions/ConsoleAction.java (at line 77)
    [javac] 	private boolean isRunning(ConsoleView cv) {
    [javac] 	                          ^^^^^^^^^^^
    [javac] Discouraged access: The type ConsoleView is not accessible due to re
striction on classpath entry /home/wcohen/rh-rpms/BUILD/eclipse-systemtapgui-1.0
/build/SDK/plugins/org.eclipse.ui.console_3.3.0.v20080529-1300.jar

Are these warnings something that should be cleaned up? Or can they be safely ignored?

Comment 31 Andrew Overholt 2009-02-17 16:32:08 UTC
Sure, they should be cleaned up, but that's an upstream thing IMO :)  Yes, they can be safely ignored in the meantime.

Comment 32 Alexander Kurtakov 2009-02-26 10:20:41 UTC
Hi Anithra,
Will you close the bug as the plugin is in repos?

Thanks,
Alexander Kurtakov


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