Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/SystemTapGuiServer.spec SRPM URL: http://downloads.sourceforge.net/stapgui/SystemTapGuiServer-1.0-1.src.rpm Description: SystemtapGui is a tool being developed to assist in writing SystemTap scripts and viewing kernel performance.This package installs the server side of SystemtapGui
This RPM will build on different architectures. Remove the following from the spec file: BuildArch: i386 Make the rpm file name agree with the one in: https://bugzilla.redhat.com/show_bug.cgi?id=483205 Allow someone to do: yum install "*stapgui*"
Created attachment 330797 [details] Avoid coding the compiler flags in the Makefile When RPMs are built through the build system CFLAGS and CXXFLAGS are passed in. This patch removes the hardcode options. The attached patch shows the changes in the makefile. I would think that these would be folded into the source file rather than carried as a patch in the RPM. The spec file needs minor change to pass in the flags. That will be a separate attachment. When compiling with the x86-64 F10 got the following warning during the build that should be examined more closely: subscriptionMgr.cpp: In member function 'int datamanager::SubscriptionMgr::handleRequest(datamanager::dmRequest*, datamanager::dmResponse*, int)': subscriptionMgr.cpp:130: warning: 'rc' may be used uninitialized in this function datamanager.cpp: In member function 'void datamanager::DataManager::destroyStapProcess(datamanager::stap_process_t*)': datamanager.cpp:413: warning: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result datamanager.cpp: In member function 'void datamanager::DataManager::execStap(datamanager::dmRequest*)': datamanager.cpp:799: warning: ignoring return value of 'int system(const char*)', declared with attribute warn_unused_result
Created attachment 330798 [details] Revised spec file to use compiler options passed in by rpmbuild This spec file makes use of the CFLAGS and CXXFLAGS passed into the rpmbuild. This should make the rpmbuild a bit more flexible as the compiler options may be changed in the builds, e.g. stricter checking of code syntax.
(In reply to comment #1) > This RPM will build on different architectures. Remove the following from the > spec file: > > BuildArch: i386 > > Make the rpm file name agree with the one in: > > https://bugzilla.redhat.com/show_bug.cgi?id=483205 > > Allow someone to do: > > yum install "*stapgui*" Thanks Will. I will rename the rpm to systemtapguiserver (removing camel casing) and will be renaming the client (bug 483205) to eclipse-systemtapgui. Will post the new package with all changes mentioned above shortly.
Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec SRPM URL: http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-2.src.rpm The package includes changes to the Makefile and spec as per Will's comments. The warnings have been resolved.
Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec SRPM URL: http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-3.src.rpm Added license text and README
I don't think it's a good idea to have INSTALL_DIR = /usr/sbin/stapgui-server hard-coded in the makefile. Do it in the specfile instead.
(In reply to comment #7) > I don't think it's a good idea to have > > INSTALL_DIR = /usr/sbin/stapgui-server > > hard-coded in the makefile. Do it in the specfile instead. This was actually redundant, removed it from the makefile. Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec SRPM URL: http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-4.src.rpm
The packaging for systemtapguiserver is looking okay to me. A couple minor changes: - instead of $RPM_BUILD_ROOT use %{buildroot} -Maybe make the %description more like to describe what this package is: Server for Eclipse SystemTap GUI plugin. This eclipse plugin assists writing SystemTap scripts and viewing kernel performance.
William, if you'll take the time to read carefully https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS, you will certainly notice that there is no reason to replace $RPM_BUILD_ROOT with %{buildroot} or viceversa. The only rule is to not mix them in the same spec. And to be honest, the %description from the spec looks more clear to me than the version suggested in comment #9. I especially like this part: "This package installs the server side of systemtapgui". Part which should end with a dot (hint, hint!).
Updated spec and src rpm: Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec SRPM URL: http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-5.src.rpm
Re-read the packaging guidelines and used Bug 483205 (review of eclipse-systemtapgui) comment #23 to do a more systematic review. Found a couple minor things: X %description, minor typo, "client.It" Should be a space in there X License: EPL but FSF GPLv2 based COPYING and INSTALL files in root of build. X Empty AUTHORS, NEWS, README, and ChangeLog files in root of build directory * 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 Just starting to look over the internals of systemtapgui-server code. In SubscriptionMgr::SubscriptionMgr() is the "sleep(1);" necessary?
Thanks Will, Updated spec and src rpm: Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec SRPM URL: http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-6.src.rpm I've removed the empty files, and INSTALL and COPYING. They were created cos i had used automake to create makefile.in, I agree the sleep(1) doesn't look too good, its a stop gap solution , added it after there were some sync issues.
Started playing with the systemtapgui-server (and finally got it to work). The instructions are not clear but the current code needs to be run as root. It then figures out user from packet information and su to that user. It would be much better if people could run it as normal user for the case where the eclipse and the server are run by the same user. There are a couple questionable cases in datamanager.cpp:DataManager::execStap(): case (SHELL): allows executing arbitary script (as root this seems like a bad idea). what would prevent someone from using this to just connect and run arbitrary commands. cases (BLUEDYE): mentions a package (Bluedye) that doesn't appear to be available in fedora Why scp the file to the server machine? why not send it to the stapgui-server with the command and run with stap -e 'script...'? Currently, the plugin stores the password in plantext in a possibly world readable file. Also the current checks in the plugin do not seem to notice if the transfer failed (due to missing password). The compile server does compile code, but it doesn't perform the other aspects of systemtapgui server such as execute the script and collect stdout/stderr. Could systemtapgui be stripped down just to use staprun to run a compiled script? Make it possible to run systemtap scripts on stripped down machines. This would be useful for cases of running code on compute nodes in a cluster.
(In reply to comment #14) > It would be > much better if people could run it as normal user for the case where the > eclipse and the server are run by the same user. I agree that users could be apprehensive about running the app as root and this is one of the issues that we are looking to fix soon. eclipse and the server in most cases may not be run by the same user as the server could be on a different machine. The server minimally needs to be run as root or users of group stapdev/stapusr to be able to run systemtap scripts, although the current code mandates that the server is run as root. > There are a couple questionable cases in > datamanager.cpp:DataManager::execStap(): > > case (SHELL): allows executing arbitary script (as root this seems like a bad > idea). what would prevent someone from using this to just connect and run > arbitrary commands. > > cases (BLUEDYE): mentions a package (Bluedye) that doesn't appear to be > available in fedora The server was designed to be able to run any script(not just systemtap) and hence the above cases. This part of the code is there for future expansion. Currently these cases are never true and so there is no chance of arbitrary commands being run. > > Why scp the file to the server machine? why not send it to the stapgui-server > with the command and run with stap -e 'script...'? Currently, the plugin stores I had some problems with protocol management due to the variable length of the script. It might be better to transfer as part of the command through the socket connection and remove one step from the setup. Will redesign in subsequent releases. > the password in plantext in a possibly world readable file. Also the current > checks in the plugin do not seem to notice if the transfer failed (due to > missing password). > You should have got an error message saying "File transferred failed". Will look into it > The compile server does compile code, but it doesn't perform the other aspects > of systemtapgui server such as execute the script and collect stdout/stderr. > Could systemtapgui be stripped down just to use staprun to run a compiled > script? Make it possible to run systemtap scripts on stripped down machines. > This would be useful for cases of running code on compute nodes in a cluster. This is one feature we are looking at in future releases of the client. Possibly by the end of the year. We are exploring the option of using the compile-server for compilation, and systemtapguiserver for execution. Thanks for the review. Can we treat the code changes/bug fixes as upstream issues so that this review is not blocked?.
What's the status of this? Seems like it's being reviewed, but not flags or assignee is set.
(In reply to comment #16) > What's the status of this? Seems like it's being reviewed, but not flags or > assignee is set. The request is still waiting for a formal review and approval. It has not been assigned to anybody.
- compiler flags used correctly - spec file clean, legible, using American english - filelist sane - builds fine in mock 1.) Don't override reqprov generator please AutoReqProv: no 2.) This is useless, coreutils is in the build group (see "koji list-groups dist-f11-build") BuildRequires: coreutils 3.) Don't strip the binary Debuginfo generator will do that strip stapgui-server 4.) Please use %{_smp_mflags} make flags make 5.) You don't need to redundantly set 0755 mode 0755 is install's default anyways Moreover, you have a duplicate entry in filelist install -m0755 stapgui-server ${RPM_BUILD_ROOT}/%{_bindir}/stapgui-server ... %{_bindir}/* ... %attr(0755,root,root) %{_bindir}/stapgui-server 6.) RPMlint whines Both problems failry simple to fix Source package: systemtapguiserver.src: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 2) 1 packages and 0 specfiles checked; 0 errors, 1 warnings. And lots of these for debuginfo: systemtapguiserver-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/systemtapguiserver-1.0/logger.cpp The file is installed with executable permissions, but was identified as one that probably should not be executable. Verify if the executable bits are desired, and remove if not.
Spec URL: http://nchc.dl.sourceforge.net/sourceforge/stapgui/systemtapguiserver.spec SRPM URL: http://downloads.sourceforge.net/stapgui/systemtapguiserver-1.0-7.src.rpm I had always had debuginfo disabled so have never verified it with rpmlint before :(. rpmlint output: [root@localhost SOURCES]# rpmlint -i systemtapguiserver.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. [root@localhost SOURCES]# rpmlint -i /usr/src/redhat/RPMS/i586/systemtapguiserver-1.0-7.i586.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [root@localhost SOURCES]# rpmlint -i /usr/src/redhat/RPMS/i586/systemtapguiserver-debuginfo-1.0-7.i586.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [root@localhost SOURCES]# rpmlint -i /usr/src/redhat/SRPMS/systemtapguiserver-1.0-7.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1300001
* Wed Apr 15 2009 Anithra P Janakiraman <anithra.ibm.com> 1.0-7 - Changes to spec file. Anithra, this is the extreme example of poorly written changelog entry. When writing change log entries, describe what you changed, such as: - Don't strip binary anymore - Correct the file modes ... Anyways, not anything that would block the review. - compiler flags used correctly - spec file clean, legible, using American english - filelist sane - builds fine in mock - requires fine - provides fine - builds in mock - rpmlint happy The package is APPROVED
(In reply to comment #20) > Anithra, this is the extreme example of poorly written changelog entry. When > writing change log entries, describe what you changed, such as: > > - Don't strip binary anymore > - Correct the file modes Point taken!. will do. > > The package is > > APPROVED Thanks Lubomir!
New Package CVS Request ======================= Package Name: systemtapguiserver Short Description: Server for the eclipse-systemtapgui client Owners: anithra Branches: F-10 F-11 InitialCC:
cvs done.
systemtapguiserver-1.0-8.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/systemtapguiserver-1.0-8.fc11
systemtapguiserver-1.0-8.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/systemtapguiserver-1.0-8.fc10
systemtapguiserver-1.0-8.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report.
systemtapguiserver-1.0-8.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report.