Dne 24. 11. 22 v 13:12 Jarek Prokop napsal(a):
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 think that it used to be valid identifier at some point in time. This
should be fixed in rubygem-rdoc then:
https://src.fedoraproject.org/rpms/rubygem-rdoc/c/5e165eed4822757b49dd3b1...
but mainly upstream:
https://github.com/ruby/rdoc/issues/924
I am maybe coming too soon with this comment
Maybe just a bit ;)
, but since we are also on the SPDX topic in parallel ;).
Yeah, IC.
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=...
>
>
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`
Eh, have I left the message ^^ there? :)
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/rubyg...
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
~~~
Good catch. I was testing plain `--document=rdoc`.
Hm, there should be probably done some early matching, if the `fedora::`
prefixed generator is there. Otherwise just move on with whatever was
specified.
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.
I have not spend too much time thinking about %check section yet, but
something along these lines seems reasonable.
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/fedor...
?
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.
Yes, that was probably just oversight. Will fix this.
> 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".
Thx 👍
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 :).
Lets leave future problems for future us. So far, there is no other RDoc
generator and I don't see any other generator coming any time soon, so
lets ignore the `rubygem-` prefix for now. This still might end up as a
gem after all :)
> PTAL and let me know what do you think about this approach. Thx
>
I'd say we are on the right path with this.
Thx. Next thing to consider is how to enforce the dependency on the
-assets. There should probably be included some generator, otherwise we
would need to update gem2rpm as well as every gem.
Also, the other question is how to pull this into the build root. Should
this be pulled in by rubygems-devel? Shouldn't it even be part of
rubygems-devel? Or shouldn't this be opt-in, where one needs to specify
this explicitly? Maybe just from the beginning unless we are sure this
does not cause more troubles then it solves?
Vít