Bug 889011 - (asciidoctor) Review Request: rubygem-asciidoctor - AsciiDoc implementation in Ruby
Review Request: rubygem-asciidoctor - AsciiDoc implementation in Ruby
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:
  Show dependency treegraph
 
Reported: 2012-12-19 21:34 EST by Dan Allen
Modified: 2015-05-29 17:46 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-03-21 20:42:55 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 Dan Allen 2012-12-19 21:34:48 EST
Repository: https://github.com/mojavelinux/rubygem-asciidoctor
Spec URL: https://raw.github.com/mojavelinux/rubygem-asciidoctor/master/rubygem-asciidoctor.spec
SRPM URL: https://github.com/mojavelinux/rubygem-asciidoctor/raw/master/srpms/rubygem-asciidoctor-0.0.8-1.fc17.src.rpm
Description: Asciidoctor is a pure-Ruby processor for parsing AsciiDoc documents or strings and rendering them as HTML and other formats using Tilt-based templates.
Fedora Account System Username: mojavelinux
Comment 1 Vít Ondruch 2012-12-20 06:10:07 EST
Hi Dan,

I'll take this for a review and I can sponsor you as well (though you are not mentioning it here).
Comment 2 Vít Ondruch 2012-12-20 06:48:54 EST
* gem is not available upstream
  - Review guidelines [1] says: "MUST: The sources used to build the package
    must match the upstream source, as provided in the spec URL." Your package
    builds on top of version 0.0.8, which is newer then the version available on
    RubyGems. It would be nice if you can either release 0.0.8 to rubygems.org
    (I would suggest this as a the best option) or if you can at least describe
    how you obtained the gem you are providing (but in this case, it would be
    probably better to provide tarball and use some pre-release versioning [2]).

* Exclude %{gem_cache}
  - We typically exclude %{gem_cache} from the package:

    %exclude %{gem_cache}

    The %{gem_cache} has no meaning on Fedora, since you can achieve similar
    goals to purpose of %{gem_cache} by RPM means.

* Comment patches
  - If you include patches in your SRPM, it is usually good idea to introduce
    them with comment what they are good for, typically including for example
    upstream commit or issue reference.

* Follow %build and %install sections of Ruby packaging guidelines
  - Could you please modify the %build and %install section according to Ruby
    Packaging Guideliens [3]? It would help in the future during possible
    automatic rebuilds of packages as well as other maintainers to understand to
    your package. There are also small differences for example
      - Difference between upstream .gemspec file and the .gemspec file
        generated by RubyGems during installation of gem
      - Difference in binary. The binary generated by RubyGems is not the same
        file as the binary in your gem's bin folder.

I will stop the review at this point, since the change of %build and %install sections is substantial, so it could made my other comments irrelevant. Please update the package before we continue. Thank you.


[1] http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
[2] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages
[3] https://fedoraproject.org/wiki/Packaging:Ruby#Building_gems
Comment 3 Dan Allen 2012-12-31 20:45:57 EST
Vít,

Thanks for taking the time to review my RPM spec! I'll incorporate your feedback and post again when it's ready for another round.

Btw, regarding the source, I anticipated when I submitted that 0.0.8 would be tagged and released later that day, but alas it didn't happen. When I post again, I'll make sure the spec aligns with an official release :)
Comment 4 Dan Allen 2013-01-20 01:14:03 EST
Vít,

This package is now ready for another round of review.

I noticed that some of the information on the Packaging:Ruby page is incorrect. For instance, it says that %{gem_dir} resolves to /usr/share/rubygems when in fact it resolves to /usr/share/gems. Also, I had to use %{gem_instdir} instead of %{gem_dir} in some of the commands in %install because it just doesn't make sense otherwise.

I look forward to your feedback.
Comment 5 Dan Allen 2013-01-20 13:32:22 EST
Spec URL: https://raw.github.com/mojavelinux/rubygem-asciidoctor/master/rubygem-asciidoctor.spec
SRPM URL: https://github.com/mojavelinux/rubygem-asciidoctor/raw/master/srpms/rubygem-asciidoctor-0.0.9-1.fc17.src.rpm

