[Tapioca Addon] Support gem RBI generation #2081

alexcrocha · 2024-11-20T21:57:44Z

Motivation

To support gem RBI generation, we needed a way to detect changes in Gemfile.lock. Currently, changes to this file cause the Ruby LSP to restart, resulting in loss of access to any previous state information. This limitation prevents monitoring and processing of gem changes.

Implementation

We've implemented a solution that uses git diff to track Gemfile.lock changes:

Change Detection
- Added git_repo? check to ensure we're in a git repository
- Use git diff HEAD Gemfile.lock to detect uncommitted changes
- Parse the diff output to identify gem removals, additions or modifications
Gem Change Analysis
- Created LockfileDiffParser class to analyze diff output
- Identifies:
  - Removed gems (completely deleted)
  - Added or modified gems (new or version changes)
Gem RBI Processing
- Remove RBI files for deleted gems
- Triggers gem rbis generation command for added or modified gems

Key Components

Addon: Monitors Gemfile.lock changes and handles RBI file operations when changes are detected
LockfileDiffParser: Analyzes git diff output to identify added, modified, and removed gems

This approach specifically targets manual bundle updates while avoiding unnecessary RBI regeneration during normal git operations like branch switches.

Tests

lib/ruby_lsp/tapioca/addon.rb

lib/ruby_lsp/tapioca/server_addon.rb

lib/ruby_lsp/tapioca/addon.rb

KaanOzkan · 2024-11-21T14:44:56Z

lib/ruby_lsp/tapioca/addon.rb

+        current_lockfile = File.read("Gemfile.lock")
+        snapshot_lockfile = File.read(GEMFILE_LOCK_SNAPSHOT) if File.exist?(GEMFILE_LOCK_SNAPSHOT)
+
+        unless snapshot_lockfile


Have we compared the snapshot approach to git diff? I can think of one scenario where you update the lockfile before snapshot is created so upon the consequent restart there are changes in git status however, snapshot doesn't exist yet so we don't generate. It's not a common scenario so not important. But feels like running git diff instead is a viable solution. Curious about the reasoning here. Does it have downsides for changing branches?

Thank you for mentioning git diff again. I looked into it with more attention and I do agree it is a much nicer solution.

I have pushed a refactor to take advantage of git diff. Let me know what you think.

KaanOzkan · 2024-11-21T14:48:18Z

lib/ruby_lsp/tapioca/addon.rb

+      def handle_gemfile_changes
+        return unless File.exist?(".git")
+
+        gemfile_status = %x(git status --porcelain Gemfile.lock).strip


With shelling out and the file parsing I'm curious how long this adds to the boot time. Could you add some simple timings to the PR description, before and after this change for core?

This is now an older implementation, but I do agree some timings would be nice to have. I'll take a look on Monday.

lib/ruby_lsp/tapioca/addon.rb

lib/ruby_lsp/tapioca/server_addon.rb

lib/ruby_lsp/tapioca/addon.rb

KaanOzkan · 2024-11-25T15:11:49Z

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb

+      attr_reader :removed_gems
+
+      def initialize(diff_content)
+        @diff_content = diff_content


Suggested change

@diff_content = diff_content

@diff_content = diff_content.lines

If the only thing we do is call .lines on it we could do it here and do it only once instead of 2 calls below.

andyw8 · 2024-11-25T15:35:39Z

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb

+# typed: true
+# frozen_string_literal: true
+
+module RubyLsp


Nice work on this!

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb

lib/ruby_lsp/tapioca/server_addon.rb

andyw8 · 2024-11-26T16:30:45Z

Some feedback from testing. It's possible some won't need addressed:

I noticed the output from Tapioca is double-spaced (vertically) in the logs. It's likely that Ruby LSP itself is adding extra newlines unnecessarily.
Once the add-on succesfully creates/update some RBIs, it will so again on every restart and report them as identical, until committed/staged. We could be smart and check the new/changed RBIs files on disk but this may add unwanted complexity.

andyw8 · 2024-11-26T16:33:23Z

(Continued)

After running, we show a message in the logs "Please review changes and commit them." but users would not normally be reading that. We may want to consider showing an alert instead.
It would be nice if we could eliminate the FILE_HEADER_OPTION_DESC warnings in Tapioca to prevent them showing on startup.
I added a gem to the Gemfile and the add-on generated the RBI. Without committing, then removed the new entry from the Gemfile. The 'orphaned' RBI remained on disk.

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb

