Bug 889011 (asciidoctor)

Hi Dan,

I'll take this for a review and I can sponsor you as well (though you are not mentioning it here).

* 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

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 :)

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.

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.

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?

Thanks Vit! I'll incorporate those changes and post back when they are ready.

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.

(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.

BTW do you still need to be sponsored? What is your FAS account then?

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!

(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

New Package SCM Request
=======================
Package Name: asciidoctor
Short Description: AsciiDoc implementation in Ruby
Owners: mojavelinux
Branches: f17 f18 f19 el6

Can you change the fedora-cvs flag to ?

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.

(In reply to comment #14)
> Can you change the fedora-cvs flag to ?

You should be able to do it yourself.

(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

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.

(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.

Requested package name asciidoctor doesn't match bug summary
rubygem-asciidoctor, please correct.

New Package SCM Request
=======================
Package Name: rubygem-asciidoctor
Short Description: AsciiDoc implementation in Ruby
Owners: mojavelinux
Branches: f17 f18 f19 el6

(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.

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

Thanks for exploring the permission issue Vit. I will make the necessary adjustments.

I also aligned the descriptions of the package.

Git done (by process-git-requests).

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

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

rubygem-asciidoctor-0.1.1-1.fc18 has been pushed to the Fedora 18 testing repository.

rubygem-asciidoctor-0.1.1-1.fc18 has been pushed to the Fedora 18 stable repository.

rubygem-asciidoctor-0.1.1-1.fc17 has been pushed to the Fedora 17 stable repository.

Package Change Request
======================
Package Name: rubygem-asciidoctor
Short Description: AsciiDoc implementation in Ruby
New Branches: epel7
Owners: mojavelinux ktdreyer

Git done (by process-git-requests).

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

rubygem-asciidoctor-1.5.2-1.el7 has been pushed to the Fedora EPEL 7 stable repository.