Hi,

looks great :), I'll comment more inline.

nit regarding the spec license, "GPL-2.0" is not valid SPDX (according to license-validate at least), however, there is "GPL-2.0-only" or "GPL-2.0-or-later".
I am maybe coming too soon with this comment, but since we are also on the SPDX topic in parallel ;).

On 11/23/22 17:47, Vít Ondruch wrote:

Building upon this, here is another attempt:

https://fedorapeople.org/cgit/vondruch/public_git/darkfish.git/commit/?h=rawhide&id=487234ef5f64f78291ce767a8a989649ee941c73

https://koji.fedoraproject.org/koji/taskinfo?taskID=94460961

Would you consider using COPR for these proof-of-concept builds?
It makes testing the builds a small bit easier.

I am having trouble making it work, there already is registered generator in the "done_installing_hooks", resulting in hitting both the raises (the second one I hit after commenting out the first one).
Secondly (if I ignore both exceptions), I am met with a trace when trying to generate `ri` (default on my `gem install --local`):
~~~
$ gem uninstall chronic; gem install ./chronic-0.10.2.gem --document=ri,rdoc --local
Successfully uninstalled chronic-0.10.2
Successfully installed chronic-0.10.2
FedoraRDoc.generation_hook - the `specs` could be modified to include additional `rdoc_options`
Parsing documentation for chronic-0.10.2
ERROR:  While executing gem ... (OptionParser::InvalidArgument)
    invalid argument: Invalid output formatter fedora::ri
    /usr/share/gems/gems/rdoc-6.4.0/lib/rdoc/options.rb:1220:in `setup_generator'
    /usr/share/gems/gems/rdoc-6.4.0/lib/rdoc/rubygems_hook.rb:121:in `document'
    /usr/share/darkfish-rdoc/rubygems_plugin.rb:30:in `document'
    /usr/share/gems/gems/rdoc-6.4.0/lib/rdoc/rubygems_hook.rb:195:in `generate'
    /usr/share/gems/gems/rdoc-6.4.0/lib/rdoc/rubygems_hook.rb:56:in `block in generation_hook'
    /usr/share/gems/gems/rdoc-6.4.0/lib/rdoc/rubygems_hook.rb:55:in `each'
    /usr/share/gems/gems/rdoc-6.4.0/lib/rdoc/rubygems_hook.rb:55:in `generation_hook'
    /usr/share/darkfish-rdoc/rubygems_plugin.rb:25:in `generation_hook'
    /usr/share/rubygems/rubygems/request_set.rb:311:in `block in install_hooks'
    /usr/share/rubygems/rubygems/request_set.rb:310:in `each'
    /usr/share/rubygems/rubygems/request_set.rb:310:in `install_hooks'
    /usr/share/rubygems/rubygems/request_set.rb:209:in `install'
    /usr/share/rubygems/rubygems/commands/install_command.rb:210:in `install_gem'
    /usr/share/rubygems/rubygems/commands/install_command.rb:226:in `block in install_gems'
    /usr/share/rubygems/rubygems/commands/install_command.rb:219:in `each'
    /usr/share/rubygems/rubygems/commands/install_command.rb:219:in `install_gems'
    /usr/share/rubygems/rubygems/commands/install_command.rb:167:in `execute'
    /usr/share/rubygems/rubygems/command.rb:323:in `invoke_with_build_args'
    /usr/share/rubygems/rubygems/command_manager.rb:185:in `process_args'
    /usr/share/rubygems/rubygems/command_manager.rb:149:in `run'
    /usr/share/rubygems/rubygems/gem_runner.rb:51:in `run'
    /usr/bin/gem:13:in `<main>'

~~~

From `invalid argument: Invalid output formatter fedora::ri` present in the log,
I'd say that following line https://fedorapeople.org/cgit/vondruch/public_git/darkfish.git/tree/rubygems_plugin.rb?h=rawhide&id=487234ef5f64f78291ce767a8a989649ee941c73#n29
is too aggressive. We can also provide the corresponding RDoc generator (even if it would be empty subclass).

This succeeds with only generating rdoc, ignoring ri format:
~~~
$ gem uninstall chronic; gem install ./chronic-0.10.2.gem --document=rdoc --local
Successfully uninstalled chronic-0.10.2
Successfully installed chronic-0.10.2
FedoraRDoc.generation_hook - the `specs` could be modified to include additional `rdoc_options`
Parsing documentation for chronic-0.10.2
Installing fedora::darkfish documentation for chronic-0.10.2
Done installing documentation for chronic after 1 seconds
1 gem installed

