Bug 1199693

Summary: Review Request: pcp-pmda-cpp - C++ library for PCP PMDAs
Product: [Fedora] Fedora Reporter: Paul Colby <redhat>
Component: Package ReviewAssignee: Lukas Berk <lberk>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: bhubbard, green, lberk, mgoodwin, michele, nathans, package-review, zebob.m
Target Milestone: ---Flags: lberk: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Enhancement
Doc Text:
Feature: New C++ API for Performance Co-Pilot PMDAs Reason: Object oriented interfaces augmenting the underlying C libraries providing more options for PCP PMDA developers. Result: Increased language coverage and features for PCP PMDA developers.
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-06-13 15:56:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Workaround for build issues with cmake and overriding optimisation
none
Build log with errors on F24 none

Description Paul Colby 2015-03-07 02:08:11 UTC
Spec URL: https://raw.githubusercontent.com/pcolby/pcp-pmda-cpp/v0.4.2/package/rpm/pcp-pmda-cpp.spec
SRPM URL: https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.2/pcp-pmda-cpp-0.4.2-1.src.rpm

Description:

Hi, I'm the author of the PMDA++ library, and would love to get it included in Fedora.

PMDA++ is a header-only library that allows developers to write Performance Metrics Domain Agents (PMDAs) for Performance Co-Pilot (PCP) in C++.

Fedora Account System Username: pcolby

Comment 1 Michael Schwendt 2015-03-10 15:49:04 UTC
Considering pointing the fedora-review tool at review tickets like this. It evaluates the "Spec URL:" and "SRPM URL:" lines, downloads the latest files, does a local test-build with Mock and then performs checks for many packaging issues:

  fedora-review -b 1199693

Currently, the src.rpm does not build yet. I'll return to this ticket. It would be good, if you could take a look at the fedora-review tool and make it build (or use koji scratch-builds as explained in the How To Join guide).

Comment 2 Paul Colby 2015-03-10 19:56:38 UTC
Thanks Michael.  I'll take a look at the fedora-review tool, and fix whatever it finds.

Thanks again :)

Comment 3 Nathan Scott 2015-03-12 05:30:28 UTC
Created attachment 1000797 [details]
Workaround for build issues with cmake and overriding optimisation

Comment 4 Nathan Scott 2015-03-12 05:31:22 UTC
Hi Paul,

I came across some build issues related to the overriding of optimisation flags (for test coverage verification AIUI) playing havoc with -D_FORTIFY_SOURCE.  I ended up getting a build to complete by cheating and patching out the cmake code that overrides optimisation.  Have attached a little patch... but I think you've since been hacking on this (based on upstream git log) - for me, though, 0.4.2 still exhibits this problem.

The other two little rpmlint issues I saw after that were:
- the package "Summary:" lines need to not end with full stops
- need not 'Requires: pcp-libs' - the build process inserts this dep

cheers.

Comment 5 Paul Colby 2015-03-12 06:16:19 UTC
Thanks Nathan,

I've fixed the _FORTIFY_SOURCE issues (you're right, they were related to disabling optimisations for unit test coverage reports only) by updating the build system to not touch optimisaton flags if _FORTIFY_SOURCE(!=0) is present.

I've also updated the build system to use Fedora's gtest library (Debian refuses to provide that) instead of the external gtest project (which still gets pulled in on Debian systems, or when gtest-devel is not installed), and so prevents the build system needing svn - at least for Fedora, when gtest-devel is installed (the spec file now requires gtest-devel, but otherwise would require svn, and external network access).

I've just updated the *.spec and *.src.rpm files listed above (probably just minutes after your fedora-review run).  fedora-review now builds cleanly for me, but I can see the two issues you've noted in the generated review file.

Also, before building the *.src.rpm, I run rpmlint which is reporting:

pcp-pmda-cpp.spec:28 W: rpm-buildroot-usage %build %{__rm} -r %{buildroot}

So I'll look into that one too.

Thanks again :)

Paul.

Comment 6 Nathan Scott 2015-03-12 06:48:17 UTC
Ah, good stuff - yep, build issues are resolved now, few minor rpmlint errors as discussed already but you're onto those too already.

I remember now there's also a man page warning on the example executables, which can probably be waived since they're examples.  However, I notice we're installing the PMDA binaries into /usr/bin - whereas, the usual location would be below $PCP_PMDAS_DIR from /etc/pcp.conf (these are not user-invocable binaries of course).

