Bug 850469 - Review Request: rubygem-Ascii85 - Methods to encode/decode Adobe's binary-to-text encoding of the same name
Review Request: rubygem-Ascii85 - Methods to encode/decode Adobe's binary-to-...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Vít Ondruch
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 850679
  Show dependency treegraph
 
Reported: 2012-08-21 11:52 EDT by Miroslav Suchý
Modified: 2012-09-17 18:05 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-09-01 20:24:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
vondruch: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Miroslav Suchý 2012-08-21 11:52:39 EDT
Spec URL: http://miroslav.suchy.cz/fedora/rubygem-Ascii85/rubygem-Ascii85.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-Ascii85/rubygem-Ascii85-1.0.1-3.fc17.src.rpm
Description: Ascii85 is a simple gem that provides methods for encoding/decoding Adobe's
binary-to-text encoding of the same name.

Fedora Account System Username:msuchy

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4410283

$ rpmlint /tmp/tito-build/rubygem-Ascii85-1.0.1-3.fc17.src.rpm /tmp/tito-build/noarch/rubygem-Ascii85-1.0.1-3.fc17.noarch.rpm
rubygem-Ascii85.src: W: invalid-url Source1: ascii85.1.pod.tgz
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
Comment 1 Vít Ondruch 2012-08-23 06:21:53 EDT
I'll take it for a review.
Comment 2 Vít Ondruch 2012-08-23 08:46:18 EDT
* Man pages
  - Wouldn't be better to keep the .pod file in original form, i.e. no tarball?
    It is so small that the benefits (you can read it in your editor, it could
    be kept in git and you could diff it easily) would outweigh the negatives
    (yes, the tarball is few bytes smaller probably, but git compresses its
    repository anyway). Moreover, you could reference the %{SOURCE1} directly
    in pod2man command, instead of unpacking and then referring to it.

* Do not delete %{gem_instdir}/bin
  - This is definitely bad idea:

    $ ascii85 
    /usr/bin/ascii85:23:in `load': cannot load such file -- /usr/share/gems/gems
        /Ascii85-1.0.1/bin/ascii85 (LoadError)
	from /usr/bin/ascii85:23:in `<main>'

  - The %{_bindir} executable is just stub generated by rubygems and the
    %{gem_instdir}/bin is the real executable.

* BuildRequires: perl
  - Really? What is it good for?

* Test suite
  - Cannot be executed
    - See your Koji link referenced above ;)
    - Adding "BuildRequires: %{_bindir}/rspec" could solve the issue
  - Not sure why are you using the -P option. Can't see any difference running
    the test suite with or without it.
  - I prefer to run the test suite in .%{gem_instdir} instead of
    %{buildroot}%{gem_instdir}. While not exclusively better, if there is some
    test suite debris, it will not get into the RPM at least.

* Keep the documentation in its original location
  - I know [1], but unless standardized differently, I'd like to point it out.
    However not a blocker.

* ruby -KU
  - There are two alternatives which might be better:

    RUBYOPT="-KU" gem build %{gem_name}.gemspec

    or

    LANG=en_US.utf8 gem build %{gem_name}.gemspec


[1] http://lists.fedoraproject.org/pipermail/packaging/2012-August/008598.html
Comment 3 Miroslav Suchý 2012-08-23 10:53:34 EDT
> man pages
I use tito for packaging. And it include in src.rpm only files with extenstion from this list:
'.tar.gz', '.tgz', '.tar.bz2', '.tar', '.zip', '.jar', '.gem', ".spec", ".patch"
So I would either had to make patch from it or do it manualy.
And since I'm lazy as hell I prefer easy maintenance. Therefore I created tar from those files. They have to be download from obsucure location so there is no way for rpmlint to check md5sum, so I think it make no harm.

> BuildRequires: perl - What is it good for?
 For pod2man.

> Test suite
fixed


> Keep the documentation in its original location
I will rebel here, if it is not blocker. :)


> -KU
"LANG=en_US.utf8 gem" does not work for me
"RUBYOPT="-KU"" works find, I used this.

> Do not delete %{gem_instdir}/bin
fixed

