Bug 889011 (asciidoctor) - Review Request: rubygem-asciidoctor - AsciiDoc implementation in Ruby
Summary: Review Request: rubygem-asciidoctor - AsciiDoc implementation in Ruby
Keywords:
Status: CLOSED ERRATA
Alias: asciidoctor
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-12-20 02:34 UTC by Dan Allen
Modified: 2015-05-29 21:46 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-03-22 00:42:55 UTC
Type: ---
vondruch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Dan Allen 2012-12-20 02:34:48 UTC
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 11:10:07 UTC
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 11:48:54 UTC
* 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 2013-01-01 01:45:57 UTC
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 06:14:03 UTC
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 18:32:22 UTC
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 17:11:03 UTC
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 22:05:19 UTC
Thanks Vit! I'll incorporate those changes and post back when they are ready.

Comment 8 Dan Allen 2013-03-01 19:05:00 UTC
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 11:13:19 UTC
(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 11:15:39 UTC
BTW do you still need to be sponsored? What is your FAS account then?

Comment 11 Dan Allen 2013-03-05 11:00:28 UTC
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 11:31:35 UTC
(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 17:44:09 UTC
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 17:44:28 UTC
Can you change the fedora-cvs flag to ?

Comment 15 Dan Allen 2013-03-05 17:48:06 UTC
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 06:07:26 UTC
(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 06:13:26 UTC
(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 13:22:42 UTC
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 13:57:24 UTC
(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 14:25:39 UTC
Requested package name asciidoctor doesn't match bug summary
rubygem-asciidoctor, please correct.

Comment 21 Dan Allen 2013-03-06 17:27:14 UTC
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 07:54:14 UTC
(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 07:57:39 UTC
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 19:11:59 UTC
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 12:15:37 UTC
Git done (by process-git-requests).

Comment 26 Fedora Update System 2013-03-11 20:58:13 UTC
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 21:01:52 UTC
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 08:25:15 UTC
rubygem-asciidoctor-0.1.1-1.fc18 has been pushed to the Fedora 18 testing repository.

Comment 29 Fedora Update System 2013-03-22 00:42:57 UTC
rubygem-asciidoctor-0.1.1-1.fc18 has been pushed to the Fedora 18 stable repository.

Comment 30 Fedora Update System 2013-03-22 00:43:50 UTC
rubygem-asciidoctor-0.1.1-1.fc17 has been pushed to the Fedora 17 stable repository.

Comment 31 Dan Allen 2015-05-12 23:23:45 UTC
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 10:19:34 UTC
Git done (by process-git-requests).

Comment 33 Fedora Update System 2015-05-13 22:57:09 UTC
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 21:46:29 UTC
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.