Sorbet integration #310

kostyanf14 · 2024-02-22T17:07:27Z

Please ignore everything from the sorbet folder during a review.

akihikodaki

It doesn't make sense to check in RBIs when you don't check in gems.

Gemfile

lib/engines/hckinstall/hckinstall.rb

bin/tapioca

lib/engines/hckinstall/hckinstall.rb

lib/engines/hcktest/hcktest.rb

akihikodaki · 2024-02-23T10:16:04Z

.github/workflows/check.yml

+  typecheck:
+    runs-on: ubuntu-20.04
+    strategy:
+      fail-fast: false
+
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v4
+
+    - name: Update Ubuntu package repository
+      run: sudo apt-get update
+    - name: Setup sqlite-devel
+      run: sudo apt-get -y install libsqlite3-dev
+    - name: Setup libcurl-devel
+      run: sudo apt-get -y install libcurl4 libcurl3-gnutls libcurl4-openssl-dev
+
+    - name: Set up Ruby 3.1.4
+      uses: ruby/setup-ruby@v1
+      with:
+        ruby-version: 3.1.4
+        bundler-cache: false
+
+    - name: Install dependencies
+      run: bundle install
+
+    - name: Run sorbet type check
+      run: bundle exec srb typecheck
+


We already have different jobs for rspec and rubocop, but they actually should be one job; we can save many lines and computation time for setup in that way.

I expect to run all jobs even if one fails. How do you propose to combine it?

Add continue-on-error for each of typecheck, rspec, and rubocop. You can later check if all of the steps succeeded.

spec/srb_json_load_spec.rb

akihikodaki · 2024-02-23T10:37:46Z

Thanks for providing a proof of concept. This is very informative.

Having two definitions for one JSON is terrible so we should pick one. I have mixed feelings for Sorbet, but I think the utility of its static type checker supasses its disadvantages.

We can write a converter to Sorbet if we want to write a schema in another language, but it's hard to support Sorbet's static type checker with such a converter. Supporting its runtime type check-ing is easy since we can generate a type definition as Ruby objects on Ruby, but static type checking requires the generated definitions to be saved as Ruby code, and that's very complicated.

The other way around is easy in contrary. If we will have a frontend written in JavaScript and need to consume Sorbet code, we can traverse the types and generate JSON, which is only a matter of calling JSON.dump.

Honestly, I don't like some syntactical aspects of Sorbet; it requires typing T:: a lot, and the enum idiom does not make sense, but I think we can tolerate that.

kostyanf14 · 2024-02-29T17:26:46Z

@akihikodaki I don't touch workflow-related commit, I want to investigate the ability to simplify it. As this is not related to Sorbet in general, let's move the deduplication part outside this PR.

kostyanf14 · 2024-02-29T17:38:42Z

I think these changes are good enough to start with Sorbet.

If there are no critical problems that we can see now, let's continue in the next way:

merge this PR
remove JTD related code
add models for other objects that we get from JSON
enable typing in other parts of the source

lib/auxiliary/json_helper.rb

sorbet/config

spec/srb_json_load_spec.rb

lib/setupmanagers/qemuhck/qemuhck.rb

akihikodaki · 2024-03-01T03:59:17Z

I think these changes are good enough to start with Sorbet.

If there are no critical problems that we can see now, let's continue in the next way:
1. merge this PR

2. remove JTD related code

3. add models for other objects that we get from JSON

4. enable typing in other parts of the source

That sounds good to me.

akihikodaki

I think we should also run tapioca gems --verify. Their documentation says it breaks Dependabot, but it apparently does not. dependabot-core itself uses Dependabot with Sorbet and it seems just fine: https://github.com/dependabot/dependabot-core/issues?page=3&q=label%3Adependencies+label%3Aruby+is%3Aclosed

lib/auxiliary/json_helper.rb

Gemfile

lib/setupmanagers/qemuhck/qemuhck.rb

spec/srb_json_load_spec.rb

kostyanf14 · 2024-03-06T12:57:52Z

webrick
Please try to remove it and see if it works.

When I update gems and run tapioca gem to regenerate RBIs, tapioca fails with the error webrick is not found. You may need to 'gem install webrick' to install webrick.

kostyanf14 · 2024-03-06T13:02:14Z

I think we should also run tapioca gems --verify. Their documentation says it breaks Dependabot, but it apparently does not. dependabot-core itself uses Dependabot with Sorbet and it seems just fine: https://github.com/dependabot/dependabot-core/issues?page=3&q=label%3Adependencies+label%3Aruby+is%3Aclosed

I think they mean this problem dependabot/dependabot-core#5962. Need to download tapioca definitions after updating a gem.

akihikodaki · 2024-03-06T13:09:30Z

I think we should also run tapioca gems --verify. Their documentation says it breaks Dependabot, but it apparently does not. dependabot-core itself uses Dependabot with Sorbet and it seems just fine: https://github.com/dependabot/dependabot-core/issues?page=3&q=label%3Adependencies+label%3Aruby+is%3Aclosed

I think they mean this problem dependabot/dependabot-core#5962. Need to download tapioca definitions after updating a gem.

