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

alexcrocha · 2024-10-31T01:23:48Z

WIP

This Draft PR is still a work in progress.

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

To address this, a snapshot file of Gemfile.lock is created to persist data across server restarts. Upon restart, both the snapshot and the current Gemfile.lock are parsed using Bundler::LockfileParser. If differences are detected, the relevant gem names and specifications are extracted, allowing us to then trigger the gem RBI generation.

This implementation handles gem additions, spec changes, and gem removals.

Tests

By leveraging file checksums using `Zlib.crc32`, we can avoid triggering the DSL generation process for files that have not changed.

Co-authored-by: Andy Waite <[email protected]>

vinistock · 2024-10-31T12:00:06Z

Independent of the code changes themselves, I still have doubts about the UX here. Take this scenario for example:

You are working on a branch developing some feature
Now, you need to rebase your branch on the latest main
The latest main has lockfile changes, but those weren't accompanied by the necessary RBI updates
After the rebase, the LSP will immediately generate gem RBIs that are completely unrelated to your work, resulting in untracked changes

I believe we need to try to come up with a strategy to be able to tell if the current developer made the update to the lockfile vs it being updated by something else (like git pull/switching branches/rebasing). Otherwise, it will result in a pretty annoying experience to constantly get unrelated changes.

Maybe, we could remember the launch timestamp of the add-on. When the lockfile is updated, it will trigger a restart and then the add-on can check if the update to the lockfile happened after the previous launch (which should indicate that the current developer made the update). I don't know, there might be better options on how to achieve this.

alexcrocha · 2024-11-01T16:21:58Z

@vinistock, thanks for pointing this out. I agree that the current behaviour could quickly become frustrating, especially when rebasing and getting unrelated gem RBIs.

I appreciate your suggestion about using the launch timestamp of the add-on to distinguish between lockfile updates made by the developer and those from other actions. I'll experiment with that idea and explore other strategies. Your feedback has given me a lot to think about.

Thanks again for your input!

Do not reload rake tasks when under add-on environment

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 creating a snapshot of Gemfile.lock, we can persist data across server restarts. Upon restart, we parse both the snapshot and current Gemfile.lock using Bundler::LockfileParser. If differences are found, we extract the relevant gem names and specifications, allowing us to trigger the gem RBI generation.

Skip gem RBI generation by comparing the last modified times of the Gemfile and Gemfile.lock. If both files are modified within a fraction of a second of each other, it is likely due to a git operation, and gem RBI generation can be skipped.

If the Ruby LSP was stopped shortly after the last git checkout/pull operation, we don't need to regenerate RBIs since any Gemfile.lock changes were likely from version control, not from bundle install

andyw8 · 2024-11-19T15:50:12Z

lib/ruby_lsp/tapioca/addon.rb

+        return unless File.exist?(".git") && File.exist?(".ruby-lsp/shutdown-timestamp")
+
+        git_timestamp = fetch_last_git_operation_time
+        return unless git_timestamp


I think this isn't right.

Let's say you're working on a new repo, where the reflog doesn't contain any checkout or pull entries. fetch_last_git_operation_time will return nil, and so we will never reach process_gemfile_changes.

andyw8 · 2024-11-19T15:51:31Z

spec/tapioca/cli/dsl_spec.rb

@@ -1760,7 +1760,7 @@ def self.gather_constants

        it "generates RBIs for lower versions of activerecord-typedstore" do
          @project.require_real_gem("activerecord-typedstore", "1.4.0")
-          @project.require_real_gem("sqlite3", "1.7.3")
+          @project.require_real_gem("sqlite3", "~> 2.1")


(looks like something was incorrectly rebased?)

andyw8 · 2024-11-19T16:17:49Z

lib/ruby_lsp/tapioca/addon.rb

+
+      sig { returns(T.nilable(Time)) }
+      def fetch_last_git_operation_time
+        git_reflog_output = %x(git reflog --date=iso | grep -E "checkout|pull" | head -n 1).strip


Suggested change

git_reflog_output = %x(git reflog --date=iso | grep -E "checkout|pull" | head -n 1).strip

git_reflog_output = %x(git reflog --date=iso --grep-reflog='^\(checkout\|pull\)' -1).strip

This approach has a couple of advantages:

It helps prevent mismatches due to a commit messages co-incidentally containing the word 'checkout' or 'pull'. (The ^ means match only at the start of the line).

It avoids the need for external programs which may not be available on Windows.

andyw8 · 2024-11-19T16:30:52Z

lib/ruby_lsp/tapioca/addon.rb

+
+      sig { void }
+      def handle_gemfile_changes
+        return unless File.exist?(".git") && File.exist?(".ruby-lsp/shutdown-timestamp")


Let's extract this path to a constant since we refer to it multiple times.

andyw8 · 2024-11-19T16:32:47Z

lib/ruby_lsp/tapioca/addon.rb

@@ -19,6 +19,9 @@ module Tapioca
    class Addon < ::RubyLsp::Addon
      extend T::Sig

+      GEMFILE_LOCK_SNAPSHOT = "tmp/tapioca/.gemfile_lock_snapshot"
+      GIT_OPERATION_THRESHOLD = 15.0 # seconds


