Bug 513896 - Review Request: pcp - performance monitoring and collection service
Review Request: pcp - performance monitoring and collection service
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jarod Wilson
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-26 21:07 EDT by Mark Goodwin
Modified: 2014-06-09 14:45 EDT (History)
7 users (show)

See Also:
Fixed In Version: pcp-3.6.3-1.el5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-05 19:36:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jarod: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)
modified specfile (6.06 KB, text/plain)
2009-07-31 12:55 EDT, Eric Sandeen
no flags Details
pcp.spec for final Fedora review, as in the pcp-3.0.0-9 community release (7.33 KB, text/plain)
2009-10-11 20:31 EDT, Mark Goodwin
no flags Details
updated pcp spec for Fedora, matching pcp-3.0.1-2 community release (7.59 KB, application/octet-stream)
2009-11-06 00:21 EST, Mark Goodwin
jarod: review+
Details

  None (edit)
Description Mark Goodwin 2009-07-26 21:07:11 EDT
Spec URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp.spec
SRPM URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-1.fc10.src.rpm

Description:

Performance Co-Pilot (PCP) provides a framework and services to support system-level performance monitoring and management. It presents a unifying abstraction for all of the performance data in a system, and many tools for interrogating, retrieving and processing that data.

PCP is a feature-rich, mature, extensible, cross-platform toolkit supporting both live and retrospective analysis. The distributed PCP architecture makes
it especially useful for those seeking centralized monitoring of distributed processing. PCP is an ideal "drop-in" replacement for sysstat/sar for those engaged in supporting enterprise level production computing environments.
For more details, documentation and white papers, see the PCP home page at http://oss.sgi.com/projects/pcp

The associated GUI package (pcp-gui) exists in a separate tree and will be submitted in a separate review request.
Comment 1 Mark Goodwin 2009-07-26 21:31:55 EDT
Setting the "seeking a sponsor" tags.
Comment 2 Eric Sandeen 2009-07-27 15:08:41 EDT
Mark, a few random questions & comments, more may follow.  :)

What is the reason for the Provides: on the libraries?  Generally RPM sorts out library provides on its own.  If there's something special here it could use a comment.

What's the reason for all the %attr specifications in the file list?  Does "make install" not put the proper permissions on them?  Assuming "rpmdiff" shows permissions differences, there would appear to be no difference and I'd just lose every explicit %attr set.

also:

%pre
exit 0

the above does nothing and should be removed.

We probably should just not package the static libs unless there's a real reason for it - is there?  If not, could just rm -f them in %install.