Yes, what I don't get is that https://github.com/dependabot/dependabot-core/ doesn't have such code yet it seems working.

akihikodaki · 2024-03-06T13:24:43Z

webrick
Please try to remove it and see if it works.

When I update gems and run tapioca gem to regenerate RBIs, tapioca fails with the error webrick is not found. You may need to 'gem install webrick' to install webrick.

Excluding rdoc fixed the error. Add it to sorbet/tapioca/config.yml and note that RDoc::Servlet needs webrick, which we don't have.

akihikodaki

Things I have pointed out earlier are all correctly resolved. I still have a few comments though.

Gemfile

spec/srb_json_load_spec.rb

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

kostyanf14 requested review from YanVugenfirer, akihikodaki and Jedoku February 22, 2024 17:07

akihikodaki requested changes Feb 23, 2024

View reviewed changes

akihikodaki reviewed Feb 23, 2024

View reviewed changes

spec/srb_json_load_spec.rb Outdated Show resolved Hide resolved

kostyanf14 force-pushed the sorbet branch from 830d31e to 0e78b77 Compare February 29, 2024 17:19

akihikodaki requested changes Mar 1, 2024

View reviewed changes

kostyanf14 force-pushed the sorbet branch 4 times, most recently from 695ef94 to d5fd9b1 Compare March 6, 2024 12:03

kostyanf14 requested a review from akihikodaki March 6, 2024 12:05

kostyanf14 changed the title ~~RFC: Sorbet integration~~ Sorbet integration Mar 6, 2024

akihikodaki requested changes Mar 6, 2024

View reviewed changes

lib/auxiliary/json_helper.rb Show resolved Hide resolved

Gemfile Outdated Show resolved Hide resolved

lib/setupmanagers/qemuhck/qemuhck.rb Outdated Show resolved Hide resolved

spec/srb_json_load_spec.rb Show resolved Hide resolved

kostyanf14 force-pushed the sorbet branch from d5fd9b1 to 593258c Compare March 6, 2024 13:50

kostyanf14 requested a review from akihikodaki March 6, 2024 13:50

akihikodaki requested changes Mar 6, 2024

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

spec/srb_json_load_spec.rb Outdated Show resolved Hide resolved

spec/srb_json_load_spec.rb Outdated Show resolved Hide resolved

kostyanf14 force-pushed the sorbet branch from 593258c to dcba5e2 Compare March 6, 2024 14:30

kostyanf14 added 4 commits March 12, 2024 16:44

Init sorber and tapioca

f887af1

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Enable sorbet runtime check

62e9a7c

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Json helper can read file without logger

020156e

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Define JSON loading helper for Sorbet structs

8ab4d22

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

kostyanf14 added 6 commits March 12, 2024 16:44

Define driver struct

1785d0b

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Enable types in hcktest.rb

2c58ec1

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Enable types in hckinstall.rb

07bc09d

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

Use driver struct instread of hash

f48faaf

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

CI: Enable Sorbet typecheck

db1c10a

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

rspec: Check that all jsons can be loaded into struct

88aa947

Signed-off-by: Kostiantyn Kostiuk <[email protected]>

kostyanf14 force-pushed the sorbet branch from dcba5e2 to 88aa947 Compare March 12, 2024 14:45

kostyanf14 requested a review from akihikodaki March 12, 2024 14:45

akihikodaki approved these changes Mar 14, 2024

View reviewed changes

Jedoku approved these changes Mar 14, 2024

View reviewed changes

kostyanf14 merged commit 5dbc025 into HCK-CI:master Mar 14, 2024
6 checks passed

kostyanf14 deleted the sorbet branch March 14, 2024 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sorbet integration #310

Sorbet integration #310

kostyanf14 commented Feb 22, 2024

akihikodaki left a comment

akihikodaki Feb 23, 2024

kostyanf14 Feb 26, 2024

akihikodaki Feb 27, 2024

akihikodaki commented Feb 23, 2024

kostyanf14 commented Feb 29, 2024

kostyanf14 commented Feb 29, 2024

akihikodaki commented Mar 1, 2024

akihikodaki left a comment

kostyanf14 commented Mar 6, 2024

kostyanf14 commented Mar 6, 2024

akihikodaki commented Mar 6, 2024

akihikodaki commented Mar 6, 2024

akihikodaki left a comment

Sorbet integration #310

Sorbet integration #310

Conversation

kostyanf14 commented Feb 22, 2024

akihikodaki left a comment

Choose a reason for hiding this comment

akihikodaki Feb 23, 2024

Choose a reason for hiding this comment

kostyanf14 Feb 26, 2024

Choose a reason for hiding this comment

akihikodaki Feb 27, 2024

Choose a reason for hiding this comment

akihikodaki commented Feb 23, 2024

kostyanf14 commented Feb 29, 2024

kostyanf14 commented Feb 29, 2024

akihikodaki commented Mar 1, 2024

akihikodaki left a comment

Choose a reason for hiding this comment

kostyanf14 commented Mar 6, 2024

kostyanf14 commented Mar 6, 2024

akihikodaki commented Mar 6, 2024

akihikodaki commented Mar 6, 2024

akihikodaki left a comment

Choose a reason for hiding this comment