-
Notifications
You must be signed in to change notification settings - Fork 440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make it loose coupling between RubyGems and RDoc #1171
base: master
Are you sure you want to change the base?
Conversation
\### Problems There are following problems because of tight coupling between RubyGems and RDoc. 1. If there are braking changes in RDoc, RubyGems is also broken. 2. When we maintain RDoc, we have to change RubyGems. The reason why they are happened is that RubyGems creates documents about a gem with installing it. Note that RubyGems uses functions of RDoc to create documents. Specifically, - Creating documents is executed by `rubygems/lib/rubygems/rdoc.rb`. - `::RDoc::RubygemsHook` which is defined by RDoc is called by the file. \### Solution RubyGems has the plugin system. If a gem includes `rubygems_plugin.rb`, RubyGems loads it. RubyGems executes a process defined in it while installing gems, uninstalling gems or other events. We can use the system to solve the problems. The root cause is RubyGems directly references the class of RDoc. We can remove the root cause by making RDoc RubyGems plugin. Alternatively `rubygems_plugin.rb` creates documents about gems. \### FAQ Q1. Do we need to change codes of RubyGems? A. No, we don't. This change keeps compatibility of API used from RubyGems. Q2. Is it better to delete existing codes related to RDoc in RubyGems? No, it isn't. If we change codes of RubyGems, we can't keep a compatibility. Example: If we delete codes that uses `RDoc::RubygemsHook` in `rubygems/lib/rubygems/rdoc.rb`, documentations are not created with old RDoc. Q3. When can we delete `rubygems/lib/rubygems/rdoc.rb`? A. We can delete it when all users use RDoc including `rubygems_plugin`. Next ruby version is 3.4. If it includes the RDoc including `rubygems_plugin`, we can delete `rubygems/lib/rubygems/rdoc.rb` after ruby 3.3 is EOL. Q4. Is it a breaking change that Rubygems creates documents with rubygems_plugin not RDoc::RubygemsHook? A. No, it isn't. If we simply implement this approach, we move the implementation from `rdoc/lib/rdoc/rubygems_hook.rb` to `rubygems_plugin.rb`. This way can be breaking change. It seems to be fine that we just need to delete `rdoc/rubygems_hook.rb` but it doesn't work. It generates multiple documents. `rubygems/lib/rubygems/rdoc.rb` has the following code. ``` begin require "rdoc/rubygems_hook" # ... rescue LoadError end ``` This code ignores RDoc related processes when `rdoc/rubygems_hook` can't be required. But, this 'require' is not failed. This is because Ruby installs Rdoc as a default gem. So, Rdoc installed as a default gem generates documents and one installed as a normal gem does it too. If you think that this behavior is accectable, we can just delete `rdoc/rubygems_hook.rb`. What do you think about this approach? In this change, we take another approach to solve the problem that creates multiple documents. If `Gem.done_installing(&Gem::RDoc.method(:generation_hook))` in `rubygems/rdoc.rb` doesn't create documents, we can solve the problem. We have some options. * We change `rubygems/rdoc.rb` and then don't execute `Gem.done_installing`. (This is a change for RubyGems.) * We change `rdoc/rubygems_hook.rb` and then make `generation_hook` a no-op method. (This is a change for RDoc.) We choose the latter to avoid changing for RubyGems. \### Test \#### Preparation Install Rdoc which including our changes by executing `rake install`. ❯ rake install We confirmed that Rdoc which including our changes was installed. ❯ gem list | grep rdoc rdoc (6.6.0, default: 6.4.0) \#### Check point We tested to check compatibility. How to chack the compatibility? We tested creating same documents by our RDoc and old RDoc with latest RubyGems. We used following versions to test. ``` ❯ ruby -v ruby 3.1.0p0 (2021-12-25 revision fb4df44d16) [arm64-darwin22] ❯ gem list | grep rdoc rdoc (default: 6.4.0) ❯ ruby -I rubygems/lib rubygems/exe/gem --version 3.5.14 ``` Here is a result of test with old RDoc. We can see that the document is created correctlly with `Parsing...` and `Done installing...`. ``` ❯ ruby -I rubygems/lib rubygems/exe/gem install pkg-config Successfully installed pkg-config-1.5.6 Parsing documentation for pkg-config-1.5.6 Done installing documentation for pkg-config after 0 seconds 1 gem installed ``` Here is a result of test with our RDoc. We can see that the document is created correctlly with `Parsing...` and `Done installing...`. ``` ❯ ruby -I rubygems/lib rubygems/exe/gem install pkg-config Successfully installed pkg-config-1.5.6 Parsing documentation for pkg-config-1.5.6 Done installing documentation for pkg-config after 0 seconds 1 gem installed ``` As you can see we got the same results, our RDoc keeps compatibility.
FYI: RubyGems wants to remove RDoc integration code if possible: |
Thank you @mterada1228 @kou. My current understanding is: this PR will allow RubyGems to remove RDoc specific code, because RDoc will start using RubyGems plugin (its official API) to generate documentation for gems. Is this correct? |
Yes, it is. Your understanding is correct. |
Correct as @mterada1228 said. Note:
RDoc will be a bundled gem (not a default gem) since Ruby 3.5: https://bugs.ruby-lang.org/issues/20309 |
Hello! From RubyGems maintainers team perspective, I think this is a nice thought out PR, and I'm very happy it will allow us to eventually remove RDoc code from RubyGems ❤️. Thank you! |
Are there people who want to review this PR? |
I'm +1 to this change. But I have some concerns. Is this working with fresh installation of ruby package? In my understanding, the current rubygems plugin system could work with How generate |
Does "fresh installation" mean that RDoc is installed as a default gem? |
Yes.
I see. Thanks. We make document generation is disabled with "RDoc is installed as a default gem" case like Ruby 3.4? |
Does this mean the following? If we ship this change in Ruby 3.4, whether The answer is that |
Really? https://github.com/ruby/rdoc/pull/1171/files#diff-57670ce1156b5670b904c97f8a01d658a0b5a458ffe71f2fcc7a7eefd4254d2cR15 seems empty. Could you explain how work |
Ah, wait. It may not work. |
@hsbt raised a good point. I think ruby-core default gem installer may need to make sure to generate plugins for default gems too, because as of now, no default gem includes rubygems plugins and I believe the mechanism is not going to work by default. |
I checked the behavior in the following case. When RDoc includes this changes was used as default gem, How to check
❯ mv $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc_bkup # for backup
❯ cp -r ./lib/rdoc $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc
❯ gem install foo
Fetching foo.gem
Successfully installed foo
1 gem installed Documents was not created. SolutionsOne solution is that If Although It is possible that I make default gems able to handle rubygems plugins, this approach is maybe too much. |
Thanks for checking the case that RDoc is installed as a default gem. I agree that merging this after Ruby 3.4 is an option. We'll migrate RDoc to a bundled gem from a default gem for Ruby 3.5: https://bugs.ruby-lang.org/issues/20309 For merging this before Ruby 3.4, mterada1228#1 may work. (Sorry. I didn't test it yet. It's a concept implementation.)
It's not complex. If this works well, this approach may be an option.
|
Thank you for your PR. I want to try whether it may work or not, but the branch have to merge some fix in the master branch to try it. |
@mterada1228 You want to try mterada1228#1 on https://github.com/ruby/rdoc/tree/master , right? Can we try mterada1228#1 without merging it to https://github.com/ruby/rdoc/tree/master ? $ git close https://github.com/kou/rdoc.git rdoc.kou
$ cd rdoc.kou
$ git switch add-rubygems-hook-default-gem
$ ... |
Sorry, My explanation was so bad... Thank you for your operation. |
Co-authored-by: mterada1228 <[email protected]>
Add support for the case that RDoc is installed as a default gem
I merged @kou's proposal mterada1228#1. Test1. RDoc is installed as normal gem.Setup❯ rake install
rdoc 6.7.0 built to pkg/rdoc-6.7.0.gem.
rdoc (6.7.0) installed.
❯ gem list | grep rdoc
rdoc (6.7.0, default: 6.6.3.1) Execution❯ gem install pkg-config
Fetching pkg-config-1.5.7.gem
Successfully installed pkg-config-1.5.7
Parsing documentation for pkg-config-1.5.7
Installing ri documentation for pkg-config-1.5.7
Done installing documentation for pkg-config after 0 seconds
1 gem installed 2. RDoc is installed as default gemSetup# For backup
❯ mv $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc_bkup
❯ mv $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc.rb $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc.rb.bkup
# Copy sources to directory of default gems
❯ cp -r ./lib/rdoc $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc
❯ cp -r ./lib/rdoc.rb $(ruby -e 'print Gem::RUBYGEMS_DIR')/rdoc.rb
# Check
❯ gem list | grep rdoc
rdoc (default: 6.6.3.1) Execution❯ gem install pkg-config
Successfully installed pkg-config-1.5.7
Parsing documentation for pkg-config-1.5.7
Done installing documentation for pkg-config after 0 seconds
1 gem installed |
56651c0
to
535c5e3
Compare
This is ready again. This should be worked when RDoc is installed as a default gem too. In the case, the existing integration logic in RubyGems (not RubyGems plugin logic) is used. Are there any other concerns? |
# If generate and remove are executed at that time, an error will occur. | ||
# So, we can't register generate and remove to Gem at that time. | ||
begin | ||
require_relative 'rdoc/markdown' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rdoc/rd/block_parser
and rdoc/rd/inline_parser
have similar problems to markdown too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
We can use one of rdoc/markdown
, rdoc/rd/block_parser
or rdoc/rd/inline_parser
here.
This just wants to fix a bootstrap problem.
We want to run bundle install
when we start developing cloned ruby/rdoc.
In the time, this lib/rubygems_plugin.rb
is used. But RDoc isn't ready yet because rdoc/markdown.rb
and so on aren't generated yet. So bundle install
is failed.
This disables this lib/rubygems_plugin.rb
only when the initial bundle install
to fix the bootstrap problem.
# To install dependency libraries of RDoc, you need to run bundle install. | ||
# At that time, rdoc/markdown is not generated. | ||
# If generate and remove are executed at that time, an error will occur. | ||
# So, we can't register generate and remove to Gem at that time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what this comment intends to express. Do you mind elaborate it a bit more?
@@ -0,0 +1,15 @@ | |||
# frozen_string_literal: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add comments to explain when and by what this file will be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comment.
Can you check that?
@mterada1228 can you also update the description to reflect the latest changes? Thanks 😄 |
4954e4f
to
42b86d1
Compare
Thank you for your comment. I changed a description of this PR. 1
2
3 |
Problems
There are following problems because of tight coupling between RubyGems and RDoc.
The reason why they are happened is that RubyGems creates documents about a gem with installing it.
Note that RubyGems uses functions of RDoc to create documents. Specifically,
rubygems/lib/rubygems/rdoc.rb
.::RDoc::RubygemsHook
which is defined by RDoc is called by the file.Solution
RubyGems has the plugin system.
If a gem includes
rubygems_plugin.rb
, RubyGems loads it. RubyGems executes a process defined in it while installing gems, uninstalling gems or other events.We can use the system to solve the problems.
The root cause is RubyGems directly references the class of RDoc.
We can remove the root cause by making RDoc RubyGems plugin.
Alternatively
rubygems_plugin.rb
creates documents about gems.FAQ
Q1. Do we need to change codes of RubyGems?
A.
No, we don't.
This change keeps compatibility of API used from RubyGems.
Q2. Is it better to delete existing codes related to RDoc in RubyGems?
No, it isn't.
If we change codes of RubyGems,
we can't keep a compatibility.
Example:
If we delete codes that uses
RDoc::RubygemsHook
inrubygems/lib/rubygems/rdoc.rb
, documentations are not created with old RDoc.Q3. When can we delete
rubygems/lib/rubygems/rdoc.rb
?A.
We can delete it when all users use RDoc including
rubygems_plugin
.Default gems and old RDoc just don't use
rubygems_plugins
.So, When following conditions are completed, we can delete
rubygems/lib/rubygems/rdoc.rb
.Q4. Is it a breaking change that Rubygems creates documents with rubygems_plugin not RDoc::RubygemsHook?
A.
No, it isn't.
If we simply implement this approach,
we move the implementation from
rdoc/lib/rdoc/rubygems_hook.rb
torubygems_plugin.rb
.This way can be breaking change.
It seems to be fine that we just need to delete
rdoc/rubygems_hook.rb
but it doesn't work. It generates multiple documents.rubygems/lib/rubygems/rdoc.rb
has the following code.This code ignores RDoc related processes when
rdoc/rubygems_hook
can't be required. But, this 'require' is not failed.This is because Ruby installs Rdoc as a default gem.
So, Rdoc installed as a default gem generates documents and one installed as a normal gem does it too.
If you think that this behavior is accectable,
we can just delete
rdoc/rubygems_hook.rb
.What do you think about this approach?
In this change, we take another approach to solve the problem that creates multiple documents.
If
Gem.done_installing(&Gem::RDoc.method(:generation_hook))
inrubygems/rdoc.rb
doesn't create documents, we can solve the problem.We have some options.
rubygems/rdoc.rb
and then don't executeGem.done_installing
. (This is a change on RubyGems.)rdoc/rubygems_hook.rb
and then makegeneration_hook
a method that create documents when there is notrubygems_plugin.rb
. (This is a change on RDoc.)We choose the latter to avoid changing on RubyGems.
Test
Preparation
Install Rdoc which including our changes by executing
rake install
.❯ rake install
We confirmed that Rdoc which including our changes was installed.
❯ gem list | grep rdoc
rdoc (6.6.0, default: 6.4.0)
Check point
We tested to check compatibility.
How to chack the compatibility?
We tested creating same documents by our RDoc and old RDoc with latest RubyGems.
We used following versions to test.
Here is a result of test with old RDoc.
We can see that the document is created correctlly with
Parsing...
andDone installing...
.Here is a result of test with our RDoc.
We can see that the document is created correctlly with
Parsing...
andDone installing...
.As you can see we got the same results, our RDoc keeps compatibility.
Test for RDoc as default gem
Setup
Result