./configure can/should be replaced with %configure, although this seems to break the build.  :(  I'd have to look and see what the difference is.

Lots of explicit paths can (and I think should?) be referred to w/ macros:

/usr/lib* -> %{_libdir}
/usr/share/man/man3 -> %{_mandir}/man3 (etc)
/usr/bin -> %{_bindir}
/usr/include -> %{_includedir}

In a lot of cases, it may be a lot simpler to use wildcards rather than listing each file & subdir individually.  For example:

%dir /foo/bar/baz
/foo/bar/baz/*

But if 2 subpackages share files in a subdir they'd need to be explicitly listed I guess, and only one should own the dir of course.  I guess it's your preference but wow, that is a long %files list ;)

When I build I get some odd messages:

cpio: pcp-3.0.0/src/dbpmda/src/<stdout>: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/dbpmda/src/gram.tab.c: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmie/src/grammar.tab.c: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmlc/<stdout>: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmlc/gram.tab.c: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmlogextract/<stdout>: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmlogextract/gram.tab.c: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmlogger/<stdout>: Cannot stat: No such file or directory
cpio: pcp-3.0.0/src/pmlogger/gram.tab.c: Cannot stat: No such file or directory

I think these are generated files, and the debuginfo collection step isn't able to find them.  This may mean that the debuginfo rpms are incomplete?

You might be able to simplify the README installation etc (DOCFILES) by dropping their install from the makefiles, and just doing:

%files
%doc README COPYING FOO BAR

I'm not sure if hand-recreating what %doc does has any pitfalls or not.
Comment 3 Eric Sandeen 2009-07-27 15:38:28 EDT
I think a lot of the DIST_ROOT DIST_MANIFEST stuff can be simplified. Also is there any real reason to be using /usr/bin/gmake instead of just calling "make?"

 %clean
-[ ! -z "$DIST_ROOT" ] && rm -rf $DIST_ROOT
 rm -Rf $RPM_BUILD_ROOT
 
 %build
-/usr/bin/gmake default_pcp
+make default_pcp
 
 %install
 rm -Rf $RPM_BUILD_ROOT
-DIST_ROOT=$RPM_BUILD_ROOT
-DIST_MANIFEST=`pwd`/install.manifest
-export DIST_ROOT DIST_MANIFEST
-rm -f $DIST_MANIFEST
-/usr/bin/gmake install_pcp
+make DIST_ROOT=$RPM_BUILD_ROOT install_pcp
Comment 4 Mark Goodwin 2009-07-31 02:47:49 EDT
Following lengthy review discussions with Eric Sandeen (thanks Eric)
and several PCP community developers, and after working thru errors
and warnings reported by 'rpmlint', the changes listed below have been
committed to the 'dev' branch in my tree at
  git://oss.sgi.com/markgw/pcp/pcp.git

For review purposes, the Fedora specific spec and SRPM for PCP have been
copied up to :

Spec URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp.spec
SRPM URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-1.fc10.src.rpm

Most of the changes made so far are to the Fedora RPM spec itself - these
need to be reconciled, where possible, with the community RPM spec.

The remaining changes are mostly to silence rpmlint:

* pcp-libs.x86_64: E: library-without-ldconfig-postin
To fix, need to add a "%post libs" scriptlet to call to ldconfig

* 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.

* 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.

* 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.

* 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

* pcp.x86_64: W: log-files-without-logrotate /var/log/pcp
PCP has it's own log management system, see pmlogger_daily(1)

* 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.

* pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmproxy
* pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmie
* pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pcp
rpmlint suggests we need to set up a lock file in /var/lock/subsys.

Changes commited so far to git://oss.sgi.com/markgw/pcp/pcp.git
(most recent changes are listed first). Please clone my git tree
if you want to see the patches  :

commit 5dc76a384254b3a0b1a72ff1be5faec042effa73
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Fri Jul 31 15:44:40 2009 +1000

        RPM spec specifically for the Fedora Project.
    
        Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit 1274605472853400f4297439c433a05829735a64
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Fri Jul 31 15:40:58 2009 +1000

    tweak configure to move PCP_BINADM_DIR out of /usr/share, into /usr/lib.
    Arch dependent binaries should not be installed below /usr/share.
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit 17700e2bdaa0060178c3d025edb92280f36451f6
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Fri Jul 31 11:47:39 2009 +1000

    delete unneeded explicit script interpreter to keep rpmlint happy
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit b971772c281a5aff6408981e7086cbfa4bcee6ba
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Fri Jul 31 11:33:20 2009 +1000

    pmpost does not really need to be setuid
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit 0a4cd4b8bfe44265bdccfb93db5e19c28c796f79
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Fri Jul 31 11:11:48 2009 +1000

    Nuke migrate_pcp_var_dir, no longer needed
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit bf98168e8a7cea7697497f4972dd835fa10e29b6
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Thu Jul 30 18:25:12 2009 +1000

    default chkconfig off for all PCP services
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit 3baf8e7a6491c9533a1b0ff00c6d711a32677536
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Mon Jul 27 09:32:57 2009 +1000

    take -fstack-protector-all back out again since it's only supported with newer compilers. Probably should add a configure test for it at some stage
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit 0c1b4b2f4bd9b0759964fb056a1469b2dd052ce6
Author: Mark Goodwin <mgoodwin@redhat.com>
Date:   Wed Jul 22 17:02:40 2009 +1000

    Tweak RPM dependency rules for 2.x -> 3.x upgrades and
    massage the PCP RPM spec to reduce rpmlint noise.
    
    Also added -fstack-protector-all to Linux PCFLAGS.
    
    On branch 3.0.0:
    	modified:   build/rpm/pcp.spec.in
    	modified:   src/include/builddefs.in
    
    Signed-off-by: Mark Goodwin <mgoodwin@redhat.com>

commit ff6cfff7845a832ab3b0affe19c6ffa33293bc93
Author: Mark Goodwin <mgoodwin@fletch.(none)>
Date:   Fri Jul 10 14:36:29 2009 +1000

    move pcp.conf into libs and check for pcp-devel in PMDAs that ship a Makefile

commit 728c61eb0aa0ebe9bf3b4096696adfc5b14abf39
Author: Mark Goodwin <mgoodwin@fletch.(none)>
Date:   Thu Jul 9 17:33:12 2009 +1000

    split RPM packaging into pcp, pcp-libs and pcp-devel, bump version to 3.0.0
Comment 5 Eric Sandeen 2009-07-31 11:52:06 EDT
Ok a few more comments, sorry for being iterative.

Naming the spec "pcp_fedora.spec" threw me off, not sure if it's kosher or not.

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.

---

%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.

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.

---

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)

---

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

---
Comment 6 Eric Sandeen 2009-07-31 12:34:14 EDT
(In reply to comment #4)

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?

> * 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.

FWIW I've seen other such things for example in db4-devel:

/usr/share/doc/db4-devel-4.7.25/examples_c/bench_001.c

> * 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 :)

> * 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 :)

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.

> * 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?

> * 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?

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?

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?

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.

Also on the root.saved stuff - I can't find anything in scripts or source that creates this.  I see root.prev though.

Thanks,
-Eric
Comment 7 Eric Sandeen 2009-07-31 12:55:33 EDT
Created attachment 355832 [details]
modified specfile

This is what I'd propose, assuming it works, and assuming I'm not being too draconian based on misunderstandings of how things actually work :)

If stuff needs to go back in, with justification, I'll let you talk that over w/ the final reviewer.

Thanks for your patience and understanding ;)

-Eric
Comment 8 Mark Goodwin 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
Comment 9 Mark Goodwin 2009-08-04 07:25:46 EDT
Eric, didn't quite get to finish this off this arvo - still want to add
%docs and also resolve the rc script issues with /var/run/*.pid and rpmlint.
I have both figured out, but haven't finished building & QA'ing .. tomorrow.

Cheers 
-- Mark
Comment 10 Eric Sandeen 2009-08-05 11:57:34 EDT
Oh, I'll take this for review, though I can't sponsor.
Comment 11 Mark Goodwin 2009-08-05 22:55:39 EDT
Hi Eric, I've just posted v3.0.0-2 :

Spec URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp.spec
Tar URL : ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-2.src.tar.gz
SRPM URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-2.fc10.src.rpm

Merged with pcp-2.9.0-1 and pushed up to:
  git://oss.sgi.com/markgw/pcp/pcp.git dev

Changes since 3.0.0-1 are listed in the git logs, in summary :

    - merge with pcp-2.9.0-1 dev

    - bump to 3.0.0-2, update changelog and delete unncessary Vendor string

    - changes to the Fedora RPM spec in response to review feedback:
      - Incorporate a large number of suggestions and cleanups from Eric Sandeen
      - Greatly simplified the %post and %postun scriptlets - remove ancient crud
      - Added ldconfig scriptlets for -libs
      - Don't explicitly require IB libs, since they're libs and RPM figures it out
      - No need to explicitly BuildRequire gcc-c++ libstdc++-devel
      - Add BuildRequires on perl-ExtUtils-MakeMaker
      - Remove explicit ia64 Requires: libunwind
      - Have pcp-libs require the base package since Fedora insists
      - Add and clarify some comments
      - Create %{_localstatedir}/run/pcp and ship it (so it'll be removed)
      - Use %doc for CHANGELOG COPYING INSTALL README VERSION.pcp pcp.lsm

    - git ignore generated gram*.tab.c

    - preserve generated gram.tab.c because debuginfo needs it

    - fix minor typo (Redhat -> Red Hat) in comment in Fedora spec

    - dont install trace demo binaries since src is installed anyway

    - tweak configure to move PCP_BINADM_DIR out of /usr/share, into /usr/lib.

    - Arch dependent binaries should not be installed below /usr/share.

    - delete unneeded explicit script interpreter to keep rpmlint happy

    - pmpost does not really need to be setuid

    - Nuke migrate_pcp_var_dir, no longer needed

    - default chkconfig off for all PCP services

Still have the (benign) rpmlint errors:
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmproxy
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmie
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pcp

Cheers
-- Mark Goodwin
Comment 12 Eric Sandeen 2009-08-06 15:48:16 EDT
Ok, here goes the last informal review and I'll hand off to Jarod.

Koji rawhide scratch build here (successful):
http://koji.fedoraproject.org/koji/taskinfo?taskID=1587488

rpmlint:

# rpmlint /tmp/pcp-3.0.0-2.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# rpmlint RPMS/x86_64/pcp*3.0.0-2.fc12.x86_64.rpm 
pcp.x86_64: E: obsolete-on-name
pcp.x86_64: W: conffile-without-noreplace-flag /etc/bash_completion.d/pcp
pcp.x86_64: W: conffile-without-noreplace-flag /etc/pcp.env
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/trivial/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/mounts/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/trace/app2.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/weblog/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/simple/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/pmclient/pmclient.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/mmv/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/trivial/trivial.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/cisco/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/mailq/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/linux/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/trace/stub.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/sample/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/txmon/txrecord.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/jstat/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/shping/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/summary/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/process/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/trace/app1.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/simple/simple.c
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/procmemstat/procmemstat.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/sendmail/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/txmon/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/trace/app3.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/trace/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /usr/share/pcp/demos/trace/pmtrace.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/txmon/txmon.c
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/lmsensors/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/apache/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/roomtemp/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/lustrecomm/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/txmon/txmon.h
pcp.x86_64: W: log-files-without-logrotate /var/log/pcp
pcp.x86_64: W: dangerous-command-in-%preun rm
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmproxy
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmie
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pcp
pcp-devel.x86_64: W: dangling-relative-symlink /usr/lib64/libpcp_gui.so.1 libpcp_gui.so.2
pcp-devel.x86_64: W: dangling-relative-symlink /usr/lib64/libpcp.so.2 libpcp.so.3
pcp-devel.x86_64: W: dangling-relative-symlink /usr/lib64/libpcp_pmda.so.2 libpcp_pmda.so.3
pcp-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpcp_pmda.so.3 exit@GLIBC_2.2.5
pcp-libs.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 4 errors, 41 warnings.


    * MUST: rpmlint must be run on every package.  See above.
    * MUST: The package must be named according to the Package Naming Guidelines. OK
    * MUST: The spec file name must match the base package %{name}. OK
    * MUST: The package must meet the Packaging Guidelines. 

NEEDSWORK? - 4 errors still above.  subsys-not-used should be easy to fix up, 

A package should not obsolete itself, as it can cause weird errors in tools.
# Prior to v3, the PCP package implicitly "provides" -libs and -devel.
# Strictly, pcp-libs should obsolete the v2.x PCP package, but since
# pcp requires pcp-libs, pcp can just obsolete itself. This is thus
# redundant dependency, but included for clarity.
Obsoletes: pcp < 3.0

I'll let Jarod be the final arbiter on this.

    * MUST: The package must be licensed with a Fedora approved license. OK
    * MUST: The License field in the package spec file must match the actual license.

NEEDSWORK?
From COPYING:
All the libraries in the Performance Co-Pilot (PCP) open source
release are licensed under Version 2.1 of the GNU Lesser General
Public License.

All other components in the PCP open source release are licensed
under Version 2 of the GNU General Public License.

but the specfile says:
License: GPL+ and LGPLv2+

All .c and .h files do say "or any later version" so ideally COPYING should be fixed to reflect this.

    * 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. OK
    * MUST: The spec file must be written in American English. OK
    * MUST: The spec file for the package MUST be legible. OK

(I might rather see a little more consistency between wildcards & explicit files, but not a big deal)

    * MUST: The sources used to build the package must match the upstream source. OK
    * MUST: The package MUST successfully compile and build into binary rpms OK
    * MUST: All build dependencies must be listed in BuildRequires OK
    * MUST: The spec file MUST handle locales properly. OK
    * MUST: Every binary RPM package (or subpackage) which stores shared library files must call ldconfig in %post and %postun. OK
    * MUST: A package must own all directories that it creates. OK
    * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. OK
    * MUST: Permissions on files must be set properly. OK
    * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). OK
    * MUST: Each package must consistently use macros. OK
    * MUST: The package must contain code, or permissable content. OK
    * MUST: Large documentation files must go in a -doc subpackage. OK
    * MUST: If a package includes something as %doc, it must not affect the runtime of the application. OK
    * MUST: Header files must be in a -devel package. OK

Note: "OK" based on pmda .h files "not being header files but rather used for configuration"

    * MUST: Static libraries must be in a -static package. N/A
    * MUST: Packages containing pkgconfig(.pc) files N/A
    * MUST: If a package contains library files with a suffix then library files that end in .so must go in a -devel package. OK
    * MUST: Devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}

NEEDSWORK: Requires: pcp-libs = %{version}

For whatever reason I guess we must require pcp, not pcp-libs.

    * MUST: Packages must NOT contain any .la libtool archives OK
    * MUST: Packages containing GUI applications must include a %{name}.desktop file N/A (but keep in mind for the pcp gui?)
    * MUST: Packages must not own files or directories already owned by other packages. OK
    * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) OK
    * MUST: All filenames in rpm packages must be valid UTF-8. OK

SHOULD Items:
    * 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/A (license text is there)
    * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. N/A (not available and I've never seen it!)
    * SHOULD: The reviewer should test that the package builds in mock. OK
    * SHOULD: The package should compile and build into binary rpms on all supported architectures. OK
    * SHOULD: The reviewer should test that the package functions as described. OK (I started pcp anyway) ;)
    * SHOULD: If scriptlets are used, those scriptlets must be sane. OK (now!)
    * SHOULD: Subpackages other than devel should require the base package using a fully versioned dependency.  NO, but it seems ok
    * SHOULD: The placement of pkgconfig(.pc) files N/A
    * 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. OK

Just a few things left here, which I'll leave to Jarod's discretion.

-Eric
Comment 13 Mark Goodwin 2009-08-07 02:04:30 EDT
Thanks Eric, once again. I'll work thru the list and have another
update ready on Monday - particularly the license, which will need to
be updated in the spec, based on the COPYING file .. ooops :)
Comment 14 Mark Goodwin 2009-08-18 04:49:10 EDT
Eric Sandeen wrote:
> * MUST: The package must meet the Packaging Guidelines. 
> NEEDSWORK? - 4 errors still above.  subsys-not-used should be easy to fix up, 

I looked at this and I'd rather not change it.

It turned out not to be that easy to fix. The three PCP services
(pcp, pmie and pmproxy) all manage their own var/run/pcp/pid files.
This pre-dates the standard functionality for managing pid files and
is also multi-platform and also rock solid stable.

> * MUST: The License field in the package spec file must match the actual 
> license.
> 
> NEEDSWORK?
> From COPYING:
> All the libraries in the Performance Co-Pilot (PCP) open source
> release are licensed under Version 2.1 of the GNU Lesser General
> Public License.
> 
> All other components in the PCP open source release are licensed
> under Version 2 of the GNU General Public License.
> 
> but the specfile says:
> License: GPL+ and LGPLv2+

The previous version tried to specify the license for the base package
and the two subpackages, which is wrong because -libs has a different
license. So I've now split this so each package specifies it's own license.
Changed the spec so the base package and -devel specify "GPLv2+"
(assuming "GPLv2+" is the best match for "GPLv2.1", as specified in COPYING
since there is no explicit option for "GPLv2.1"), and -libs is "LGPLv2"
(exactly matching what's in COPYING).

> * MUST: Header files must be in a -devel package. OK
> Note: "OK" based on pmda .h files "not being header files but rather used for
> configuration"

yes that's correct, as already discussed.

> * MUST: Devel packages must require the base package using a fully
> versioned dependency: Requires: %{name} = %{version}-%{release}
> 
> NEEDSWORK: Requires: pcp-libs = %{version}
> 
> For whatever reason I guess we must require pcp, not pcp-libs.

Nathan and I discussed this and we concluded the only True Dependencies are:
pcp-devel requires pcp-libs
pcp requires pcp-libs

Neither pcp-devel nor pcp-libs actually requires pcp. There is a good
reason we don't want pcp-devel to require pcp - basically it has to do with
the pcp daemon on the build machine getting killed when pcp in the chroot
gets uninstalled, i.e. we want to be able to build packages (such as pcp-gui)
that BuildRequires pcp-devel *without* pcp installed (just pcp-devel and
pcp-libs should be installed).

> * SHOULD: Subpackages other than devel should require the base package
> using a fully versioned dependency.  NO, but it seems ok

Comment: if -libs and the base package require each other, then what's the point
of splitting out -libs since they can never be installed separately?

So based on the above, I'm leaving the run-time and build-time dependencies
as they strictly need to be. If the final Fedora reviewer and/or sponsor insist
on additional dependencies, then I'll conform, reluctantly :)

Other changes in this round:

- reconciled the open source spec with the current Fedora spec.
- merged with latest 2.9 dev tree
- bumped to 3.0.0-3 and pushed updated spec, .tgz and srpm files to oss.
- pushed my tree to git://oss.sgi.com/markgw/pcp/pcp.git dev

Cheers
-- Mark
Comment 15 Jarod Wilson 2009-08-20 09:42:56 EDT
(In reply to comment #14)
> Eric Sandeen wrote:
> > * MUST: The package must meet the Packaging Guidelines. 
> > NEEDSWORK? - 4 errors still above.  subsys-not-used should be easy to fix up, 
> 
> I looked at this and I'd rather not change it.
> 
> It turned out not to be that easy to fix. The three PCP services
> (pcp, pmie and pmproxy) all manage their own var/run/pcp/pid files.
> This pre-dates the standard functionality for managing pid files and
> is also multi-platform and also rock solid stable.

I think we can live with that.

> > * MUST: The License field in the package spec file must match the actual 
> > license.
> > 
> > NEEDSWORK?
> > From COPYING:
> > All the libraries in the Performance Co-Pilot (PCP) open source
> > release are licensed under Version 2.1 of the GNU Lesser General
> > Public License.
> > 
> > All other components in the PCP open source release are licensed
> > under Version 2 of the GNU General Public License.
> > 
> > but the specfile says:
> > License: GPL+ and LGPLv2+
> 
> The previous version tried to specify the license for the base package
> and the two subpackages, which is wrong because -libs has a different
> license. So I've now split this so each package specifies it's own license.
> Changed the spec so the base package and -devel specify "GPLv2+"
> (assuming "GPLv2+" is the best match for "GPLv2.1", as specified in COPYING
> since there is no explicit option for "GPLv2.1"), and -libs is "LGPLv2"
> (exactly matching what's in COPYING).

"GPLv2+" is only for "GPLv2.anything or later", if it doesn't say "or later", then simply "GPLv2" is the tag you want.

> > * MUST: Devel packages must require the base package using a fully
> > versioned dependency: Requires: %{name} = %{version}-%{release}
> > 
> > NEEDSWORK: Requires: pcp-libs = %{version}
> > 
> > For whatever reason I guess we must require pcp, not pcp-libs.
> 
> Nathan and I discussed this and we concluded the only True Dependencies are:
> pcp-devel requires pcp-libs
> pcp requires pcp-libs
> 
> Neither pcp-devel nor pcp-libs actually requires pcp. There is a good
> reason we don't want pcp-devel to require pcp - basically it has to do with
> the pcp daemon on the build machine getting killed when pcp in the chroot
> gets uninstalled, i.e. we want to be able to build packages (such as pcp-gui)
> that BuildRequires pcp-devel *without* pcp installed (just pcp-devel and
> pcp-libs should be installed).

I think it might be cleanest/most obvious to users if the -devel package were renamed to pcp-libs-devel, since its really the devel headers for the libraries. Then we're even reasonably satisfying the guidelines, I'd argue.

> > * SHOULD: Subpackages other than devel should require the base package
> > using a fully versioned dependency.  NO, but it seems ok
> 
> Comment: if -libs and the base package require each other, then what's the
> point
> of splitting out -libs since they can never be installed separately?

Welcome to multiarch. :) Splitting the libs out allows you to have pcp.x86_64, pcp-libs.x86_64 and pcp-libs.i686 all installed at the same time without any file conflicts (in theory).

> So based on the above, I'm leaving the run-time and build-time dependencies
> as they strictly need to be. If the final Fedora reviewer and/or sponsor insist
> on additional dependencies, then I'll conform, reluctantly :)

I don't think its necessary on this one, there's a reasonable reason not to.


I presume this is the srpm I should be looking at now?

ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-3.fc10.src.rpm
Comment 16 Jarod Wilson 2009-08-20 11:27:51 EDT
Okay, there's still some slightly alarming rpmlint spew... I understand the header files need to be in the main package, but what about all these c files? Particularly the ones under 'sample' and 'demo' sub-directories. These should go into somewhere like /usr/share/doc/pcp/{sample,demo}/, not into /usr/share/pcp or /var/lib/pcp.

The .NeedRebuild touch and rm in the scriptlets is also... Not pretty. Files created by an rpm need to be owned by the rpm, even if they're temporary files. You could add an empty file to the rpm %files list itself, so the rpm lays it down and owns it, and there's technically no harm in the file being deleted from the system after this rebuild takes place. If the file continues to exist, an rpm -e will remove it, no need for the rm in the %postun scriptlet.

Speaking of the scriptlets... Standard convention is to put them before the %files lists, not after. %files is generally the last thing before the %changelog.
Comment 17 Mark Goodwin 2009-08-21 01:50:29 EDT
Jarod, thanks - I think I've addressed most of your questions, apart
from the C source files, which I'll get to early next week if not sooner.
Comments below.

In Comment #15 from Jarod Wilson <jwilson@redhat.com> wrote:
> "GPLv2+" is only for "GPLv2.anything or later", if it doesn't say "or later",
> then simply "GPLv2" is the tag you want.

Thanks for the clarification, fixed.

> I think it might be cleanest/most obvious to users if the -devel package were
> renamed to pcp-libs-devel, since its really the devel headers for the
> libraries. Then we're even reasonably satisfying the guidelines, I'd argue.

done. consulted the PCP community and they agree, good suggestion.

> >> > > * SHOULD: Subpackages other than devel should require the base package
> >> > > using a fully versioned dependency.  NO, but it seems ok
> > > 
> > > Comment: if -libs and the base package require each other, then what's the
> > > point
> > > of splitting out -libs since they can never be installed separately?
> 
> Welcome to multiarch.  :)  Splitting the libs out allows you to have pcp.x86_64,
> pcp-libs.x86_64 and pcp-libs.i686 all installed at the same time without any
> file conflicts (in theory).

ah ok, that's a good reason then :)

> > > So based on the above, I'm leaving the run-time and build-time dependencies
> > > as they strictly need to be. If the final Fedora reviewer and/or sponsor insist
> > > on additional dependencies, then I'll conform, reluctantly  :) 
> 
> I don't think its necessary on this one, there's a reasonable reason not to.
> I presume this is the srpm I should be looking at now?
> ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-3.fc10.src.rpm

yes and I see you found it. The new version will be pcp-3.0.0-4.fc10 after the
changes in this update.

In Comment #16 from Jarod Wilson <jwilson@redhat.com> wrote:
> Okay, there's still some slightly alarming rpmlint spew... I understand the
> header files need to be in the main package, but what about all these c files?
> Particularly the ones under 'sample' and 'demo' sub-directories. These should

The /usr/share/pcp/demos directory can easily enough be moved to
/usr/share/doc/pcp-*/demos  so I'll do that (haven't in this update
though because I need to investigate the QA fall-out).