Care will need to be taken with pmdasimple and pmdatrivial though, since similarly named C-library demos exist in the base PCP packages... should we use a /var/lib/pcp/pmdas/pcp-cpp-examples directory, perhaps?  Ah, I see also that the source code for these has ended up in /usr/share/doc/pcp-cpp/examples -- 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.

cheers.

Comment 7 Paul Colby 2015-03-12 07:14:33 UTC
> few minor rpmlint errors as discussed

Fixed :)

> there's also a man page warning on the example executables,
> which can probably be waived since they're examples

Great.  I have no idea (yet) how to write man pages :|

There's also a warning for no-documentation on the *-devel package.  I could make the Spec file build the Doxygen documentation and include that, but currently it only generates HTML.  Apparently Doxygen supports Unix man pages output too, but I've never tried it.  I'll have a quick look into that, but not sure how much of a rabbit-hole it might be.

> Care will need to be taken with pmdasimple and pmdatrivial though, ...
> so consider relocating everything from pcp-pmda-cpp-examples into
> /var/lib/pcp/pmdas/pcp-cpp-examples.

Yep, I'll do that :)

Thanks.

Comment 8 Michael Schwendt 2015-03-12 09:23:39 UTC
> There's also a warning for no-documentation on the *-devel package.

The story about warnings and errors is really just that rpmlint (and a few other tools) _try_ to be helpful by pointing out things which may be "bad". It is not always MUST-FIX error condition, and in some cases rpmlint is mistaken.

Also see:

  https://fedoraproject.org/wiki/Common_Rpmlint_issues#no-documentation
  ( from https://fedoraproject.org/wiki/Category:Package_Maintainers )

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

Especially:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> %clean
> %if 0%{?rhel} < 6
> %{__rm} -rf %{buildroot}
> %endif

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.

  %if 0%{?rhel} < 6
  %clean
  %{__rm} -rf %{buildroot}
  %endif


> Name: pcp-pmda-cpp
> Group: Development/Libraries

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.


> %description devel
> PMDA++ is a header-only library 

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


> %files examples
> %{_bindir}/pmda*
> %{_datadir}/doc/pcp-cpp/examples/

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.

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> unit test coverage

If the tests are suitable for a %check section, one should be added to the spec file. [Some people disagree because they would like tests to be run much more often than at rpmbuild-time, but %check sections can be helpful if they reveal incompatibilities in a changed build environment.]

Comment 9 Paul Colby 2015-03-12 10:37:43 UTC
Thanks Michael, some really good stuff there! :D

It's going to take me a day or two to work through it all :)

Cheers.

Comment 10 Paul Colby 2015-03-13 11:43:19 UTC
Hi Nathan,

The short version: should I be able to use dbpmda without ever running the pmcd service?

The long version:

I've added a %check section to the RPM Spec file.

The PMDA++ project provides two different checks - unit tests and functional tests.  When you run "make check" it simply runs both, but you can run "make check-unit" and/or "make check-functional" independently.

Now, the unit tests are working fine, but some of the functional tests use the dbpmda command to validate the sample PMDAs' DSO interfaces.

For example https://github.com/pcolby/pcp-pmda-cpp/blob/master/test/functional/test_simple_open_dso.command

Now, it seems that the dbpmda command won't succeed unless the pmcd service is running (or has run at some point... presumably it caches something somwhere). Otherwise I get an error like: 

dbpmda: Cannot load namespace from "/tmp/.../root": Problems parsing PMNS definitions.

This error goes away if I start the pmcd service.

It seems inappropriate for an RPM Spec file to require a specific service, such as pmcd, to be running to build.  So, is there something I can change to avoid the need for dbpmda to have pmcd running?  Or is it simply impractical to include these tests?

Note, this looks the simlar to (if not the same as) https://bugzilla.redhat.com/show_bug.cgi?id=1062443

What I'll probably do for now, is separate out the tests that require dbpmda (really they're more integration than functional tests anyway), and set the %check section to run all but those tests.  But I would like to know if it is (or should be) possible to include those tests eventually.

Thanks.

Paul.

Comment 11 Paul Colby 2015-03-14 07:02:55 UTC
Hi Nathan,

It seems that the issue with needing pmcd is that my PMNS roots "#include <stdpmid>", but stdpmid is not generated until pmcd runs at least once.