Spec URL: http://miroslav.suchy.cz/fedora/rubygem-Ascii85/rubygem-Ascii85.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-Ascii85/rubygem-Ascii85-1.0.1-4.fc17.src.rpm
Comment 4 Vít Ondruch 2012-08-24 06:14:47 EDT
(In reply to comment #3)
> > man pages
> I use tito for packaging. And it include in src.rpm only files with
> extenstion from this list:
> '.tar.gz', '.tgz', '.tar.bz2', '.tar', '.zip', '.jar', '.gem', ".spec",
> ".patch"
> So I would either had to make patch from it or do it manualy.
> And since I'm lazy as hell I prefer easy maintenance. Therefore I created
> tar from those files. They have to be download from obsucure location so
> there is no way for rpmlint to check md5sum, so I think it make no harm.

May be time to update Tito to support .pod files? Tools you are using cannot be excuse to provide suboptimal .spec files. I already explained what harm it does. It would be the same as compressing .patch files etc. Moreover, I believe (although not 100% sure) that the tgz file will get always uploaded into the look-aside cache, which will be definitely less effective in handling of updates to this file.

Actually, you should use the following line:

Source1: http://rubyforge.org/tracker/download.php/7826/30313/29377/5301/ascii85.1.pod

which would fix the one remaining rpmlint warning.


> > BuildRequires: perl - What is it good for?
>  For pod2man.

Negative. I tried to remove it and it still works. Please remove the BR: perl, unless you have evidence that it is needed on Fedora(RHEL) != 19

* Man generation flags
  - Could you please keep the flags suggested in the ticket where you took the
    manual pages? If you omit them, there is stated "Perl" all over the place,
    which is not true. 

* Separate license file
  - Could you pleas query upstream about separate license file?

From the review, I have the feeling, that you are not using mock for testing your packages. May be you should consider to look into it. It is worth of the time spent to learn.

Since these are more or less just cosmetic issues and the package looks good otherwise, I APPROVE the package. However, please consider to include fixes to my comments prior importing.
Comment 5 Miroslav Suchý 2012-08-24 07:10:16 EDT
> Man generation flags
fixed

> Separate license file
done:
http://rubyforge.org/tracker/index.php?func=detail&aid=29629&group_id=7826&atid=30313


> > BuildRequires: perl
This is because perl is in minimal Buildroot. Per discussion on irc flipped to
BuildRequires: %{_bindir}/pod2man

Thanks for the review.
Comment 6 Miroslav Suchý 2012-08-24 07:16:01 EDT
New Package SCM Request
=======================
Package Name: rubygem-Ascii85
Short Description: Methods to encode/decode Adobe's binary-to-text encoding of the same name
Owners: msuchy
Branches: F-18, F-17, F-16, EL-6
InitialCC:
Comment 7 Gwyn Ciesla 2012-08-24 07:52:03 EDT
Git done (by process-git-requests).
Comment 8 Fedora Update System 2012-08-24 08:32:48 EDT
rubygem-Ascii85-1.0.1-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-Ascii85-1.0.1-5.fc18
Comment 9 Fedora Update System 2012-08-24 08:36:14 EDT
rubygem-Ascii85-1.0.1-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-Ascii85-1.0.1-5.fc17
Comment 10 Fedora Update System 2012-08-24 08:42:24 EDT
rubygem-Ascii85-1.0.1-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-Ascii85-1.0.1-5.fc16
Comment 11 Fedora Update System 2012-08-24 09:09:42 EDT
rubygem-Ascii85-1.0.1-7.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-Ascii85-1.0.1-7.el6
Comment 12 Fedora Update System 2012-08-24 17:32:48 EDT
rubygem-Ascii85-1.0.1-5.fc18 has been pushed to the Fedora 18 testing repository.
Comment 13 Fedora Update System 2012-09-01 20:24:40 EDT
rubygem-Ascii85-1.0.1-5.fc17 has been pushed to the Fedora 17 stable repository.
Comment 14 Fedora Update System 2012-09-01 20:28:34 EDT
rubygem-Ascii85-1.0.1-5.fc16 has been pushed to the Fedora 16 stable repository.
Comment 15 Fedora Update System 2012-09-09 14:35:13 EDT
rubygem-Ascii85-1.0.1-7.el6 has been pushed to the Fedora EPEL 6 stable repository.
Comment 16 Fedora Update System 2012-09-17 18:05:03 EDT
rubygem-Ascii85-1.0.1-5.fc18 has been pushed to the Fedora 18 stable repository.

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