Suggested change

GIT_OPERATION_THRESHOLD = 15.0 # seconds

GIT_OPERATION_THRESHOLD = 15 # seconds

andyw8 · 2024-11-19T16:40:04Z

lib/ruby_lsp/tapioca/addon.rb

+        return if git_reflog_output.empty?
+
+        timestamp_string = T.must(git_reflog_output.match(/\{(.*)\}/))[1]
+        Time.iso8601(T.must(timestamp_string).sub(" ", "T").delete(" "))


We could avoid the need for this parsing by setting the output so it contains only the timestamp:

git reflog ..... --format=tformat:'%cI'

(cI means strict ISO, so we don't need to add the 'T' ourselves).

alexcrocha · 2024-11-26T19:40:37Z

Closed in favour of #2081

vinistock and others added 18 commits October 24, 2024 16:51

Add ruby-lsp and ruby-lsp-rails to Gemfile for add-on development

7e0772c

Add requires to generate ruby-lsp related RBIs

2c61cf4

Generate ruby-lsp RBIs

cc95f0e

Prevent re-definition error for source URI

d322305

Add add-on boiler plate

e7e8f96

[Tapioca Add-on] Trigger DSL generation upon file changes

1303433

Bump ruby-lsp & ruby-lsp-rails requirements

e75b7c3

Bump ruby-lsp-rails dependency to v0.3.18

dfaa8ba

Add Server Addon boilerplate

a58bec6

Generate DSL RBIs in add-on mode

1837b6a

Temporarily remove experimental feature requirement

35eca20

Temporarily remove another experimental features guard

2169eca

Fix bug: ensure flat_map does not return nil elements

0b7ce68

Skip test files in watched change handler

b13cb58

Log tapioca addon activation

f1aeeaa

Bump depend_on_ruby_lsp! version

9b1714c

Skip processing if the file hasn't changed

7e68f5c

By leveraging file checksums using `Zlib.crc32`, we can avoid triggering the DSL generation process for files that have not changed.

Do not reload rake tasks when under add-on environment

f976ea6

Co-authored-by: Andy Waite <[email protected]>

alexcrocha added the enhancement New feature or request label Oct 31, 2024

vinistock and others added 2 commits November 4, 2024 11:52

Merge pull request #2061 from Shopify/vs-do-not-reload-rake-tasks

cc19ba0

Do not reload rake tasks when under add-on environment

alexcrocha force-pushed the ar/gem-regeneration branch from 1952bd0 to 32b6afc Compare November 7, 2024 07:43

alexcrocha force-pushed the ar/gem-regeneration branch 2 times, most recently from 898a5aa to 52045e7 Compare November 9, 2024 00:58

alexcrocha added 2 commits November 18, 2024 16:20

WIP Skip RBI regeneration after git operations

0f5138c

If the Ruby LSP was stopped shortly after the last git checkout/pull operation, we don't need to regenerate RBIs since any Gemfile.lock changes were likely from version control, not from bundle install

Refactor skip RBI regeneration after git op

8cfe769

alexcrocha force-pushed the ar/gem-regeneration branch from 52045e7 to 8cfe769 Compare November 19, 2024 00:21

WIP Fix failing tests

ff9ccec

alexcrocha force-pushed the ar/gem-regeneration branch from 09b2e33 to ff9ccec Compare November 19, 2024 02:21

andyw8 reviewed Nov 19, 2024

View reviewed changes

alexcrocha mentioned this pull request Nov 20, 2024

[Tapioca Addon] Support gem RBI generation #2081

Draft

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

alexcrocha closed this Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexcrocha commented Oct 31, 2024

vinistock commented Oct 31, 2024

alexcrocha commented Nov 1, 2024

andyw8 Nov 19, 2024

andyw8 Nov 19, 2024

andyw8 Nov 19, 2024

andyw8 Nov 19, 2024

andyw8 Nov 19, 2024

andyw8 Nov 19, 2024

alexcrocha commented Nov 26, 2024

	git_reflog_output = %x(git reflog --date=iso \| grep -E "checkout\|pull" \| head -n 1).strip
	git_reflog_output = %x(git reflog --date=iso --grep-reflog='^\(checkout\\|pull\)' -1).strip

	GIT_OPERATION_THRESHOLD = 15.0 # seconds
	GIT_OPERATION_THRESHOLD = 15 # seconds

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

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

Conversation

alexcrocha commented Oct 31, 2024

WIP

Motivation

Implementation

Tests

vinistock commented Oct 31, 2024

alexcrocha commented Nov 1, 2024

andyw8 Nov 19, 2024

Choose a reason for hiding this comment

andyw8 Nov 19, 2024

Choose a reason for hiding this comment

andyw8 Nov 19, 2024

Choose a reason for hiding this comment

andyw8 Nov 19, 2024

Choose a reason for hiding this comment

andyw8 Nov 19, 2024

Choose a reason for hiding this comment

andyw8 Nov 19, 2024

Choose a reason for hiding this comment

alexcrocha commented Nov 26, 2024