> ... go into somewhere like /usr/share/doc/pcp/{sample,demo}/, not into
> /usr/share/pcp or /var/lib/pcp.

I'll investigate simply not installing the source for the 'simple',
'trivial' and 'sample' agents at all. This source was shipped back
in the days when PCP was proprietary; nowdays of course it's all readily
available anyway :)

> The .NeedRebuild touch and rm in the scriptlets is also... Not pretty. Files
> created by an rpm need to be owned by the rpm, even if they're temporary files.
> You could add an empty file to the rpm %files list itself, so the rpm lays it
> down and owns it, and there's technically no harm in the file being deleted
> from the system after this rebuild takes place. 

OK, I've added .NeedRebuild as an install target in src/pmns/GNUmakefile
(since other platforms need it too, so just touching it during %install in
the spec would be sub-optimal), and added an entry in %files for it.

> If the file continues to exist,
> an rpm -e will remove it, no need for the rm in the %postun scriptlet.

ok fine, removed.

> Speaking of the scriptlets... Standard convention is to put them before the
> %files lists, not after. %files is generally the last thing before the
> %changelog.

ok, fixed.

New srpm, spec and *.tgz uploaded to ftp://oss.sgi.com/projects/pcp/download/v3
and pushed the changes up to git://oss.sgi.com/markgw/pcp/pcp.git dev
(see the git log in that tree for commits)

