Bug 877096 - Review Request: perl-Fsdb - A set of commands for manipulating flat-text databases from the shell
Summary: Review Request: perl-Fsdb - A set of commands for manipulating flat-text data...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Petr Šabata
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-11-15 16:56 UTC by John Heidemann
Modified: 2015-07-20 16:43 UTC (History)
5 users (show)

Fixed In Version: perl-Fsdb-2.50-1.fc19
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-06-17 23:33:23 UTC
Type: ---
Embargoed:
psabata: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
updated perl-Fsdb.spec wrt comment 20/21 (3.13 KB, text/x-rpm-spec)
2013-10-17 20:07 UTC, John Heidemann
no flags Details
updated perl-Fsdb.spec wrt comment 23 to comment 25 (3.48 KB, text/x-rpm-spec)
2013-12-10 00:13 UTC, John Heidemann
no flags Details

Description John Heidemann 2012-11-15 16:56:21 UTC
Spec URL: http://www.isi.edu/~johnh/SOFTWARE/FSDB/Fsdb.spec
SRPM URL: http://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb-2.28-1.src.rpm
Description: A set of commands for manipulating flat-text databases from the shell
Fedora Account System Username: johnh

Detailed package description is at http://www.isi.edu/~johnh/SOFTWARE/FSDB/

Comment 1 John Heidemann 2013-02-22 00:42:50 UTC
This bug is marked "needsponser".  What is the mechanism to get that resolved?

