Spec URL: https://in.waw.pl/~zbyszek/fedora/rubygem-ronn-ng.spec SRPM URL: https://in.waw.pl/~zbyszek/fedora/rubygem-ronn-ng-0.9.1-1.fc34.src.rpm Description: Ronn builds manuals. It converts simple, human readable text files to roff for terminal display, and also to HTML for the web. The source format includes all of Markdown but has a more rigid structure and syntax extensions for features commonly found in man pages (definition lists, link notation, etc.). The ronn-format(7) manual page defines the format in detail. Fedora Account System Username: zbyszek This is a replacement for rubygem-ronn, see https://bugzilla.redhat.com/show_bug.cgi?id=1817168.
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