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.
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
Is the camel casing necessary in the name? Just curious ...
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.
(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
(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.
(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
This is my first package to Fedora and I need a sponsor
(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
(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.
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 :)
I will sponsor Anithra. I will also review this package sometime before Friday.
(In reply to comment #11) > I will sponsor Anithra. I will also review this package sometime before > Friday. Thanks Andrew.
(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?.
(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.
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.
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!
(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?.
(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.
Anithra: on the website there is a screenshot showing graphing. Is that functionality included here? Does it use Draw2D, GEF, BIRT, or something else?
(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
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
(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.}.
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
(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
(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
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!
Thanks Andrew.
New Package CVS Request ======================= Package Name: eclipse-systemtapgui Short Description: GUI interface for SystemTap Owners: anithra Branches: F-9 F-10 InitialCC:
cvs done.
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?
Sure, they should be cleaned up, but that's an upstream thing IMO :) Yes, they can be safely ignored in the meantime.
Hi Anithra, Will you close the bug as the plugin is in repos? Thanks, Alexander Kurtakov