Bug 1871157

Taking this for a review.

Zbysek, could you please use gem2rpm to bring the .spec to the more up2date form? It seems that you spec contains a lot of archaisms. Also, it does not execute the test suite.

Also, it does not replace the rubygem-ronn, which it should IMO. However, I am not sure how many times is required 'rubygem-ronn' vs '%{_bindir}/ronn'.

Updated in place.

* Test suite
  - There is not missing rubygem(contest) just proper load path [1]. To load the `contest`, there have to be added `-Itest` specified.
    And for loading rest of the Ronn, the load path has to be `-Ilib:test`.
  - You also need a few BRs:

~~~
BuildRequires:  rubygem(kramdown)
BuildRequires:  rubygem(mustache)
BuildRequires:  rubygem(nokogiri)
BuildRequires:  rubygem(test-unit)
~~~

* Soft dependency on groff-base?
  - The `Requires: groff-base` could be possibly changed to `Recommends`. This could help if Ronn is used as Ruby library. OTOH,
    if it is included into buildroot to generate documentation, the soft dependency would not be installed and that could be annoying.
  - Nevertheless, wouldn't it be better to use `Requires: %{_bindir}/groff` instead?

* Changing gem version dependencies.
  - I prefer to remove/add specific versions with `%gemspec_{remove,add}_dep`. This gives me hint if the dependencies are changing.
    This way, I would be able to remove the lines in the future (https://github.com/apjanke/ronn-ng/pull/46), otherwise changes like
    this will go unnoticed.
  - Also, you are changing wrong .gemspec file. The right file will be modified if you drop the `-s %{gem_name}.gemspec`.
  - IOW, this and nothing else should be what you want to do:

~~~
%gemspec_remove_dep -g mustache "~> 0.7"
~~~

* Use macros where appropriate
  - Please use macros on places where `/usr/share/` is used ATM.

* Keep license file in its original place?
  - I typically keep the license file in its original place, but this might be even better, so I'm just saying because I stumbled upon it.

* Small typo in changelog
  - s/get2rpm/gem2rpm/

* Shebang vs execute bit
  - Not sure this is worth of fixing, but upstream report about the first one would not hurt:

~~~
*** WARNING: ./usr/share/gems/gems/ronn-ng-0.9.1/lib/ronn.rb is executable but has no shebang, removing executable bit
mangling shebang in /usr/share/gems/gems/ronn-ng-0.9.1/bin/ronn from /usr/bin/env ruby to #!/usr/bin/ruby
~~~



[1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/Ruby/#_minitest_testunit

Thanks!

(In reply to Vít Ondruch from comment #5)
> * Test suite
>   - There is not missing rubygem(contest) just proper load path [1]. To load
> the `contest`, there have to be added `-Itest` specified.
>     And for loading rest of the Ronn, the load path has to be `-Ilib:test`.
>   - You also need a few BRs:
> 
> ~~~
> BuildRequires:  rubygem(kramdown)
> BuildRequires:  rubygem(mustache)
> BuildRequires:  rubygem(nokogiri)
> BuildRequires:  rubygem(test-unit)
> ~~~

Yep, I did that, and the tests pass.

> * Soft dependency on groff-base?
>   - The `Requires: groff-base` could be possibly changed to `Recommends`.
> This could help if Ronn is used as Ruby library. OTOH,
>     if it is included into buildroot to generate documentation, the soft
> dependency would not be installed and that could be annoying.
>   - Nevertheless, wouldn't it be better to use `Requires: %{_bindir}/groff`
> instead?

I changed it to /usr/bin/groff. I think this is not going to be used as a library
much, and breaking the commandline use would be annoying. (I didn't use a macro
in the build requires, because if this package was rebuilt with a different
%_bindir, e.g. for a flatpak, we'd still want the original location for groff.)

> ~~~
> %gemspec_remove_dep -g mustache "~> 0.7"
> ~~~

Done.

> * Use macros where appropriate
>   - Please use macros on places where `/usr/share/` is used ATM.

Done.
 
> * Keep license file in its original place?
>   - I typically keep the license file in its original place, but this might
> be even better, so I'm just saying because I stumbled upon it.

Done.

> * Small typo in changelog
>   - s/get2rpm/gem2rpm/

Done.

> * Shebang vs execute bit
>   - Not sure this is worth of fixing, but upstream report about the first
> one would not hurt:
>
> ~~~
> *** WARNING: ./usr/share/gems/gems/ronn-ng-0.9.1/lib/ronn.rb is executable
> but has no shebang, removing executable bit
> mangling shebang in /usr/share/gems/gems/ronn-ng-0.9.1/bin/ronn from
> /usr/bin/env ruby to #!/usr/bin/ruby
> ~~~

I remove the bit now in %prep.

.spec and .src.rpm updated in place.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> Thanks!
> 
> (In reply to Vít Ondruch from comment #5)
> > * Test suite
> Yep, I did that, and the tests pass.

We typically execute test in `pushd .%{gem_instdir}`, because that is the final layout as after `gem install` call.

> > * Soft dependency on groff-base?
> >   - The `Requires: groff-base` could be possibly changed to `Recommends`.
> > This could help if Ronn is used as Ruby library. OTOH,
> >     if it is included into buildroot to generate documentation, the soft
> > dependency would not be installed and that could be annoying.
> >   - Nevertheless, wouldn't it be better to use `Requires: %{_bindir}/groff`
> > instead?
> 
> I changed it to /usr/bin/groff. I think this is not going to be used as a
> library
> much, and breaking the commandline use would be annoying. (I didn't use a
> macro
> in the build requires, because if this package was rebuilt with a different
> %_bindir, e.g. for a flatpak, we'd still want the original location for
> groff.)

While I understand the Flatpak argument, I don't think it is valid.

1) It is not reflected in guidelines, so if you feel strong about it, the guidelines should be updated.
2) It should be resolved on RPM level and there is already discussion:

https://github.com/rpm-software-management/rpm/issues/721


> .spec and .src.rpm updated in place.

Other that that, I don't have any serious concerns => APPROVED

> We typically execute test in `pushd .%{gem_instdir}`, because that is the final layout as after `gem install` call.
Added.

> https://github.com/rpm-software-management/rpm/issues/721
I commented there. I'll leave it as is for now. If FPC decides clarifies that %_bindir should be used here,
it will be trivial change.

Thank you for the review.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-ronn-ng

Ping?

Oops, thanks for the reminder. Built in rawhide now.

Thx. I have retired rubygem-ronn:

https://src.fedoraproject.org/rpms/rubygem-ronn/c/0db54f01bd7e7086b05808288e0ec6d60f1a42cc?branch=master