Thanks
-- Mark Goodwin
Comment 18 Jarod Wilson 2009-09-01 15:50:09 EDT
(In reply to comment #17)
> Jarod, thanks - I think I've addressed most of your questions, apart
> from the C source files, which I'll get to early next week if not sooner.

Just checking in to see where we're at on this. I think I was waiting to hear back on this part before doing another (hopefully the last) pass over things.
Comment 19 Mark Goodwin 2009-09-01 20:12:28 EDT
(In reply to comment #18)
> (In reply to comment #17)
> > Jarod, thanks - I think I've addressed most of your questions, apart
> > from the C source files, which I'll get to early next week if not sooner.
> 
> Just checking in to see where we're at on this. I think I was waiting to hear
> back on this part before doing another (hopefully the last) pass over things.  

Hi Jarod, thanks for checking back - sorry I've been completely snowed under with other stuff. I've moved the C source to a better place and have just a few things left to clean up, so I'll make a concerted effort on that front :)

We also had a discussion on freenode.net #pcp about what to do with the Perl binding for PCP. Perhaps you could provide some guidance here for the packaging perspective - the perl binding is part of the main src tree (below src/cpan) but the packaging is currently driven by MakeMaker (with a manual step to build the pcp perl RPMs from the resulting spec). Ideally, we'd like the perl binding to be a sub-package, e.g. "pcp-perl-this-and-that", with the build and packaging all driven by the main spec. I have some simple changes to the spec that implements this but it probably violates the Fedora Perl packaging guidelines - anything perl'ish should be named "perl-something". So the only way to resolve that would be to run MakeMaker in %build or %install and to generate additional perl-pcp-something RPMs that way ... which seems hacky.

Any advice? maybe we should split the PCP perl binding into a separate src
tree, with appropriate BuildRequires dependencies?

Cheers
-- Mark
Comment 20 Jarod Wilson 2009-09-04 15:26:37 EDT
> We also had a discussion on freenode.net #pcp about what to do with the Perl
> binding for PCP. Perhaps you could provide some guidance here for the packaging
> perspective - the perl binding is part of the main src tree (below src/cpan)
> but the packaging is currently driven by MakeMaker (with a manual step to build
> the pcp perl RPMs from the resulting spec). Ideally, we'd like the perl binding
> to be a sub-package, e.g. "pcp-perl-this-and-that", with the build and
> packaging all driven by the main spec. I have some simple changes to the spec
> that implements this but it probably violates the Fedora Perl packaging
> guidelines - anything perl'ish should be named "perl-something".

Yes, they should definitely be built from the main spec and the same source rpm, since they're all in the same source tarball. If naming is the only issue here, then simply use '%package -n perl-pcp-something'. The -n says "don't prepend the main package name to this". Do teh same for %description, %files, etc.
Comment 21 Mark Goodwin 2009-09-06 21:07:18 EDT
Hi Jarod, v3.0.0-5 is ready for review :
Spec URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp.spec
SRPM URL: ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-5.fc10.src.rpm
Src.tgz : ftp://oss.sgi.com/projects/pcp/download/v3/pcp-3.0.0-5.src.tar.gz
(spec and src.tgz are identical to what's in the SRPM).

The pcp-perl packaging (thanks for the comments) is not included - I'd
rather avoid any further delay and instead add the perl packaging later
once we're in Fedora - if this is a bad plan, please let me know :)

The demo agents that ship src have been moved into the devel package.
They're required for the PCP QA suite to run, but so is the rest of
the devel package, so no problem there. I've also merged up with the
recent 2.9.1 community release, which is stable and fully QA'd.

[mgoodwin@fletch rpmbuild]$ rpmlint SRPMS/pcp-3.0.0-5.fc10.src.rpm RPMS/x86_64/{pcp-3.0.0-5.fc10.x86_64.rpm,pcp-libs-3.0.0-5.fc10.x86_64.rpm,pcp-libs-devel-3.0.0-5.fc10.x86_64.rpm}
pcp.x86_64: W: conffile-without-noreplace-flag /etc/bash_completion.d/pcp
pcp.x86_64: W: conffile-without-noreplace-flag /etc/pcp.env
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/mounts/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/weblog/domain.h
pcp.x86_64: W: hidden-file-or-dir /var/lib/pcp/pmns/.NeedRebuild
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/mmv/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/cisco/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/mailq/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/linux/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/jstat/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/shping/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/summary/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/process/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/sendmail/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/trace/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/lmsensors/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/apache/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/roomtemp/domain.h
pcp.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/lustrecomm/domain.h
pcp.x86_64: W: log-files-without-logrotate /var/log/pcp
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmproxy
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pmie
pcp.x86_64: E: subsys-not-used /etc/rc.d/init.d/pcp
pcp-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libpcp_pmda.so.3 exit@GLIBC_2.2.5
pcp-libs.x86_64: W: no-documentation
pcp-libs-devel.x86_64: W: dangling-relative-symlink /usr/lib64/libpcp_gui.so.1 libpcp_gui.so.2
pcp-libs-devel.x86_64: W: dangling-relative-symlink /usr/lib64/libpcp.so.2 libpcp.so.3
pcp-libs-devel.x86_64: W: dangling-relative-symlink /usr/lib64/libpcp_pmda.so.2 libpcp_pmda.so.3
4 packages and 0 specfiles checked; 3 errors, 25 warnings.

Many thanks
-- Mark Goodwin
Comment 22 Jarod Wilson 2009-10-05 13:51:48 EDT
Finally getting back to looking at this again, sorry for the delay.

Formal review check list from https://fedoraproject.org/wiki/Packaging:ReviewGuidelines:

MUST items-
* rpmlint output: still rather noisy, but has all been explained acceptably
* package naming: ok
* spec file name: ok
* package meets guidelines: almost... Just noticed the use of %makeinstall, which I don't think we've touched on yet... Its use is HIGHLY frowned upon...
* license: ok
* license in spec matches: ok
* license doc included: ok
* spec in english: ok
* spec legible: ok
* sources match upstream: ok
    $ md5sum pcp-3.0.0-5.src.tar.gz*
    de5b6be8fcd08985756a01fc276774d7  pcp-3.0.0-5.src.tar.gz
    de5b6be8fcd08985756a01fc276774d7  pcp-3.0.0-5.src.tar.gz.1
* compiles and packages correctly: ok (F12/x86_64 smoke-tested)
* ExcludeArch where necessary: n/a
* ExcludeArch for build failure filed and noted: n/a
* sane BuildRequires: ok
* locales handled properly: n/a
* ldconfig called where appropriate: ok
* no bundled copies of system libs: ok
* relocatable justification: n/a
* owns created directories: ok
* files not listed 2x: ok
* permissions: ok
* consistent macros: ok
* permissible code: ok
* documentation: ok
* header files in -devel: ok (with exceptions as noted earlier in review)
* static libs: n/a
* pkgconfig bits: n/a
* versioned libs properly split up: ok
* devel packages requires its base package: ok
* no libtool archives: ok
* gui .desktop file: n/a
* no owning others files: ok
* %install starts w/rm -rf BR: ok
* %files all UTF-8: ok

SHOULD items-
* source needs license text: n/a
* spec translations, if available: n/a
* package builds in mock: ok
* builds on all supported arches: untested
* package functions as expected: presumed
* sane scriptlets: ok
* fully version requires for sub-packages: ok and/or n/a
* pkgconfig stuff: n/a
* file-based deps: n/a


Just the %makeinstall bit that needs fixing up, and I think we're all set.

https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used
Comment 23 Mark Goodwin 2009-10-07 06:59:12 EDT
Thanks Jarod, we've moved along a little bit and now have the perl-PCP
packages being built from the same spec. I've nuked %makeinstall and
further tweaked things for rpmlint - we (the PCP community developers)
are just going thru final QA now and expect to be all done by the end
of this week. So I'll UPDATE here again with the new spec and tarball
details when that occurs.

Thanks again
-- Mark
Comment 24 Mark Goodwin 2009-10-11 20:21:40 EDT
Hi Jarod,

final spec and src tarball are now ready for final review:

The src tarball is in it's permanent place (matching the spec) :
ftp://oss.sgi.com/projects/pcp/download/pcp-3.0.0-9.src.tar.gz

I'll attach the spec to this BZ in the next update.

Thanks
-- Mark Goodwin
Comment 25 Mark Goodwin 2009-10-11 20:31:04 EDT
Created attachment 364404 [details]
pcp.spec for final Fedora review, as in the pcp-3.0.0-9 community release


pcp.spec for final Fedora review. This is the Fedora version of the
specfile that was used to build the RPMS for the pcp-3.0.0-9
community release. The URL to the matching src tarball is specified
as Source0 in this specfile.
Comment 26 Mark Goodwin 2009-11-06 00:21:26 EST
Created attachment 367782 [details]
updated pcp spec for Fedora, matching pcp-3.0.1-2 community release

Minor update to the Fedora spec after the pcp-3.0.1-2 community release.
Note there is one new rpmlint 'E': fter doing some upgrade testing, we
changed /etc/pcp.conf to be tagged as %config rather than %config(noreplace).

Jarod, I'd appreciate an ack on this if you have time :)

Thanks
-- Mark
Comment 27 Mark Goodwin 2009-12-10 23:52:58 EST
Jarod, can I take comment #22 as final ack?

Thanks
-- Mark
Comment 28 Jarod Wilson 2009-12-15 13:51:29 EST
Apologies for the delays, I've been a bit, um... tied up... with new happenings here in the office. Yeah, with the tweaks requested in comment #22 done, I'm going to say you're all set. Thanks much for your patience working though this

Package APPROVED.

iirc, we still need to get you hooked up packager group sponsorship, which I think needs to happen prior to filing a cvs request to add the package...
Comment 29 Eric Sandeen 2010-05-17 14:40:49 EDT
Hm, what's next?  An awful lot of effort on this bug to not yet have it built ;)
Comment 30 Mark Goodwin 2010-05-17 18:25:27 EDT
(In reply to comment #29)
> Hm, what's next?  An awful lot of effort on this bug to not yet have it built
> ;)    

Hi Eric, yes activity has resumed - Bill Cohen is talking with Andrew Overholt
in the tools group as a possible sponsor. Alternatively, we could get you
promoted to become sponsor-capable - you maintain enough important packages
in Fedora that maybe this would be a good idea regardless (?).

Cheers
-- Mark
Comment 31 Eric Sandeen 2010-05-17 18:30:49 EDT
Fine by me, I am not terribly well-versed in the ins & outs of fedora policy TBH...
Comment 32 Andrew Overholt 2010-05-17 20:23:46 EDT
This is the first I've heard of this :)  However, I'm not really well-versed enough in the packaging guidelines for non-Java stuff so I can't be a sponsor, sorry.
Comment 33 Jarod Wilson 2010-05-18 15:07:12 EDT
Bah, sorry, I was unclear. I can take care of sponsorship, but iirc, you have to get a fedora account set up and apply for membership in the packager group before I can do that.
Comment 34 Mark Goodwin 2010-06-16 02:02:25 EDT
Many thanks Jarod. Returning to this task again now. I already have a Fedora
FAS account, though it's not entirely clear to me on how to get added to the 
packager group. The docs at
http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored
says "Your sponsor can add you to the packager group. You should receive email confirmation of your sponsorship."
 
In any case, removing FE-NEEDSPONSOR now we have a sponsor. Next update will 
include a New Package CVS Request. Once processed, that might let me set
the fedora‑cvs flag (which may be how I get into the packager group?).

Cheers
-- Mark
Comment 35 Mark Goodwin 2010-06-16 02:07:31 EDT
New Package CVS Request
=======================
Package Name: pcp
Short Description: System-level performance monitoring and performance management toolkit
Owners: mgoodwin
Branches: F-13 EL-4 EL-5 EL-6
InitialCC: jarod@redhat.com
Comment 36 Jarod Wilson 2010-06-16 11:26:33 EDT
Typically, you request inclusion in the group 'packager' within the Fedora Account System (https://admin.fedoraproject.org/accounts/), and then someone agrees to sponsor you, at which point you're approved for membership in that group. I went ahead and applied for you and sponsored you in one pass just now.
Comment 37 Mark Goodwin 2010-06-23 07:54:05 EDT
Thanks again Jarod. PCP version 3.3.0 is just about done now and the
community are asking for Fedora and RHEL builds. So setting the fedora‑cvs
flag for the branches specified in comment #35.

 -- Mark
Comment 38 Jason Tibbitts 2010-06-26 02:51:52 EDT
InitialCC takes a Fedora account, not an email address, and the account "jarod"
seems to belong to someone else so I've simply done the request without any
InitialCC.  Please add yourself to the package in pkgdb as appropriate.

CVS done (by process-cvs-requests.py).
Comment 39 Fedora Update System 2010-07-19 22:20:18 EDT
pcp-3.3.3-1.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/pcp-3.3.3-1.el5
Comment 40 Fedora Update System 2010-07-19 22:20:36 EDT
pcp-3.3.3-1.el4 has been submitted as an update for Fedora EPEL 4.
http://admin.fedoraproject.org/updates/pcp-3.3.3-1.el4
Comment 41 Fedora Update System 2010-07-19 22:20:42 EDT
pcp-3.3.3-1.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/pcp-3.3.3-1.fc13
Comment 42 Eric Sandeen 2010-07-20 13:27:51 EDT
Mark, FWIW, you could build this for EPEL6 now as well

-Eric
Comment 43 Fedora Update System 2010-07-20 18:44:35 EDT
pcp-3.3.3-1.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update pcp'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/pcp-3.3.3-1.fc13
Comment 44 Fedora Update System 2010-07-21 16:02:32 EDT
pcp-3.3.3-1.el4 has been pushed to the Fedora EPEL 4 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update pcp'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/pcp-3.3.3-1.el4
Comment 45 Fedora Update System 2010-07-21 16:03:59 EDT
pcp-3.3.3-1.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update pcp'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/pcp-3.3.3-1.el5
Comment 46 Fedora Update System 2010-08-05 19:36:21 EDT
pcp-3.3.3-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 47 Fedora Update System 2010-08-06 15:57:22 EDT
pcp-3.3.3-1.el4 has been pushed to the Fedora EPEL 4 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 48 Fedora Update System 2010-08-06 15:57:42 EDT
pcp-3.3.3-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 49 Fedora Update System 2011-12-15 05:33:21 EST
pcp-3.5.11-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/pcp-3.5.11-2.el6
Comment 50 Fedora Update System 2011-12-15 05:33:36 EST
pcp-3.5.11-2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/pcp-3.5.11-2.el5
Comment 51 Fedora Update System 2011-12-15 05:33:47 EST
pcp-3.5.11-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pcp-3.5.11-2.fc16
Comment 52 Fedora Update System 2011-12-15 05:34:09 EST
pcp-3.5.11-2.el4 has been submitted as an update for Fedora EPEL 4.
https://admin.fedoraproject.org/updates/pcp-3.5.11-2.el4
Comment 53 Fedora Update System 2011-12-15 05:34:19 EST
pcp-3.5.11-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/pcp-3.5.11-2.fc15
Comment 54 Fedora Update System 2012-05-02 20:39:06 EDT
pcp-3.6.3-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pcp-3.6.3-1.fc16
Comment 55 Fedora Update System 2012-05-02 20:39:51 EDT
pcp-3.6.3-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/pcp-3.6.3-1.el6
Comment 56 Fedora Update System 2012-05-02 20:39:59 EDT
pcp-3.6.3-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/pcp-3.6.3-1.fc15
Comment 57 Fedora Update System 2012-05-02 20:40:05 EDT
pcp-3.6.3-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/pcp-3.6.3-1.el5
Comment 58 Fedora Update System 2012-05-12 21:51:08 EDT
pcp-3.6.3-1.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 59 Fedora Update System 2012-05-12 21:56:24 EDT
pcp-3.6.3-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 60 Fedora Update System 2012-05-18 17:38:33 EDT
pcp-3.6.3-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 61 Fedora Update System 2012-05-18 17:39:14 EDT
pcp-3.6.3-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

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