However, it seems that the #include was unnecessary since my sample PMNS files don't depend on anything defined in stdpmid, so I'll remove that include from the examples.

Cheers.

Comment 12 Paul Colby 2015-03-15 01:35:32 UTC
> 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.

Comment 13 Nathan Scott 2015-03-18 05:43:05 UTC
(In reply to Paul Colby from comment #11)
> Hi Nathan,
> 
> It seems that the issue with needing pmcd is that my PMNS roots "#include
> <stdpmid>", but stdpmid is not generated until pmcd runs at least once.

That's correct.  That may change at some point, but best to not rely on it.

> However, it seems that the #include was unnecessary since my sample PMNS
> files don't depend on anything defined in stdpmid, so I'll remove that
> include from the examples.

OK, sounds good.

Comment 14 Nathan Scott 2015-03-18 05:48:41 UTC
(In reply to Paul Colby from comment #12)
> > 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 :)

Ah, good idea.

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

Yep, I think your approach there is correct.

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

I agree - these are benign, and there is value in having these files remain there.  Since this is a development package (and we've done the same thing in PCP itself), I'd recommend keeping it as-is.

It's all looking pretty good to me - Michael, Brad, did you guys see any remaining issues in the latest version?

cheers.

--
Nathan

Comment 15 Brad Hubbard 2015-03-18 23:36:48 UTC
Not sure I'm the best person to ask but it looks to me like you guys have worked out most of the kinks.

Comment 16 Michael Schwendt 2015-03-22 12:15:31 UTC
* The -examples package now needs an explicit dependency on "pcp", because that is the package that provides the /var/lib/pcp/pmdas directory.

The same bug affects the "pcp" packages: bug 1204467


> %package devel
> Requires: pcp-libs-devel

The explicit dependency on "pcp-libs-devel" would be safer (for the case when linking with any of the pcp shared libs), if it were arch-specific:

  Requires: pcp-libs-devel%{?_isa}

That is, because -devel packages and their dependencies are multilib packages:
# yum list pcp-libs-devel|grep ^pcp
pcp-libs-devel.i686                     3.10.3-1.fc21                    updates
pcp-libs-devel.x86_64                   3.10.3-1.fc21                    updates


* It doesn't build with latest GCC 5 in Rawhide:

[...]
/builddir/build/BUILD/pcp-pmda-cpp-0.4.2/pcp-pmda-cpp-0.4.2/example/simple/../../include/pcp-cpp/pmda.hpp: In member function 'virtual int pcp::pmda::on_store(pmResult*, pmdaExt*)':
/builddir/build/BUILD/pcp-pmda-cpp-0.4.2/pcp-pmda-cpp-0.4.2/example/simple/../../include/pcp-cpp/pmda.hpp:1135:38: error: suggest parentheses around operand of '!' or change '&' to '&&' or '!' to '~' [-Werror=parentheses]
                     if (!description.flags & pcp::storable_metric) {
                                      ^
In file included from /builddir/build/BUILD/pcp-pmda-cpp-0.4.2/pcp-pmda-cpp-0.4.2/example/simple/simple.cpp:18:0:
/builddir/build/BUILD/pcp-pmda-cpp-0.4.2/pcp-pmda-cpp-0.4.2/example/simple/../../include/pcp-cpp/pmda.hpp: In member function 'virtual int pcp::pmda::on_store(pmResult*, pmdaExt*)':
/builddir/build/BUILD/pcp-pmda-cpp-0.4.2/pcp-pmda-cpp-0.4.2/example/simple/../../include/pcp-cpp/pmda.hpp:1135:38: error: suggest parentheses around operand of '!' or change '&' to '&&' or '!' to '~' [-Werror=parentheses]
                     if (!description.flags & pcp::storable_metric) {
                                      ^
cc1plus: all warnings being treated as errors
[...]

Comment 17 Paul Colby 2015-03-24 10:33:13 UTC
> The -examples package now needs an explicit dependency on "pcp"

Done :)

> The explicit dependency on "pcp-libs-devel" would be safer ... if it were arch-specific

Done :)  I see now that this is a requirement (well, stated as "should" at least) in the official Packaging Guidelines too (https://fedoraproject.org/wiki/Packaging:Guidelines#Requires_2).

> It doesn't build with latest GCC 5 in Rawhide

Fixed.  Now builds, tests, etc on Fedora 22 Alpha 3.

Once again, I've updated the Spec and SRPM files linked at the top.

You can see all of the changes (not many) since the previous review Spec and SRPM files at:

https://github.com/pcolby/pcp-pmda-cpp/compare/5fde84b0f1468a8d7f4266e8a910b4eb74f13086...440a0fd3aa4182b037cf04818d8210513959d2ac

Thanks again!

Comment 18 Nathan Scott 2015-03-31 01:26:32 UTC
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #16)
> * The -examples package now needs an explicit dependency on "pcp", because
> that is the package that provides the /var/lib/pcp/pmdas directory.
> 
> The same bug affects the "pcp" packages: bug 1204467
> 

This has been fixed in the upstream pcp repo, will be tested further and Fedora update on next pcp release (pcp-3.10.4).  Thanks again.

cheers.

Comment 19 Nathan Scott 2015-03-31 01:27:17 UTC
(In reply to Paul Colby from comment #17)
> You can see all of the changes (not many) since the previous review Spec and
> SRPM files at:

Looks good to me Paul, thanks.

Comment 20 Nathan Scott 2015-04-08 23:28:35 UTC
Hi all,

This package appears to be in good shape now, from all the reviews so far - can we get this moved along to the next step of the new-Fedora-package process at this stage, or is further refinement needed?

Thanks!

Comment 21 Paul Colby 2015-04-22 05:58:00 UTC
I'm not sure how to progress this?  Any suggestions?

Thanks :)

Comment 22 Nathan Scott 2015-04-22 07:04:11 UTC
I think its waiting in the queue http://fedoraproject.org/PackageReviewStatus/ not sure there's anything else needing to be done on our end currently, Paul.

Comment 24 Paul Colby 2015-04-22 09:01:53 UTC
Thanks Michael.

Are you now happy now that the issues you raised above have been addressed? Or are you unable to re-review them now? (I was kind of waiting on your ack/nak before attempting any next steps).

Cheers.

Comment 25 Nathan Scott 2015-06-03 23:06:27 UTC
Marking reviewed (by myself and Michael Schwendt) - no further follow ups received and all earlier recommendations incorporated.

Comment 26 Michael Schwendt 2015-12-29 12:08:59 UTC
Running into this ticket again while cleaning up my filtered mail folders, I have no idea what's been going on in here.

By setting the fedora-review flag to '+', you've removed the ticket from the visible part of the needsponsor list at: http://fedoraproject.org/PackageReviewStatus/

Due to that, it's very difficult to find this ticket.

Only very few people browse the "Tickets under review" list and watch out for stalled reviews.


> Nathan Scott 2015-06-03 19:06:27 EDT
>
> Marking reviewed (by myself and Michael Schwendt) - no further follow ups
> received and all earlier recommendations incorporated.
> Flags: fedora-review+

This is unusual activity.

Normally, you would assign the ticket to yourself and set the fedora-review flag to '?' to show that you accept the role of the reviewer:

  https://fedoraproject.org/wiki/Package_Review_Process

Only when done with the official review, you would set the flag to '+'.

Further, until recently, you would have needed to be a member of the packagersponsors group to be able to review and approve this package. The recent change to the process is that everyone in packager group may review and approve packages from new contributors. Yet they still need to follow the How To Get Sponsored document.

Comment 27 Nathan Scott 2016-01-03 23:27:41 UTC
Hmm, OK, thanks Michael.  I've reset that unusual flags setting, hopefully that gets some additional eyeballs on this one.

Comment 28 Anthony Green 2016-10-12 03:37:31 UTC
Created attachment 1209426 [details]
Build log with errors on F24

Hi.  I'd like to see this in Fedora as well.  I've tried building the srpm but got errors.  See the attached build-log.txt.
Thanks!

Comment 29 Brad Hubbard 2016-10-12 04:30:41 UTC
Looks like it's missing https://github.com/pcolby/pcp-pmda-cpp/pull/29/commits/817e66f0ba5bf94cb2234216b33ebf86fba86bc0 which makes sense. let me see if I can get it working.

Comment 30 Brad Hubbard 2016-10-12 04:58:45 UTC
I applied the following patch to the spec file and it seems to build fine, in copr and koji, now that it's at the latest release. Not sure about the "Source" line below?

diff -up SPECS/pcp-pmda-cpp.spec.fix SPECS/pcp-pmda-cpp.spec
--- SPECS/pcp-pmda-cpp.spec.fix 2016-10-12 14:45:52.970266334 +1000
+++ SPECS/pcp-pmda-cpp.spec     2016-10-12 14:39:20.601300196 +1000
@@ -8,11 +8,11 @@
 
 Summary: PMDA++ Library
 Name: pcp-pmda-cpp
-Version: 0.4.2
+Version: 0.4.3
 Release: 1%{?dist}
 License: Boost
 Group: System Environment/Libraries
-Source: https://github.com/pcolby/%{name}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz
+Source: https://github.com/pcolby/%{name}/archive/v%{version}.tar.gz
 URL: https://github.com/pcolby/%{name}
 
 BuildRequires: boost-devel >= 1.32



I uploaded the new spec file here: http://people.redhat.com/bhubbard/pcp-pmda-cpp.spec
srpm: http://people.redhat.com/bhubbard/pcp-pmda-cpp-0.4.3-1.fc24.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=16057195
Copr repo: https://copr.fedorainfracloud.org/coprs/badone/pcp-pmda-cpp/build/464191/

Hope this is OK? Didn't mean to hijack the show...

Comment 31 Paul Colby 2016-10-12 05:23:01 UTC
> I applied the following patch to the spec file

Thanks Brad.  Yep, that should match the 0.4.3 release spec file (https://github.com/pcolby/pcp-pmda-cpp/blob/v0.4.3/package/rpm/pcp-pmda-cpp.spec).

I can't see how to edit the "description" to update the Spec URL (will look again in a minute - I know I've done it in the past, but its been a while).

> Not sure about the "Source" line below?

In the past, I've built source RPM via the make-src-rpm.sh script (https://github.com/pcolby/pcp-pmda-cpp/blob/v0.4.3/package/rpm/make-src-rpm.sh), which just does some sanity checking and validation around packaging up the release source code.  Then I upload it to the GitHub release manually.  I see that I haven't done that for the 0.4.3 release yet, and I don't have a Fedora build machine handy at the moment.  Will try to build the SRPM shortly.

> Hope this is OK? Didn't mean to hijack the show...

Perfectly OK with me :)

Comment 32 Paul Colby 2016-10-12 05:49:59 UTC
I've added the *.src.rpm to the 0.4.3 release on GitHub, so the latest / current Spec and SRPM URLS should be:

https://github.com/pcolby/pcp-pmda-cpp/blob/v0.4.3/package/rpm/pcp-pmda-cpp.spec

and

https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.3/pcp-pmda-cpp-0.4.3-1.src.rpm

respectively.

Though I still can't see any way to update the top-level post's URLs, which I used to do in the past...?

Comment 33 Brad Hubbard 2016-10-12 05:51:14 UTC
(In reply to Paul Colby from comment #31)
> > I applied the following patch to the spec file
> 
> Thanks Brad.  Yep, that should match the 0.4.3 release spec file
> (https://github.com/pcolby/pcp-pmda-cpp/blob/v0.4.3/package/rpm/pcp-pmda-cpp.
> spec).

yw.

The Source: URL looks odd and seems to make rpmbuild look for an archive called %{name}-%{version}.tar.gz in SOURCES, are you sure it's right (see my patch)?

Source: https://github.com/pcolby/%{name}/archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz

> 
> I can't see how to edit the "description" to update the Spec URL (will look
> again in a minute - I know I've done it in the past, but its been a while).

You can't. I just create a new comment with the updated links generally.

Comment 34 Paul Colby 2016-10-12 05:56:17 UTC
> The Source: URL looks odd

Yes, it is a little odd ;) But its intentional; see https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Git_Hosting_Services

Updated URLs (since I forgot make the above Spec URL raw):

Spec URL: https://raw.githubusercontent.com/pcolby/pcp-pmda-cpp/v0.4.3/package/rpm/pcp-pmda-cpp.spec
SRPM URL: https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.3/pcp-pmda-cpp-0.4.3-1.src.rpm

Thanks :)

Comment 35 Brad Hubbard 2016-10-12 06:41:59 UTC
(In reply to Paul Colby from comment #34)
> > The Source: URL looks odd
> 
> Yes, it is a little odd ;) But its intentional; see
> https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/
> SourceURL#Git_Hosting_Services

Ah, the penny drops, I see where/how I got confused now. Maybe the following would be better to download the tar archive in your script?

$ spectool -R -g SPECS/pcp-pmda-cpp.spec

> 
> Updated URLs (since I forgot make the above Spec URL raw):
> 
> Spec URL:
> https://raw.githubusercontent.com/pcolby/pcp-pmda-cpp/v0.4.3/package/rpm/pcp-
> pmda-cpp.spec
> SRPM URL:
> https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.3/pcp-pmda-cpp-
> 0.4.3-1.src.rpm
> 
> Thanks :)

