https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Bug ID: 1199693 Summary: Review Request: pcp-pmda-cpp - C++ library for PCP PMDAs Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: redhat@colby.id.au QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://raw.githubusercontent.com/pcolby/pcp-pmda-cpp/v0.4.2/package/rpm/pcp... SRPM URL: https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.2/pcp-pmda-cpp...
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
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Paul Colby redhat@colby.id.au changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #1 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net --- 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).
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #2 from Paul Colby redhat@colby.id.au --- Thanks Michael. I'll take a look at the fedora-review tool, and fix whatever it finds.
Thanks again :)
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |nathans@redhat.com
--- Comment #3 from Nathan Scott nathans@redhat.com --- Created attachment 1000797 --> https://bugzilla.redhat.com/attachment.cgi?id=1000797&action=edit Workaround for build issues with cmake and overriding optimisation
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |bhubbard@redhat.com
--- Comment #4 from Nathan Scott nathans@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #5 from Paul Colby redhat@colby.id.au --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #6 from Nathan Scott nathans@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #7 from Paul Colby redhat@colby.id.au ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #8 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net ---
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_Li...
%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_Owner... 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.]
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #9 from Paul Colby redhat@colby.id.au --- Thanks Michael, some really good stuff there! :D
It's going to take me a day or two to work through it all :)
Cheers.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #10 from Paul Colby redhat@colby.id.au --- 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_simp...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #11 from Paul Colby redhat@colby.id.au --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #12 from Paul Colby redhat@colby.id.au ---
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_Li...
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/838957be550f0826179cc3cd16ab6...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #13 from Nathan Scott nathans@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #14 from Nathan Scott nathans@redhat.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #15 from Brad Hubbard bhubbard@redhat.com --- Not sure I'm the best person to ask but it looks to me like you guys have worked out most of the kinks.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #16 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net --- * 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 [...]
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #17 from Paul Colby redhat@colby.id.au ---
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/5fde84b0f1468a8d7f4266e8a910b...
Thanks again!
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #18 from Nathan Scott nathans@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #19 from Nathan Scott nathans@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?
--- Comment #20 from Nathan Scott nathans@redhat.com --- 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!
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #21 from Paul Colby redhat@colby.id.au --- I'm not sure how to progress this? Any suggestions?
Thanks :)
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo? |
--- Comment #22 from Nathan Scott nathans@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #23 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net --- Well, there's
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group [from https://fedoraproject.org/wiki/Category:Package_Maintainers ]
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #24 from Paul Colby redhat@colby.id.au --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |brolley@redhat.com, | |lberk@redhat.com Doc Type|Bug Fix |Enhancement Flags| |fedora-review+
--- Doc Text *updated* --- 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.
--- Comment #25 from Nathan Scott nathans@redhat.com --- Marking reviewed (by myself and Michael Schwendt) - no further follow ups received and all earlier recommendations incorporated.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #26 from Michael Schwendt bugs.michael@gmx.net --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review+ |fedora-review?
--- Comment #27 from Nathan Scott nathans@redhat.com --- Hmm, OK, thanks Michael. I've reset that unusual flags setting, hopefully that gets some additional eyeballs on this one.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #28 from Anthony Green green@redhat.com --- Created attachment 1209426 --> https://bugzilla.redhat.com/attachment.cgi?id=1209426&action=edit 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!
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #29 from Brad Hubbard bhubbard@redhat.com --- Looks like it's missing https://github.com/pcolby/pcp-pmda-cpp/pull/29/commits/817e66f0ba5bf94cb2234... which makes sense. let me see if I can get it working.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #30 from Brad Hubbard bhubbard@redhat.com --- 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/%%7Bname%7D/archive/v%%7Bversion%7D.tar.gz#/%%7Bna... +Source: https://github.com/pcolby/%%7Bname%7D/archive/v%%7Bversion%7D.tar.gz URL: https://github.com/pcolby/%%7Bname%7D
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...
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #31 from Paul Colby redhat@colby.id.au ---
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....).
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....), 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 :)
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #32 from Paul Colby redhat@colby.id.au --- 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....
and
https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.3/pcp-pmda-cpp...
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...?
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #33 from Brad Hubbard bhubbard@redhat.com --- (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/%%7Bname%7D/archive/v%%7Bversion%7D.tar.gz#/%%7Bna...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #34 from Paul Colby redhat@colby.id.au ---
The Source: URL looks odd
Yes, it is a little odd ;) But its intentional; see https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Gi...
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... SRPM URL: https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.3/pcp-pmda-cpp...
Thanks :)
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #35 from Brad Hubbard bhubbard@redhat.com --- (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 :)
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #36 from Paul Colby redhat@colby.id.au --- (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 :)
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mgoodwin@redhat.com
--- Comment #37 from Nathan Scott nathans@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Michael Schwendt bugs.michael@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |
--- Comment #38 from Michael Schwendt bugs.michael@gmx.net --- You could have cleared the fedora-review flag a long time ago after the explanation in earlier comments.
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #39 from Paul Colby redhat@colby.id.au --- 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...
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #40 from Paul Colby redhat@colby.id.au --- 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... SRPM URL: https://github.com/pcolby/pcp-pmda-cpp/releases/download/v0.4.4/pcp-pmda-cpp...
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Lukas Berk lberk@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |lberk@redhat.com Flags| |fedora-review+
--- Comment #41 from Lukas Berk lberk@redhat.com ---
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... : 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
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
--- Comment #42 from Gwyn Ciesla limburgher@gmail.com --- (fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/pcp-pmda-cpp
https://bugzilla.redhat.com/show_bug.cgi?id=1199693
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com Blocks|177841 (FE-NEEDSPONSOR) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com -
Red Hat Bugzilla