Updated to 0.0.9 release of Asciidoctor, followed Ruby package spec guidelines, added test framework requires.
Comment 6 Vít Ondruch 2013-02-04 12:11:03 EST
Hi Dan, 

* Could you please consider to simplify the install section a bit? Two copy commands should be enough (if there is no special reason):

mkdir -p %{buildroot}%{gem_dir}
cp -pa .%{gem_dir}/* \
        %{buildroot}%{gem_dir}/


mkdir -p %{buildroot}%{_bindir}
cp -pa .%{_bindir}/* \
        %{buildroot}%{_bindir}/

and then you can exclude the gem cache in files section using:

%exclude %{gem_cache}

* Could you please remove the Requires: ruby
  - Unless you know that the gem is compatible just with MRI, it would be nice
    if you could drop the ruby dependency, since we are going to add support for
    JRuby in F19.

* I observe several test failures running the build on F19, could you please check them?
Comment 7 Dan Allen 2013-02-04 17:05:19 EST
Thanks Vit! I'll incorporate those changes and post back when they are ready.
Comment 8 Dan Allen 2013-03-01 14:05:00 EST
I've updated the package as you requested, fixed the tests, updated the source to Asciidoctor 0.1.1 (which is really the one we want to see out there in the wild) and built it on F18.

We introduced a man page in 0.1.0, so I had to add an additional copy command to get it to work. If you have a cleaner way to do it, just let me know. Otherwise, it gets the job done.

Here are the new files:

Spec URL: https://raw.github.com/asciidoctor/rubygem-asciidoctor-rpm/master/rubygem-asciidoctor.spec
SRPM URL: https://github.com/asciidoctor/rubygem-asciidoctor-rpm/raw/master/srpms/rubygem-asciidoctor-0.1.1-1.fc18.src.rpm

I'm really hoping to turn this one around quickly and get it into the hands of users.
Comment 9 Vít Ondruch 2013-03-04 06:13:19 EST
(In reply to comment #8)
> We introduced a man page in 0.1.0, so I had to add an additional copy
> command to get it to work. If you have a cleaner way to do it, just let me
> know. Otherwise, it gets the job done.

I don't know any other better way. On the other hand, it'd be nice RFE for RubyGems upstream 


* Add Requires: ruby(abi)
  - I asked you to remove "Requires: ruby". Unfortunately, you removed ruby(abi)
    as well. However, this must stay.

This is the only issue I see with this package currently, so I conditionally APPROVE the package.

However, please note that there are approved new Ruby packaging guidelines for F19. According to them, you should replace the ruby(abi) with ruby(release) and you should replace a "gem install" command by %gem_install macro. It would be cool if you can update the package accordingly and build already in f19-ruby targed, where the rebuild for Ruby 2.0.0 is ongoing.
Comment 10 Vít Ondruch 2013-03-04 06:15:39 EST
BTW do you still need to be sponsored? What is your FAS account then?
Comment 11 Dan Allen 2013-03-05 06:00:28 EST
Vit,

I've updated the package to restore the require Ruby line as "Requires: ruby(release)".

Spec URL: https://raw.github.com/asciidoctor/rubygem-asciidoctor-rpm/master/rubygem-asciidoctor.spec
SRPM URL: https://github.com/asciidoctor/rubygem-asciidoctor-rpm/raw/master/srpms/rubygem-asciidoctor-0.1.1-1.fc18.src.rpm

I don't have a F19 box setup, nor am I familiar with how to get one. Is that just rawhide? Is there a recommended installation procedure for that? Alos, how do I maintain that spec file separate from F18? Do I use a branch in the git repository?

Yes, I still need to be sponsered. My FAS account is mojavelinux.

Let me know what step I need to do next.

Thanks!
Comment 12 Vít Ondruch 2013-03-05 06:31:35 EST
(In reply to comment #11)
> I've updated the package to restore the require Ruby line as "Requires:
> ruby(release)".

You should use also the %gem_install macro then.

> Spec URL:
> https://raw.github.com/asciidoctor/rubygem-asciidoctor-rpm/master/rubygem-
> asciidoctor.spec
> SRPM URL:
> https://github.com/asciidoctor/rubygem-asciidoctor-rpm/raw/master/srpms/
> rubygem-asciidoctor-0.1.1-1.fc18.src.rpm
> 
> I don't have a F19 box setup, nor am I familiar with how to get one.

You don't need F19 box, mock is enough: 

http://fedoraproject.org/wiki/Projects/Mock
http://fedoraproject.org/wiki/Using_Mock_to_test_package_builds

It is good idea to get familiar with mock, since mock is used by Koji for builds, it allows you to maintain packages for several Fedora, EPEL or RHEL releases and it allows you to build your package in clean environment.

> Is that just rawhide?

The ruby(release) is just in Rawhide, for older releases, you should use ruby(abi) = 1.9.1 or something similar. The %gem_install macro was backported into older Fedoras, so you should be able to use it everywhere.

> how do I maintain that spec file separate from F18? Do I use a branch in the
> git repository?

Branch is one way, other possibility is conditionalize your spec. It depends on preferences. If there is not much differences (for you just the ruby(abi) vs ruby(release)) I would suggest to use conditions, such as:

%if 0%{?fedora} <= 18
Requires: ruby(abi) = 1.9.1
BuildRequires: ruby(abi) = 1.9.1
%else
Requires: ruby(release)
BuildRequires: ruby(release)
%endif

If the differences should be bigger, then the branch is good option. There is no "better way", it is your choice. However, the conditions tends to make the spec file harder to read.

Also, please note that for the moment, you need to build the package as 'fedpkg build --target=f19-ruby', otherwise your dependencies will not be satisfied. This is just temporary thing, please see Ruby-SIG mailing list for more information.

> Yes, I still need to be sponsered. My FAS account is mojavelinux.

You are sponsored now.

> Let me know what step I need to do next.

You should continue with importing package into SCM [1]. Please let know anytime if you need more guidance.


[1] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Comment 13 Dan Allen 2013-03-05 12:44:09 EST
New Package SCM Request
=======================
Package Name: asciidoctor
Short Description: AsciiDoc implementation in Ruby
Owners: mojavelinux
Branches: f17 f18 f19 el6
Comment 14 Dan Allen 2013-03-05 12:44:28 EST
Can you change the fedora-cvs flag to ?
Comment 15 Dan Allen 2013-03-05 12:48:06 EST
Thanks for sponsoring this package and for the info.

I updated the spec with conditional logic to align the commands to the package conventions for each fedora version.

Spec URL: https://raw.github.com/asciidoctor/rubygem-asciidoctor-rpm/master/rubygem-asciidoctor.spec
SRPM URL: https://github.com/asciidoctor/rubygem-asciidoctor-rpm/raw/master/srpms/rubygem-asciidoctor-0.1.1-1.fc18.src.rpm

The %gem_install macro did not work for me on F18 (I'm using rpmbuild -ba).

I'm still trying to sort out how to use mock.
Comment 16 Vít Ondruch 2013-03-06 01:07:26 EST
(In reply to comment #14)
> Can you change the fedora-cvs flag to ?

You should be able to do it yourself.
Comment 17 Vít Ondruch 2013-03-06 01:13:26 EST
(In reply to comment #15)
> I updated the spec with conditional logic to align the commands to the
> package conventions for each fedora version.

Looks good.

> The %gem_install macro did not work for me on F18 (I'm using rpmbuild -ba).

It was probably bad timing :/. The update [1] went stable yesterday night, so it should work now. I.e. you should be using rubygems-1.8.25-4.fc18 to have success.

[1] https://admin.fedoraproject.org/updates/rubygems
Comment 18 Dan Allen 2013-03-06 08:22:42 EST
I see the %gem_install macro now. I've updated the spec file in git (but haven't pushed the updated srpm yet).

I can't change the flag in this issue. The select menu is disabled for me.
Comment 19 Vít Ondruch 2013-03-06 08:57:24 EST
(In reply to comment #18)
> I can't change the flag in this issue. The select menu is disabled for me.

That is weird. I can set it for you for now, but try to contact somebody (probably Jon Ciesla) on freenode, who might be able to help you.
Comment 20 Gwyn Ciesla 2013-03-06 09:25:39 EST
Requested package name asciidoctor doesn't match bug summary
rubygem-asciidoctor, please correct.
Comment 21 Dan Allen 2013-03-06 12:27:14 EST
New Package SCM Request
=======================
Package Name: rubygem-asciidoctor
Short Description: AsciiDoc implementation in Ruby
Owners: mojavelinux
Branches: f17 f18 f19 el6
Comment 22 Vít Ondruch 2013-03-07 02:54:14 EST
(In reply to comment #19)
> (In reply to comment #18)
> > I can't change the flag in this issue. The select menu is disabled for me.
> 
> That is weird. I can set it for you for now, but try to contact somebody
> (probably Jon Ciesla) on freenode, who might be able to help you.

Actually, Dan, I realized that you are using different email for BZ then in FAS, so it is not possible to match you as one user. Not sure if there is better way then change of email in FAS.


BTW, you still have different "short description" in BZ subject then in SCM request. Not sure if that is critical, but it is convenient to have them in sync.
Comment 23 Vít Ondruch 2013-03-07 02:57:39 EST
Now I found similar issue discussed on fedora-devel [1]. So you should speak to Kevin I guess.


[1] http://lists.fedoraproject.org/pipermail/devel/2012-November/174520.html
Comment 24 Dan Allen 2013-03-08 14:11:59 EST
Thanks for exploring the permission issue Vit. I will make the necessary adjustments.

I also aligned the descriptions of the package.
Comment 25 Gwyn Ciesla 2013-03-11 08:15:37 EDT
Git done (by process-git-requests).
Comment 26 Fedora Update System 2013-03-11 16:58:13 EDT
rubygem-asciidoctor-0.1.1-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-asciidoctor-0.1.1-1.fc18
Comment 27 Fedora Update System 2013-03-11 17:01:52 EDT
rubygem-asciidoctor-0.1.1-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-asciidoctor-0.1.1-1.fc17
Comment 28 Fedora Update System 2013-03-12 04:25:15 EDT
rubygem-asciidoctor-0.1.1-1.fc18 has been pushed to the Fedora 18 testing repository.
Comment 29 Fedora Update System 2013-03-21 20:42:57 EDT
rubygem-asciidoctor-0.1.1-1.fc18 has been pushed to the Fedora 18 stable repository.
Comment 30 Fedora Update System 2013-03-21 20:43:50 EDT
rubygem-asciidoctor-0.1.1-1.fc17 has been pushed to the Fedora 17 stable repository.
Comment 31 Dan Allen 2015-05-12 19:23:45 EDT
Package Change Request
======================
Package Name: rubygem-asciidoctor
Short Description: AsciiDoc implementation in Ruby
New Branches: epel7
Owners: mojavelinux ktdreyer
Comment 32 Gwyn Ciesla 2015-05-13 06:19:34 EDT
Git done (by process-git-requests).
Comment 33 Fedora Update System 2015-05-13 18:57:09 EDT
rubygem-asciidoctor-1.5.2-1.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/rubygem-asciidoctor-1.5.2-1.el7
Comment 34 Fedora Update System 2015-05-29 17:46:29 EDT
rubygem-asciidoctor-1.5.2-1.el7 has been pushed to the Fedora EPEL 7 stable repository.

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