Comment 2 Petr Šabata 2013-02-22 09:09:02 UTC
(In reply to comment #1)
> This bug is marked "needsponser".  What is the mechanism to get that
> resolved?

This might interest you:
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Comment 3 Christopher Meng 2013-04-19 01:50:30 UTC
In my opinion, I think you should use cpanspec and create a spec from cpan.org.

Explicitly, I think the URL and source should be filled like this(Just an example):

URL:            http://search.cpan.org/dist/Fsdb/
Source0:        http://search.cpan.org/CPAN/authors/id/J/JO/JOHNH/Fsdb-%{version}.tar.gz


And, the %changelog has incorrect version with %{version}.

Also,if you want to maintain a spec for EPEL also, you can keep Group and %defattr(-,root,root,-) and %clean, otherwise you should remove them.

Comment 4 John Heidemann 2013-05-10 06:17:11 UTC
Thanks for the tips in comment #3.

I ran cpanspec.  It had some dependencies I picked up; they'll be in Fsdb-2.39 (not yet out).

But my website is the upstream, not CPAN,
and it's not clear to me what the %changelog problem is
(although I don't actually track changes with that since I maintain the spec and the software).

Comment 5 Christopher Meng 2013-07-24 01:25:57 UTC
ping.

Are you still interested in this package?

Comment 6 John Heidemann 2013-07-24 06:15:39 UTC
Yes, I'm happy to maintain this package, in that we maintain it internally for our research group, we run Fedora, and I am the package's lead developer.

But the guidelines for getting sponsored are a bit vague ("hang out on irc").  I think I submit reasonable bug reports if you want to assess my clue level.  It's unclear (to me) how I should proceed.

Comment 7 Michael Schwendt 2013-07-26 08:33:09 UTC
Please run rpmlint (or rpmlint -I) on the src.rpm *and* all built rpms. Feel free to ignore obvious false positives, but fix reported issues and/or comment on them.
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines


> Release: 1

Just to mention its existance:
https://fedoraproject.org/wiki/Packaging:DistTag

Depending on which build targets you'd like to release this package for, you could remove a few old items from the spec file.
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean


> OPTIMIZE="$RPM_OPT_FLAGS"

$RPM_OPT_FLAGS and %optflags are C/C++ compiler flags and don't apply to noarch Perl packages.


> find $RPM_BUILD_ROOT -type d -depth -exec rmdir {} 2>/dev/null ';'

Not needed, pretty much useless (since empty dirs in buildroot don't cause any build error) and more of a hindrance in packages where you would like to include an empty directory actually.


> %defattr(-,root,root,-)

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


> it's not clear to me what the %changelog problem is

The %changelog contains an entry for 2.28-1 whereas the linked spec file says 2.40-1.


> But the guidelines for getting sponsored are a bit vague ("hang out on irc").

Well, that's just one way that works for some people to get to know other package maintainers and potential sponsors. IRC can be time-consuming and is not mandatory. Doing a few reviews can be one way to show that you know Fedora's Packaging Guidelines (and especially where to find them when it becomes necessary to look them up):
http://fedoraproject.org/PackageReviewStatus/
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Reviewing_packages

Comment 8 John Heidemann 2013-07-26 18:18:41 UTC
rpmlint found some problems; I'll fix them in the next release.

Thanks for the pointers wrt optimize/find/etc.  There's some level of cargo cultery there (rpm documentation is not always obvious).

I was hoping to avoid %changelog since it has nothing useful to say, since I am both developer and packager.  But I can fill it with low-content but rpmlint-satisfying entries "updated for version x".  (I realize in most cases developer and packager are different parties.)

Comment 9 John Heidemann 2013-07-29 21:23:32 UTC
All of the problems mentioned in comment 7, plus most other rpmlint errors, are now fixed in release 2.41
at http://www.isi.edu/~johnh/SOFTWARE/FSDB/

I have this rpmlint error on the srpm:

  perl-Fsdb.src: E: invalid-spec-name

which seems puzzling to me, as the only contents of that rpm are:

  Fsdb-2.41.tar.gz
  Fsdb.spec

and that spec looks valid to me.  (Unless maybe the spec need to be perl-Fsdb.spec?)

Comment 10 Christopher Meng 2013-07-29 22:42:50 UTC
Can you provide the links? 

I don't know the problem if you just talk.

Comment 11 John Heidemann 2013-07-29 22:51:02 UTC
I quoted the error message in comment 9, but if you get:

Spec URL: http://www.isi.edu/~johnh/SOFTWARE/FSDB/Fsdb.spec
SRPM URL: http://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb-2.41-1.src.rpm

  bash> rpmlint perl-Fsdb-2.41-1.fc19.src.rpm
  perl-Fsdb.src: W: spelling-error %description -l en_US outliers -> outlines
  perl-Fsdb.src: E: invalid-spec-name
  1 packages and 0 specfiles checked; 1 errors, 1 warnings.

Comment 12 John Heidemann 2013-07-29 22:52:23 UTC
Sorry, wrong url (cut and paste error).
It should be:

SRPM URL: http://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb-2.41-1.fc19.src.rpm

Comment 13 Christopher Meng 2013-07-29 23:46:17 UTC
Aha of course it's wrong...

In your SRPM:

perl-Fsdb.spec, not Fsdb.spec...

Comment 14 John Heidemann 2013-07-30 00:24:01 UTC
Wrt comment 13: you're correct.  Will be fixed in next release.

I believe then it rpmlints without error, and with only one warning about an incorrect spelling error.

Are there any other changes needed to get this package accepted?

Comment 15 Christopher Meng 2013-07-30 00:43:53 UTC
I don't know(haven't looked into very deep.)

You can leave these to your sponsor.

I suggest that you should get in touch with perl SIG people, maybe they can sponsor you. Not every sponsor knows perl very well.

Ah..Michael, will you do him a favor?

Comment 16 Michael Schwendt 2013-07-31 10:02:16 UTC
Hmm, I don't even maintain a Perl based package anymore. ;-)

Comment 17 Petr Šabata 2013-10-17 02:46:06 UTC
Ok, I'll take a look at this.

Comment 18 Petr Šabata 2013-10-17 02:49:10 UTC
Could you please provide current SPEC and SRPM links?
I see perl-Fsdb.spec is there, using version 2.47-1, however there is no SRPM.

Comment 19 John Heidemann 2013-10-17 04:33:38 UTC
Thanks, Petr.

A current SRPM is:
https://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb-2.47-1.fc19.src.rpm

The spec is in the SRPM, or at
https://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb.spec

(Sigh, there's an rpmlint warning about mixing spaces and tabs in the spec that I will fix in the next release.)

Comment 20 Petr Šabata 2013-10-17 08:40:05 UTC
There's no such package as perl-Jdb or anything providing perl(Jdb); could you explain that obsoletes/provides?

Comment 21 John Heidemann 2013-10-17 20:05:59 UTC
Wrt comment #20, perl-Jdb was an earlier version of Fsdb.
The name changed in 2008.
We packaged it for internal use. 
Since it was not publicly released, and is now very old,
I will remove those lines in the next release.

Comment 22 John Heidemann 2013-10-17 20:07:48 UTC
Created attachment 813534 [details]
updated perl-Fsdb.spec wrt comment 20/21

Comment 23 Petr Šabata 2013-10-18 10:00:55 UTC
Ok, I'll repeat some of the things that have already been mentioned but are still in the spec.

Just remove the old cruft unless you really want to support this package in EPEL5.  That means the BuildRoot tag (line 10), buildroot removal (line 58), and the %clean section (lines 72-73).

As this is a noarch package, you don't need line 61.  Remove it.

I think the lines 64 and 65 could be replaced with simple %{_fixperms} %{buildroot}/*.

EE::MM supports DESTDIR and using that is preferred over PERL_INSTALL_ROOT.

Now about the dependencies:
You have to BuildRequire everything used during the build phase which isn't available in the minimal buildroot (see the Packaging Guidelines for the list).  This, of course, includes all the perl modules used by the executed code.
When it comes to runtime dependencies, rpmbuild tries to parse the code present in the final RPM and generate the dependency list from that.  It's fairly smart but has some known issues.  I'd recommend not explicitly Requiring anything, building the package, checking the detected dependencies with 'rpm -qRp package.rpm', and just adding what's missing.

In your case, the following BRs are required (Makefile.PL, executed scripts, testsuite, and the tested code):
perl
perl(Carp)
perl(Config)
perl(Exporter)
perl(ExtUtils::MakeMaker)
perl(File::Copy)
perl(Getopt::Long)
perl(IO::File)
perl(IO::Handle)
perl(IO::Uncompress::AnyUncompress)
perl(Pod::Usage)
perl(strict)
perl(Test::More)
perl(vars)

The IO::Compress::, Text::CSV_XS, and HTML::Parser modules don't seem to be required at build time.  You should drop those.  Both Text::CSV_XS and HTML::Parser will most likely be autodetected by rpmbuild, too.  No need to Require them explicitly.

Comment 24 John Heidemann 2013-10-23 06:10:22 UTC
Thank you for the suggestions in comment 23.  I will update the spec again.  I thought I had picked up all prior comments, but maybe not.

A couple of things though:

- yes I do want to support EL5; we have some old boxes here

- wrt IO::Compress::, etc. not being reuqired at build time: they should be exercised in the test suites

I will post an updated spec when I can check all these out.

Comment 25 Petr Šabata 2013-10-24 06:12:21 UTC
(In reply to John Heidemann from comment #24)
> Thank you for the suggestions in comment 23.  I will update the spec again. 
> I thought I had picked up all prior comments, but maybe not.
> 
> A couple of things though:
> 
> - yes I do want to support EL5; we have some old boxes here

Alright, keep the buildroot and the related things in there then.

> - wrt IO::Compress::, etc. not being reuqired at build time: they should be
> exercised in the test suites

Those are only loaded in the _enable_compression() sub which doesn't seem to be called during the test phase.  But I might be wrong.

> I will post an updated spec when I can check all these out.

Ok :)

Comment 26 John Heidemann 2013-12-10 00:13:44 UTC
Created attachment 834601 [details]
updated perl-Fsdb.spec wrt comment 23 to comment 25

Comment 27 John Heidemann 2014-01-04 00:41:32 UTC
A new release is out:
https://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb-2.48-1.fc20.src.rpm

The spec is in the SRPM, or at
https://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb.spec

This includes the updated spec (see comment 26), fixes some small bugs, and adds an =encoding to work with perl-5.18 in Fedora-20 (due to someone's name with a unicode character in it :-).

Unfortunately I just noticed it gets rpmlint errors "non-standard-dir-perm".  This is because the suggestion in comment 23 about using %{_fixperms} is not sufficiently strongly---fixperms sets many things, but doesn't include g-s for the directories.

Please let me know if this is sufficient to address the concerns needed to unblock this package.

Comment 28 John Heidemann 2014-05-20 05:54:51 UTC
Folks, I think spec issues have been addressed.  Can we move this package forward?

Comment 29 Petr Šabata 2014-05-20 08:21:45 UTC
Oh, I've missed the update.
I'll take a look at the new version.

Comment 30 Petr Šabata 2014-05-23 12:31:27 UTC
Ok, this is a different version, so going through it again...

Notes on %description:
- s/FSDB is package/FSDB is a package/
- I'd cut it at the "Although it's often easy" sentence.

Missing BRs:
perl(utf8)
perl(warnings)

Unneeded BRs:
perl(HTML::Parser) is not used during build
perl(Text::CSV_XS) is not used during build
perl(vars) is not used used during build (anymore?)

Unneeded runtime dependencies:
perl(HTML::Parser) is detected and added automatically by rpmbuild
perl(Test::More) is not needed at runtime at all
perl(Text::CSV_XS) is detected and added automatically by rpmbuild

Comment 31 John Heidemann 2014-05-28 05:12:25 UTC
Ok, thank you.  Changes in comment #30 made.
New release at
https://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb-2.50-1.fc20.src.rpm

The spec is in the SRPM, or at
https://www.isi.edu/~johnh/SOFTWARE/FSDB/perl-Fsdb.spec

Your turn again.

Comment 32 Petr Šabata 2014-05-29 15:03:13 UTC
Alright, looks good enough now, approving.
I've also just sponsored you into the Packager group.  Welcome aboard.

Comment 33 Petr Šabata 2014-05-29 15:05:32 UTC
You may now submit the SCM request for this package.  See the following guide:
http://fedoraproject.org/wiki/Package_SCM_admin_requests

Also, since this is a perl package, add `perl-sig' to `InitialCC', please.

Comment 34 John Heidemann 2014-05-29 18:15:52 UTC
Thanks!

New Package SCM Request
=======================
Package Name: perl-Fsdb
Short Description:  A set of commands for manipulating flat-text databases from the shell
Upstream URL: https://www.isi.edu/~johnh/SOFTWARE/FSDB/Fsdb-2.50.tar.gz
Owners: johnh
Branches: f19 f20 epel7
InitialCC: perl-sig

Comment 35 Gwyn Ciesla 2014-06-04 10:04:04 UTC
Git done (by process-git-requests).

Comment 36 Fedora Update System 2014-06-06 00:14:26 UTC
perl-Fsdb-2.50-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/perl-Fsdb-2.50-1.fc20

Comment 37 Fedora Update System 2014-06-07 16:18:47 UTC
perl-Fsdb-2.50-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/perl-Fsdb-2.50-1.fc19

Comment 38 Fedora Update System 2014-06-10 03:06:26 UTC
perl-Fsdb-2.50-1.fc19 has been pushed to the Fedora 19 testing repository.

Comment 39 Fedora Update System 2014-06-17 23:33:23 UTC
perl-Fsdb-2.50-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 40 Fedora Update System 2014-06-17 23:35:23 UTC
perl-Fsdb-2.50-1.fc19 has been pushed to the Fedora 19 stable repository.

Comment 41 John Heidemann 2015-05-21 18:31:03 UTC
Package Change Request
======================
Package Name: perl-Fsdb
New Branches: el6
Owners: johnh
InitialCC: perl-sig

I maintain
this package against EPEL7 and want to get into the older RHEL6 tree.
(RHEL5, you are dead to me, but RHEL6 lives on.)

Comment 42 Kevin Fenzi 2015-07-20 16:43:26 UTC
Git done (by process-git-requests).


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