spec/tapioca/ruby_lsp/tapioca/lockfile_diff_parser_spec.rb

lib/ruby_lsp/tapioca/server_addon.rb

lib/ruby_lsp/tapioca/addon.rb

andyw8

Looks good but I think we need a few more tests for the main flows in addon.rb .

lib/ruby_lsp/tapioca/addon.rb

andyw8 · 2024-12-18T19:40:06Z

lib/ruby_lsp/tapioca/addon.rb

+
+      sig { returns(T::Boolean) }
+      def lockfile_changed?
+        !fetch_lockfile_diff.empty?


Suggested change

!fetch_lockfile_diff.empty?

fetch_lockfile_diff

!@lockfile_diff.empty?

(to avoid violating https://en.wikipedia.org/wiki/Command%E2%80%93query_separation )

If we're doing this then it might be better to assign the return value of fetch_lockfile_diff to the instance variable in lockfile_changed? instead. No strong opinions.

I was aiming to avoid any side-effects in the predicate method.

Main side effect is in fetch_lockfile_diff with the ivar which is triggered by the predicate method. I don't know the best solution but trying to move it above.

lib/ruby_lsp/tapioca/addon.rb

andyw8

@KaanOzkan I think it's in a pretty good shape and I've marked as approved for if you want to release. It does need some more tests, but it would valuable to ship it so that we can start getting feedback and iterating. We can expand on the tests while that is happening.

lib/ruby_lsp/tapioca/addon.rb

To support gem RBI generation, we needed a way to detect changes in Gemfile.lock. Currently, changes to this file cause the Ruby LSP to restart, resulting in loss of access to any previous state information. By running git diff on Gemfile.lock, we can detect changes to the file, and trigger the gem RBI generation process. Then we can parse the diff output to determine which gems have been removed, added or modified, and either remove the corresponding RBI files or trigger the RBI generation process for the gems.

KaanOzkan · 2024-12-19T15:11:33Z

lib/ruby_lsp/tapioca/addon.rb

+
+        if added_or_modified_gems.any?
+          # Resetting BUNDLE_GEMFILE to root folder to use the project's Gemfile instead of Ruby LSP's composed Gemfile
+          stdout, stderr, status = T.unsafe(Open3).capture3(


Tapioca doesn't log clearly before starting generating RBIs. I think it'll be good to mention here, something like:

Identified lockfile changes, attempting to generate gem RBIs

KaanOzkan · 2024-12-19T15:59:36Z

lib/ruby_lsp/tapioca/addon.rb

@@ -13,6 +13,7 @@
 end


One thing I noticed is we keep generating RBIs for updated gems. An ideal solution would be like GemSync where we determine that gem RBI is up to date and not generate. It may not be needed yet assuming restarts are rare and folks will stage their lockfile changes. Any thoughts?

alexcrocha added the enhancement New feature or request label Nov 20, 2024

KaanOzkan reviewed Nov 21, 2024

View reviewed changes

andyw8 reviewed Nov 21, 2024

View reviewed changes

lib/ruby_lsp/tapioca/addon.rb Outdated Show resolved Hide resolved

vinistock force-pushed the tapioca-addon-feature-branch branch from cc19ba0 to 3ce0a60 Compare November 22, 2024 14:23

alexcrocha force-pushed the ar/gem-regeneration-git-status branch from 351a2fb to f387c8d Compare November 22, 2024 19:10

andyw8 reviewed Nov 22, 2024

View reviewed changes

lib/ruby_lsp/tapioca/server_addon.rb Outdated Show resolved Hide resolved

andyw8 reviewed Nov 22, 2024

View reviewed changes

lib/ruby_lsp/tapioca/addon.rb Outdated Show resolved Hide resolved

KaanOzkan reviewed Nov 25, 2024

View reviewed changes

andyw8 reviewed Nov 25, 2024

View reviewed changes

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb Outdated Show resolved Hide resolved

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb Outdated Show resolved Hide resolved

andyw8 reviewed Nov 25, 2024

View reviewed changes

lib/ruby_lsp/tapioca/server_addon.rb Outdated Show resolved Hide resolved

alexcrocha force-pushed the ar/gem-regeneration-git-status branch from 7f173a8 to 5f3dc43 Compare November 26, 2024 02:27

alexcrocha changed the title ~~WIP: [Tapioca Addon] Support gem RBI generation on dirty lockfile~~ [Tapioca Addon] Support gem RBI generation Nov 26, 2024

andyw8 reviewed Nov 26, 2024

View reviewed changes

lib/ruby_lsp/tapioca/lockfile_diff_parser.rb Show resolved Hide resolved

alexcrocha mentioned this pull request Nov 26, 2024

WIP: [Tapioca Addon] Support gem RBI generation #2063

Closed

alexcrocha marked this pull request as ready for review November 27, 2024 20:15

alexcrocha requested a review from a team as a code owner November 27, 2024 20:15

alexcrocha requested review from KaanOzkan, andyw8 and vinistock and removed request for a team November 27, 2024 20:15

alexcrocha force-pushed the ar/gem-regeneration-git-status branch from 1098d0d to 21df7e6 Compare November 27, 2024 23:09

alexcrocha changed the base branch from tapioca-addon-feature-branch to main November 27, 2024 23:09

alexcrocha force-pushed the ar/gem-regeneration-git-status branch from 21df7e6 to ebfb33d Compare November 28, 2024 01:34

vinistock reviewed Nov 28, 2024

View reviewed changes

alexcrocha force-pushed the ar/gem-regeneration-git-status branch from ebfb33d to e19934f Compare November 30, 2024 00:40

alexcrocha force-pushed the ar/gem-regeneration-git-status branch 2 times, most recently from e2697ce to b28ebb0 Compare December 18, 2024 19:21

andyw8 requested changes Dec 18, 2024

View reviewed changes

andyw8 approved these changes Dec 18, 2024

View reviewed changes

andyw8 reviewed Dec 18, 2024

View reviewed changes

lib/ruby_lsp/tapioca/addon.rb Outdated Show resolved Hide resolved

andyw8 reviewed Dec 18, 2024

View reviewed changes

lib/ruby_lsp/tapioca/addon.rb Outdated Show resolved Hide resolved

alexcrocha and others added 5 commits December 18, 2024 14:40

Create lockfile diff parser

ca32192

Add test for lockfile diff parser

8524589

Cleanup orphaned RBIs

077b5c4

Refactor how we detect git repo

3ab954c

alexcrocha force-pushed the ar/gem-regeneration-git-status branch from b28ebb0 to 3ab954c Compare December 18, 2024 22:57

KaanOzkan reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tapioca Addon] Support gem RBI generation #2081

[Tapioca Addon] Support gem RBI generation #2081

alexcrocha commented Nov 20, 2024 •

edited

Loading

KaanOzkan Nov 21, 2024

alexcrocha Nov 23, 2024

KaanOzkan Nov 21, 2024

alexcrocha Nov 23, 2024

KaanOzkan Nov 25, 2024

andyw8 Nov 25, 2024

andyw8 commented Nov 26, 2024 •

edited

Loading

andyw8 commented Nov 26, 2024 •

edited

Loading

andyw8 left a comment

andyw8 Dec 18, 2024

KaanOzkan Dec 18, 2024

andyw8 Dec 19, 2024

KaanOzkan Dec 19, 2024 •

edited

Loading

andyw8 left a comment

KaanOzkan Dec 19, 2024

KaanOzkan Dec 19, 2024

	@diff_content = diff_content
	@diff_content = diff_content.lines

	!fetch_lockfile_diff.empty?
	fetch_lockfile_diff
	!@lockfile_diff.empty?

[Tapioca Addon] Support gem RBI generation #2081

Are you sure you want to change the base?

[Tapioca Addon] Support gem RBI generation #2081

Conversation

alexcrocha commented Nov 20, 2024 • edited Loading

Motivation

Implementation

Key Components

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Nov 26, 2024 • edited Loading

andyw8 commented Nov 26, 2024 • edited Loading

andyw8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaanOzkan Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

andyw8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrocha commented Nov 20, 2024 •

edited

Loading

andyw8 commented Nov 26, 2024 •

edited

Loading

andyw8 commented Nov 26, 2024 •

edited

Loading

KaanOzkan Dec 19, 2024 •

edited

Loading