Bug 876409 - Review Request: perl-Growl-GNTP - Perl implementation of GNTP Protocol (Client Part)
Review Request: perl-Growl-GNTP - Perl implementation of GNTP Protocol (Clien...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michal Ingeli
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 890839
  Show dependency treegraph
 
Reported: 2012-11-13 22:06 EST by Miro Hrončok
Modified: 2013-01-02 09:36 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-29 01:36:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mi: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Miro Hrončok 2012-11-13 22:06:06 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Growl-GNTP.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Growl-GNTP-0.15-1.fc17.src.rpm
Description: Perl implementation of GNTP Protocol (Client Part)
Fedora Account System Username: churchyard
Comment 1 Michal Ingeli 2012-11-15 06:17:55 EST
Picking this one, since it looks like we are working on/packaging same thing.

* spec file clean, in american english
* license ok
* source matches upstream, clean without binaries
* rpmlint silent, package builds

- many BR are redundant and useless, while most of them is pulled in by perl or automatically generated by rpmbuild. list only what you really need to build (in mock).
- is there anything to filter out with this?
  %{?perl_default_filter} # Filters (not)shared c libs
- use dos2unix BR rather than this:
  sed -i "s/\r//" README
- specfile is not consistent with provided srpm
Comment 2 Miro Hrončok 2012-11-15 12:23:21 EST
> many BR are redundant and useless, while most of them is pulled in by perl or automatically generated by rpmbuild. list only what you really need to build (in mock).

I've noticed Requires are autogenerated, but how this works with BR? I supposed the autogeneration is done during the build procces, while chcecking if BRs are present is done before the build.

May I simply remove BRs provided by the perl package?
Comment 3 Michal Ingeli 2012-11-16 07:38:24 EST
Blah, you are right (should have corrected myself after correcting myself). Requires are only auto generated for install, not for build. Anyway, don't pull pull in explicitly packages provided by perl itself. cpanspec usually generates sane BR list. Test with minimum, in mock, if it builds and tests are run.
Comment 4 Miro Hrončok 2012-11-18 09:28:55 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Growl-GNTP.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Growl-GNTP-0.15-2.fc17.src.rpm

- Removed BRs provided by perl package
- Removed useless perl autofilter
- Using dos2unix
Comment 5 Michal Ingeli 2012-11-19 10:08:43 EST
Still, you don't need to require each module used at runtime, just to build a package.

I was able to build this package with as minimum as this:

BuildRequires:  perl(Crypt::CBC) >= 2.29
BuildRequires:  perl(Data::UUID) >= 0.149
BuildRequires:  perl(Digest::MD5) >= 2.36
BuildRequires:  perl(Digest::SHA) >= 5.45
BuildRequires:  perl(ExtUtils::MakeMaker)
BuildRequires:  perl(ExtUtils::MM_Unix)

Another thing is, there are bundled modules of Module::Install in 'inc/' added by author via Module::Install. You should remove this and use Module::Install from fedora. 
In this phase, there will be missing Module::Install::AuthorTests (not in repositories) and probably others (that are not listed above). Or you can disable author_tests in Makefile.PL. This tests are not run anyway.
Comment 6 Miro Hrončok 2012-11-19 10:19:17 EST
I was told I need all modules in BR, so tests can run. How it works?
Comment 7 Michal Ingeli 2012-11-19 11:35:51 EST
The only test that will run, in this case, is t/00 (simple inclusion). t/01 will fail softly, as there is no growl backend to send to. xt are not run, because you are not "author".

Build in mock, look into build output in build.log.

So remove 'inc' directory, and see, where that will lead; what packages you will (really) need. I would consider it optional, whether you want to run author_tests or not. As of f18 there is perl(Module::Install::AuthorTests), #809931.
Comment 8 Miro Hrončok 2012-11-19 13:20:03 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Growl-GNTP.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Growl-GNTP-0.15-3.fc17.src.rpm

- Removed local inc and xs directories
- Patched source so it doesn't need them
- Removed lots of BR (builds in mock)
Comment 9 Michal Ingeli 2012-11-20 05:31:12 EST
It's perl(Module::Install), not perl(inc::Module::Install) (provided by the same package).

Otherwise OK.

APPROVED
Comment 10 Miro Hrončok 2012-11-20 05:56:17 EST
It seems to be inc::Module::Install. If I use Module::Install it yells.
Comment 11 Michal Ingeli 2012-11-20 06:11:51 EST
(In reply to comment #10)
> It seems to be inc::Module::Install. If I use Module::Install it yells.

Use it as BR, not in Makefile.PL. That one should use inc::Module::Install, so it can override system module from 'inc/' directory.
Comment 12 Miro Hrončok 2012-11-20 14:53:20 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Growl-GNTP.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Growl-GNTP-0.15-
4.fc17.src.rpm

- perl(inc::Module::Install) to perl(Module::Install)
Comment 13 Miro Hrončok 2012-11-20 14:59:58 EST
New Package SCM Request
=======================
Package Name: perl-Growl-GNTP
Short Description: Perl implementation of GNTP Protocol (Client Part)
Owners: churchyard ksyz
Branches: f17 f18
InitialCC:
Comment 14 Jon Ciesla 2012-11-20 15:42:10 EST
Git done (by process-git-requests).
Comment 15 Mario Blättermann 2012-11-20 15:48:56 EST
(In reply to comment #9)
> It's perl(Module::Install), not perl(inc::Module::Install) (provided by the
> same package).
> 
> Otherwise OK.
> 
> APPROVED

@Michal, perhaps you should read the review guidelines once again:
http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
A review shouldn't be picking this and that from the spec file and having a look around... We need a Mock or Koji build, the rpmlint output for all packages (srpm, rpm, debug packages if any), comparing the original and packaged sources and so on. Should you actually know as a sponsored packager.
Comment 16 Michal Ingeli 2012-11-21 04:14:48 EST
(In reply to comment #15)
> @Michal, perhaps you should read the review guidelines once again:
> http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
> A review shouldn't be picking this and that from the spec file and having a
> look around... We need a Mock or Koji build, the rpmlint output for all
> packages (srpm, rpm, debug packages if any), comparing the original and
> packaged sources and so on. Should you actually know as a sponsored packager.

@mario, it's hard to reply to your rant, when it looks like, you didn't even read the review. That guideline, you cited, can you, probably, provide a source? Or a reason, why you think a reviewer should not look around in the package, and pick parts from spec file? "If it builds, ship it" is not a good policy.
Comment 17 Mario Blättermann 2012-11-21 14:27:19 EST
(In reply to comment #16)
> "If it builds, ship it" is not a good policy.

That's not what I said. Of course rpmlint doesn't find all problems, but this is no reason to omit it. Have a look at some of my reviews to see what I mean [1]. The rpmlint output and a complete checklist is always present, and if there are some other issues, I report them, as far as I've found them. By the way, I'm even not convinced about fedora-review. Well, it seems to make the reviewers' job easier, but it tempts to _not_ looking around the files and assume to that all is already done by this tool. That's why I don't use it. I've had such reviews for my own packages, with lots of impressive output and even a lot of useless stuff. But the actual problems haven't been found, so that I had to do more work after pushing it to testing. Just for clarifying that I don't prefer semi-automatic workflow.

However, the package has been approved, the Git repo created, and it is on the way to the users. I know, most issues in packages come up after the review. But if we would consider this as a thing which happens anyway, we could stop reviewing packages in general.

[1] https://bugzilla.redhat.com/buglist.cgi?resolution=CURRENTRELEASE&resolution=RAWHIDE&resolution=ERRATA&resolution=NEXTRELEASE&resolution=---&classification=Fedora&emailtype1=substring&query_format=advanced&emailassigned_to1=1&bug_status=ON_QA&bug_status=CLOSED&email1=mario.blaettermann%40gmail.com&component=Package%20Review&product=Fedora&list_id=853171
Comment 18 Miro Hrončok 2012-11-21 15:57:15 EST
(In reply to comment #17)
> However, the package has been approved, the Git repo created, and it is on
> the way to the users. I know, most issues in packages come up after the
> review. But if we would consider this as a thing which happens anyway, we
> could stop reviewing packages in general.
I've stopped working on this, the repo was created, however, I wait for this discusion to end somehow, before I continue.
Comment 19 Mario Blättermann 2012-11-22 13:50:38 EST
(In reply to comment #18)
> I've stopped working on this, the repo was created, however, I wait for this
> discusion to end somehow, before I continue.

Go ahead as usual. Don't misunderstand me, I don't want to roll back anything. The repo has been created, and you should build your packages now. The only thing which I would show is that I don't recognize this as the right way to review packages. We have the review guidelines, and we should follow them. This is not about the quality of your package, it was just about the quality of the review. Of course, we can change everything once the packages have been built and some bugs come up. But we could perhaps avoid this by doing reviews according to the guidelines.
Comment 20 Michal Ingeli 2012-11-23 04:01:52 EST
(In reply to comment #18)
> I've stopped working on this, the repo was created, however, I wait for this
> discusion to end somehow, before I continue.

Until Mario (or anyone else) raises any objection or blocker, you are free to continue and submit builds.

(In reply to comment #19)
> The only thing which I would show is that I don't recognize this as the
> right way to review packages. We have the review guidelines, and we should
> follow them. This is not about the quality of your package, it was just
> about the quality of the review. Of course, we can change everything once
> the packages have been built and some bugs come up. But we could perhaps
> avoid this by doing reviews according to the guidelines.

There is no guideline, that requires use of fedora-review(1). As sponsor, you should know that. If you found that any of (and even 'SHOULD') guidelines were violated, please, state it here, so we can fix it. While doing review-review, if there are any parts of review, that are not clear to you, I will happily explain them.

Community projects are about cooperation and communication, not only guidelines, even if there is no guideline for that. If this should go more OT, please contact me (cc my sponsor) via mail.

Thanks.
Comment 21 Miro Hrončok 2012-11-24 08:36:55 EST
Hi,

I have some troubles with fedora-scm.

I've run fedpkg import xxx.src.rpm
I've commited the changes and pushed.

Now if I want to run fedpkg build or fedpkg switch-branch f18, I get this error:


Could not execute switch_branch/update: /home/churchyard/rpmbuild/fedora-scm/perl-Growl-GNTP has uncommitted changes.  Use git status too see details


$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	modified:   .gitignore
#	new file:   perl-Growl-GNTP-0.15-3.inc-and-xt-dereference.patch
#	new file:   perl-Growl-GNTP.spec
#	modified:   sources
#

$ git commit -am "Some message"
$ git status
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
#
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	modified:   .gitignore
#	new file:   perl-Growl-GNTP-0.15-3.inc-and-xt-dereference.patch
#	new file:   perl-Growl-GNTP.spec
#	modified:   sources
#


How is this possible?
Comment 22 Miro Hrončok 2012-11-24 17:18:11 EST
Never mind, I deleted the folder and cloned it again.
Comment 23 Fedora Update System 2012-11-24 18:16:53 EST
perl-Growl-GNTP-0.15-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-Growl-GNTP-0.15-4.fc17
Comment 24 Fedora Update System 2012-11-24 18:17:44 EST
perl-Growl-GNTP-0.15-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/perl-Growl-GNTP-0.15-4.fc18
Comment 25 Fedora Update System 2012-11-25 14:30:58 EST
perl-Growl-GNTP-0.15-4.fc18 has been pushed to the Fedora 18 testing repository.
Comment 26 Fedora Update System 2012-11-29 01:36:46 EST
perl-Growl-GNTP-0.15-4.fc18 has been pushed to the Fedora 18 stable repository.
Comment 27 Fedora Update System 2012-12-06 22:30:22 EST
perl-Growl-GNTP-0.15-4.fc17 has been pushed to the Fedora 17 stable repository.
Comment 28 Petr Šabata 2013-01-02 04:57:27 EST
Package Change Request
======================
Package Name: perl-Growl-GNTP
Branches: f17 f18
Owners:
InitialCC: perl-sig

Please add perl-sig user with watch* permissions only to all Fedora branches.
Comment 29 Jon Ciesla 2013-01-02 09:35:49 EST
Done.

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