Comment 36 Paul Colby 2016-10-28 07:39:10 UTC
(In reply to Nathan Scott from comment #27)
> Hmm, OK, thanks Michael.  I've reset that unusual flags setting, hopefully
> that gets some additional eyeballs on this one.

Hi Nathan.  It seems that did not actually work, as this ticket is being listed (I assume incorrectly) on the "Tickets under review" page (http://fedoraproject.org/PackageReviewStatus/REVIEW.html), with assignee "(Nobody)".

Based on Michael Schwendt comments above (https://bugzilla.redhat.com/show_bug.cgi?id=1199693#c26), as well as https://fedoraproject.org/wiki/Package_Review_Process as I see it either:

1. the fedora review flag should be cleared, indicating that this ticket is back to needing sponsoring (in which case the assignee stays blank until a sponsor is found); or

2. the ticket should be assigned to you (assuming you're willing / empowered to review it), and the flag left as "?" to indicate that you are the current reviewer; or

3. change the flag to "+" to indicate that review has passed (if it has). In which case I'm not sure what the assignee should be (the docs are unclear).

I'm perfectly happy for you (or someone with appropriate authority) to change the ticket to any of those states, and I'll work to progress it from there.  But as it is, I suspect its still in a not-quite-valid state as far as the Package Review Process is defined.

Thanks :)

Comment 37 Nathan Scott 2016-10-28 07:43:03 UTC
Hi Paul,

I'm literally walking out the door to go on holiday :| ... I've CC'd Mark who may be able to help here in my absence.  I tend to agree though - something's not right with this BZ - should be long-since resolved.  :(

cheers.

Comment 38 Michael Schwendt 2016-10-28 15:18:21 UTC
You could have cleared the fedora-review flag a long time ago after the explanation in earlier comments.

Comment 39 Paul Colby 2016-10-28 23:44:27 UTC
Thanks Michael :)

Back to square one, but that's a whole lot better than being on a non-existent / invalid square ;)

Now to find a sponsor...

Comment 40 Paul Colby 2017-10-14 09:14:05 UTC
Update to PMDA++ 0.4.4, which fixes a few issues with modern compilers, and more recent versions of PCP, such as found on Fedora 26.

Spec URL: https://raw.githubusercontent.com/pcolby/pcp-pmda-cpp/v0.4.4/package/rpm/pcp-pmda-cpp.spec
SRPM URL: https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.4/pcp-pmda-cpp-0.4.4-1.src.rpm

Comment 41 Lukas Berk 2017-12-08 19:31:57 UTC
This is a review *template*. Besides handling the [ ]-marked tests you are
also supposed to fix the template before pasting into bugzilla:
- Add issues you find to the list of issues on top. If there isn't such
  a list, create one.
- Add your own remarks to the template checks.
- Add new lines marked [!] or [?] when you discover new things not
  listed by fedora-review.
- Change or remove any text in the template which is plain wrong. In this
  case you could also file a bug against fedora-review
- Remove the "[ ] Manual check required", you will not have any such lines
  in what you paste.
- Remove attachments which you deem not really useful (the rpmlint
  ones are mandatory, though)
- Remove this text



Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Header files in -devel subpackage, if present.
  Note: pcp-pmda-cpp-examples : /var/lib/pcp/pmdas/pcp-pmda-cpp-
  examples/pmdas/simple/domain.h pcp-pmda-cpp-examples : /var/lib/pcp/pmdas
  /pcp-pmda-cpp-examples/pmdas/simplecpu/domain.h pcp-pmda-cpp-examples :
  /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/trivial/domain.h
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages

These files are required for pmda functionality in PCP.


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "BSL (v1.0)", "*No copyright* BSL", "Unknown or generated". 70
     files have unknown license. Detailed output of licensecheck in
     /home/lberk/src/fedora-scm/review/pcp-pmda-cpp/licensecheck.txt

     The only "no copyright" file is the license itself,  all other files are test files and generated.

[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required
[x]: Sources contain only permissible code or content.
[x]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Buildroot is not present
     Note: Buildroot: present but not needed

     Buildroot portions included for el5 era building, not applicable for modern
     fedora/epel

[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in pcp-
     pmda-cpp-devel , pcp-pmda-cpp-examples
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.


Rpmlint
-------
Checking: pcp-pmda-cpp-devel-0.4.4-1.fc28.x86_64.rpm
          pcp-pmda-cpp-examples-0.4.4-1.fc28.x86_64.rpm
          pcp-pmda-cpp-0.4.4-1.fc28.src.rpm
pcp-pmda-cpp-examples.x86_64: W: only-non-binary-in-usr-lib
pcp-pmda-cpp-examples.x86_64: W: no-documentation
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simple/domain.h
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simple/simple.cpp
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simplecpu/domain.h
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simplecpu/simplecpu.cpp
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/trivial/domain.h
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/trivial/trivial.cpp
3 packages and 0 specfiles checked; 0 errors, 8 warnings.




Rpmlint (debuginfo)
-------------------
Checking: pcp-pmda-cpp-examples-debuginfo-0.4.4-1.fc28.x86_64.rpm
pcp-pmda-cpp-examples-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
1 packages and 0 specfiles checked; 1 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
pcp-pmda-cpp-examples-debuginfo.x86_64: W: invalid-url URL: https://github.com/pcolby/pcp-pmda-cpp <urlopen error [Errno -2] Name or service not known>
pcp-pmda-cpp-examples-debuginfo.x86_64: E: useless-provides debuginfo(build-id)
pcp-pmda-cpp-devel.x86_64: W: invalid-url URL: https://github.com/pcolby/pcp-pmda-cpp <urlopen error [Errno -2] Name or service not known>
pcp-pmda-cpp-examples.x86_64: W: invalid-url URL: https://github.com/pcolby/pcp-pmda-cpp <urlopen error [Errno -2] Name or service not known>
pcp-pmda-cpp-examples.x86_64: W: only-non-binary-in-usr-lib
pcp-pmda-cpp-examples.x86_64: W: no-documentation
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simple/domain.h
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simple/simple.cpp
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simplecpu/domain.h
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/simplecpu/simplecpu.cpp
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/trivial/domain.h
pcp-pmda-cpp-examples.x86_64: W: devel-file-in-non-devel-package /var/lib/pcp/pmdas/pcp-pmda-cpp-examples/pmdas/trivial/trivial.cpp
3 packages and 0 specfiles checked; 1 errors, 11 warnings.



Requires
--------
pcp-pmda-cpp-devel (rpmlib, GLIBC filtered):
    pcp-libs-devel(x86-64)

pcp-pmda-cpp-examples (rpmlib, GLIBC filtered):
    /bin/sh
    libboost_program_options.so.1.64.0()(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.4)(64bit)
    libm.so.6()(64bit)
    libpcp.so.3()(64bit)
    libpcp.so.3(PCP_3.0)(64bit)
    libpcp_pmda.so.3()(64bit)
    libpcp_pmda.so.3(PCP_PMDA_3.0)(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    pcp
    rtld(GNU_HASH)



Provides
--------
pcp-pmda-cpp-devel:
    pcp-pmda-cpp-devel
    pcp-pmda-cpp-devel(x86-64)
    pcp-pmda-cpp-static

pcp-pmda-cpp-examples:
    pcp-pmda-cpp-examples
    pcp-pmda-cpp-examples(x86-64)



Source checksums
----------------
https://github.com/pcolby/pcp-pmda-cpp/archive/v0.4.4.tar.gz#/pcp-pmda-cpp-0.4.4.tar.gz :
  CHECKSUM(SHA256) this package     : 1141fee2eb3c7605bb6ac7ec46123203f4590bcb112b1548226e6c5253ef0481
  CHECKSUM(SHA256) upstream package : 1141fee2eb3c7605bb6ac7ec46123203f4590bcb112b1548226e6c5253ef0481


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/bin/fedora-review -m fedora-rawhide-x86_64 -rn ./pcp-pmda-cpp-0.4.4-1.src.rpm
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 42 Gwyn Ciesla 2018-01-02 13:48:15 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/pcp-pmda-cpp

Comment 43 Mattia Verga 2020-06-13 15:56:36 UTC
This package was approved and imported in repositories, but this review ticket was never closed.
I'm closing it now.