Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=513896 --- Comment #8 from Mark Goodwin <mgoodwin@xxxxxxxxxx> 2009-08-03 21:01:33 EDT --- (In reply to comment #5 and comment #6) > Ok a few more comments, sorry for being iterative. thanks for the review/update Eric, much appreciated, sorry I didn't get back to this yesterday .. those pesky customers again :) > > Naming the spec "pcp_fedora.spec" threw me off, not sure if it's kosher or not. I just checked it into (my) pcp git tree, mainly for safe-keeping, but also for easier future reconcilliation with the upstream spec, which is now quite different. I think it would also be reasonable to check-in the SLES spec as well, since it's different too. > Keeping a fedora spec upstream probably just makes it harder to keep things in > sync; fedora cvs should be pretty capable of keeping track of the fedora pcp > specfile. yeah, once it's reconciled and checked into cvs, then maybe it can be removed from the tree (or that commit can be revoked). > %if %{have_ibdev} > %define ib_prereqs libibmad libibumad libibcommon > %define ib_build_prereqs %{ib_prereqs} libibmad-devel libibumad-devel > libibcommon-devel > %endif > > %ifarch ia64 > Requires: libunwind > %endif > > I think the ib_prereqs can be completely dropped; they're all libraries, and if > configured to use it (?) rpm will work that out. So I'd just drop > %{_ib_prereqs} altogether. yes they're all libraries, but no they can't be dropped. When Max wrote the infiniband PMDA, he needed to change the IB libs. Those IB changes are upstream now, but not (yet) available in any standard RH or FC build AFAIK. So the build and run-time prereqs will need versioning added .. which I'll add once I figure out what it is :) For now, %{have_ibdev} should stay and be false. FWIW, having IB monitoring is important for most HPC users, so we really do want to revisit this. > Same goes for the libunwind stuff on ia64 most likely? Or is it explicitly > needed for some reason? If so I'd add a comment. I'd add a comment if I could remember the context for needing the prereq. Another one lost in the depths of antiquity, probably an SGI-only thing? Maybe we can drop it, run qa and see what breaks on ia64 > BuildRequires: gcc-c++ libstdc++-devel procps autoconf bison flex ncurses-devel > %{?ib_build_prereqs} > > as per: https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 > > There's no need for gcc-c++ or libstdc++-devel (as gcc-c++ requires this) yep ok thanks > However, doing a clean build fails due to missing BuildReqs: > > checking if ExtUtils::MakeMaker is installed... no > FATAL ERROR: Perl ExtUtils::MakeMaker module missing. > > so add: > > BuildRequires: perl-ExtUtils-MakeMaker ok thanks, added. > Ok and some comments on some of the warnings & errors > > > * pcp-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpcp_pmda.so.3 > > Nothing can really be done about this - the exit() context is well > > known and understood. > > By the pcp folks I guess? It's expected that the library exits, not the > application? yes that's correct. the error is basically fatal for a daemon PMDA and is a known way for a daemon PMDA to terminate. (and the exit() is not in the code-path for a DSO PMDA). > > * pcp.x86_64: E: arch-dependent-file-in-usr-share > > Several demo binaries are being installed in /usr/share/pcp/demo. > > I can move these elsewhere, if requested. These binaries are not > > used by the PCP core functionality. > > Maybe don't build but just ship the c files? As a hack, if you don't want to > change the upstream build, you could rm them post-%install. A better longer term plan would be separate packages for PMDAs, which could also have -devel variants too. > > * pcp.x86_64: W: devel-file-in-non-devel-package > > Several headers and some 'C' files in PCP are actually configuration > > files. Discussed with the PCP community and they agree to leave these > > as-is. > > Maybe a comment in the %files section would help the next reviewer :) yep ok, added the following : # Note: there are some headers (e.g. domain.h) and in a few cases some # C source files, that match the following pattern. rpmlint complains # about this, but note: these are not devel files, but rather they are # (slightly obscure) PMDA config files. %{_localstatedir}/lib/pcp/pmdas/*/* > > * pcp.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/pcp > > Well, it actually isn't a config file - we want it unconditionally > > updated after an upgrade. Same for /etc/pcp.env > > hah, well: > > # rpmlint rpmlint > rpmlint.noarch: W: non-conffile-in-etc /etc/bash_completion.d/rpmlint > > so I suppose it'll be hard to argue too much :) yep, so we'll ignore that one :) Note that rpmlint also installs %dir /etc/bash_completion.d :) > > But I think if you mark it as %config, it will still be unconditionally updated > on an upgrade, but if it's edited, the edited one will go to .rpmsave. ok I've marked it %config, can't hurt > > * pcp.x86_64: W: log-files-without-logrotate /var/log/pcp > > PCP has it's own log management system, see pmlogger_daily(1) > > Just to be a pain; is that necessary? If logrotate is available, can it not be > used so as not to surprise the admin? Or is there a requirement for a special > homegrown tool? the special homegrown tool is integral to the whole PCP logging infrastructure. I can't change that without major architectural changes ... which I'm not willing to undertake at this point! > > * pcp.x86_64: W: dangerous-command-in-%post chmod > > * pcp.x86_64: W: dangerous-command-in-%preun rm > > This could be avoided with %ghost directive, if needed. > > just out of curiosity are the chmod's actually needed? they're there for safety, I think because we weren't sure what umask is prevailing > > Looking at the scriptlets, I have to say that the .rpmsave & .rpmnew > manipulation kinda bugs me. Those are supposed to be left for the admin to > deal with. If it really has to be there can you add a comment? well that stuff was put there many moons ago because the default action on an upgrade were NOT what (SGI) customers wanted - they complained. I'll remove them for Fedora, for now, but they might come back oneday. > Also just thinking aloud; the scripts source /etc/pcp.env but that's not > expected to change, right; would it make any more sense to drop that and just > use the same rpm path macros as you installed to? /etc/pcp.conf sources /etc/pcp.conf, which may or may not get edited after install. That breaks the consistency of the whole rpm macro / /etc/pcp.conf variable sync-up. I don't know what to do about that. For now, yes let's got for the simple case and remove the crud. > > IOW could you just do: > > %preun > if [ "$1" -eq 0 ] > then > # > # Stop daemons before erasing the package > # > /sbin/service pcp stop >/dev/null 2>&1 > /sbin/service pmie stop >/dev/null 2>&1 > /sbin/service pmproxy stop >/dev/null 2>&1 > > /sbin/chkconfig --del pcp >/dev/null 2>&1 > /sbin/chkconfig --del pmie >/dev/null 2>&1 > /sbin/chkconfig --del pmproxy >/dev/null 2>&1 > > rm -f %{_datadir}/pcp/lib/.NeedRebuild > fi > exit 0 > > and similar for the creation of the .NeedRebuild stuff. It just seems simpler > to me, but just a suggestion. ok, let's go simple and standard, for now. The crud might come back oneday though ... > > Also on the root.saved stuff - I can't find anything in scripts or source that > creates this. I see root.prev though. I'll dig further for this one .. antiquity again. I think it has something to do with the XFS NULL files bug, and the need to preserve a safe copy of a critically important PCP config file. New spec copied up to same place on OSS in a few mins (and checked into my git tree too). I'll attach the new version to this BZ too. Cheers and thanks -- Mark -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review