[Bug 1199693] Review Request: pcp-pmda-cpp - C++ library for PCP PMDAs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1199693



--- Comment #12 from Paul Colby <redhat@xxxxxxxxxxx> ---
> for the base PCP packages, we were allowed to put the source in with the
> PCP_PMDAS_DIR binaries, since they're demos and its a devel package, so
> consider relocating everything from pcp-pmda-cpp-examples into
> /var/lib/pcp/pmdas/pcp-cpp-examples.

Done.  Though I replaced 'pcp-cpp-examples' with 'pcp-pmda-cpp-examples' so
that the directory includes the full RPM package name - seems more responsible
that way :)

> There are doc files in the tarball, which you could include.

Done. I've include the license file via %license and some other docs via %doc.

> You can wrap the entire %clean section in the conditionals, since if
> you don't need it you don't need to include an empty section.

Done :)

> The group for base runtime libraries has been "System Environment/
> Libraries" for many years. The group "Development/Libraries" is for the
> separate -devel buildtime packages.

Your suggestion had me a little stumped at first, since these are
development-only (ie header-only) libraries.  However, I eventually realised
that only the *-devel subpackage should be "Development/Libraries" whereas the
top-level package (which doesn't actually get built as an RPM, because it
contains no files) should/would be "System Environment/Libraries" if it were to
ever exist.

So I've updated the top-level Group as suggested :)

> https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Header_Only_Libraries

Thanks. I hadn't noticed that section before.

I've added the *-static Provides, and removed the noarch as dictated by the
Packaging Guidelines.

> Directory %{_datadir}/doc/pcp-cpp is not included, and no other package
> provides it yet either (repoquery --whatprovides /usr/share/doc/pcp-cpp). 
> This needs to be examined.

I believe this should be fixed now, as I've moved the examples under
$PCP_PMDA_DIR (aka %{_pmdasdir}), ie /var/lib/pcp/pmdas, which is owned by the
implicitly-required 'pcp' package.

(I haven't entirely groked the RPM file/dir ownership concepts though, so might
be missing something here)

> If the tests are suitable for a %check section, one should be added to
> the spec file.

Done.  Though that does introduce the otherwise-unnecessary build requirement
for the 'pcp' package (needed for some functional tests).  Seems worth it to me
:)

This also allowed me to simplify the %build and %install steps, since they were
doing extra work to avoid building the unit tests previously.  Now they just
build everything.  (Of course, that does may the build take longer, but its
still pretty quick anyway).

I've updated the files at the Spec and source RPM URLs above.  You can see a
diff for the Spec file changes since the last review (as well as the other
files that changed, if interested in those) at:

https://github.com/pcolby/pcp-pmda-cpp/compare/838957be550f0826179cc3cd16ab6df74d343702...5fde84b0f1468a8d7f4266e8a910b4eb74f13086#diff-25

There's now a couple of, what I believe to be benign, warnings in the
fedora-review output:

* Header files (*/domain.h) in the examples package - these are not C/C++
source header files, but rather, include files for the PCP "simple
preprocessor" (pmcpp).
* devel-file-in-non-devel-package *-examples/pmdas/*.cpp - as noted by Nathan
above, this follows the pre-existing convention of allowing example source code
to be included with example PMDAs.  But if it bothers anyone, I'm more than
happy to simply leave those files out of the packages for now.

Thanks!

Paul.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]