~~~

In regards to tests (%check):
What we want to test is: We can generate the documentation in context of Fedora (IOW, `gem install` passes with `--document=rdoc,ri`)
and then validate that anything that is an image, or JS file that is not "search_index.js" is symlinked to the template and that there are no fonts present,
as those are the places where we differ from the tested upstream behavior.

This would help us catch any major regression that I can think of, at build time, hopefully being a step ahead of accidentally breaking rubygem builds.

Or maybe in addition there should a new sanity test case for the general Ruby integration tests, that we can successfully run the mentioned gem install, maybe even some simple rubygem package build,
for the case that the RDoc darkfish template changes in some breaking way during a Ruby update.

While I have reverted back to non gem approach, the RubyGems plugin hook can still be utilized. I have reverted from the gem approach, because it allows to have the template outside of the RubyGems directory structure, while it does not prohibit to install RubyGems hook.

I have also tried different approaches, such as having full featured RDoc extension, but the RDoc templates are loaded quite late in the process [1], so they would not be available for the RubyGems generator.

This iteration also removes monkey patching in favor of inheritance.

Nit to the code, if it is using inheritance, maybe replace the method aliasing with `super' in this file: https://fedorapeople.org/cgit/vondruch/public_git/darkfish.git/tree/fedora_darkfish.rb?h=rawhide&id=487234ef5f64f78291ce767a8a989649ee941c73 ?

It seems like it can be simply replaced if I didn't miss anything. That was one of my reasons for inheritance, call to `super' seems simpler.

Last but not least, this iteration helps me a bit with clarification of the name of the project. The generators has to have `RDoc::Generator::` prefix, therefore the class is named `RDoc::Generator::Fedora::Darkfish` (and there is also `RDoc::Generator::Fedora::JsonIndex`), therefore I think the package/project should be named fedora-darkfish. I'm still not clear if `rdoc` suffix/prefix would help. While long, maybe `rdoc-generator-fedora-darkfish` prefix would correlate with the class name the best and it would hint to  RDoc and Darkfish. Should there be another generators packaged, they could possibly follow similar scheme (while in theory the should have rubygem- prefix 🫣).

I am a fan of the "rdoc-generator-fedora-darkfish".

RDoc generators could have rubygem-rdoc-generator-* if we would want to mandate prefixes, but that would come with new packaging guidelines, etc...
This iteration does not have rubygem-* prefix as it is not a gem, so I would say, this is the odd case :).

PTAL and let me know what do you think about this approach. Thx

I'd say we are on the right path with this.

Regards,
Jarek

Vít


[1] https://github.com/ruby/rdoc/blob/45259f9024b7d08ae2ba119c9a02983bf672f10c/lib/rdoc/rdoc.rb#L532-L546


Dne 01. 11. 22 v 18:53 Vít Ondruch napsal(a):


Dne 26. 10. 22 v 17:32 Vít Ondruch napsal(a):
We could also try to somehow modify the Gem.done_installing hooks and remove the RDoc hook and replace it with ours.


Building upon this idea, I am attaching an proof of concept. This loads the original RubyGems RDoc hook, removes it an replaces by a custom hook. In this custom hook (which is inherited from the original code), the generator name can be modified from "darkfish" to something completely different and hence load the custom generator (which can inherit from Darkfish). The options could also be potentially modified to change the #setup_generator method.

Another option could be leave the generator as it is and have additional hook, which would run after the default generators and update the generated content. While tempting, I still think the generator/template should be extracted from RDoc and therefore this is less appealing option IMHO.


BTW we could also use the hook to remove the `--document=ri,rdoc` option from `%gem_install` macro [1]. But that would be probably too much magic.


Vít



[1] https://src.fedoraproject.org/rpms/ruby/blob/588a4ae9f02928d7bedbcf46a739d36b0a76e632/f/macros.rubygems#_31


_______________________________________________
ruby-sig mailing list -- ruby-sig@lists.fedoraproject.org
To unsubscribe send an email to ruby-sig-leave@lists.fedoraproject.org
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/ruby-sig@lists.fedoraproject.org
Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue