Skip to content
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.

Sign up for GitHub

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

Run CI against minimum supported Sorbet version #1252

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Run CI against minimum supported Sorbet version #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e034d3b
Match net-smtp versions across gemfiles
sambostock Oct 31, 2022
45b9e73
Refactor gemfiles
sambostock Oct 31, 2022
0fc3114
Use actual sorbet-static-and-runtime version
sambostock Nov 8, 2022
1937577
Run CI against minimum supported Sorbet version
sambostock Oct 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ jobs:
- Gemfile
- gemfiles/Gemfile-rails-6-1
- gemfiles/Gemfile-rails-main
- gemfiles/Gemfile-sorbet-minimum
include:
- gemfile: gemfiles/Gemfile-rails-main
experimental: true
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ group(:development, :test) do
gem("smart_properties", require: false)
gem("frozen_record", require: false)
gem("sprockets", require: false)
gem("rails", require: false)
gem("rails", require: false) unless defined?(@specified_rails)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced on the need to pollute the default Gemfile just for the sake of avoiding duplication in the gemfiles used for tests.

What do you think about creating an ERB template for the test gemfiles that could be parameterized based on the Rails version we want to test and the sorbet version parsed from the gemspec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that if we do that, we still end up duplicating the rest of the Gemfile, which leads to it becoming out of sync (as it currently is on main).

I based this approach off the approach taken by Shopify/maintenance_tasks, although I used instance variables differently due to differences in the arguments to gem (specifically, the presence of require: false).

Specifically with regards to using a template, that would mean we'd have to have a step in CI that renders the template before bundle install reads it, but that would necessarily mean doing it before installing any gems, which might be problematic.

Honestly, the cleanest solution would be if we were able to do something like

eval_gemfile "../Gemfile" # Containing a regular gemfile

override do
  # No complaint about duplicate gem specification
  gem("rails", "~> 6.1.0", require: false)
end

But AFAIK Bundler doesn't support anything like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO removing that duplication (and the chance for them to get out of sync) justifies the change here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying this is better, but in this project I use env var to control which version to use (from Rails 5 to main).

Pros:

  • We only need 1 Gemfile and logics are concentrated
  • No warnings AFAICT

Cons:

  • If conditions

gem("state_machines", require: false)
gem("activerecord-typedstore", require: false)
gem("sqlite3")
Expand Down
2 changes: 1 addition & 1 deletion Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ PATH
netrc (>= 0.11.0)
parallel (>= 1.21.0)
rbi (~> 0.0.0, >= 0.0.16)
sorbet-static-and-runtime (>= 0.5.9892)
sorbet-static-and-runtime (>= 0.5.9896)
spoom (~> 1.1.0, >= 1.1.11)
thor (>= 1.2.0)
yard-sorbet
Expand Down
56 changes: 3 additions & 53 deletions gemfiles/Gemfile-rails-6-1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,56 +1,6 @@
# frozen_string_literal: true

source("https://rubygems.org")
gem("rails", "~> 6.1.0", require: false)
@specified_rails = true

gemspec path: ".."

gem("minitest")
gem("minitest-hooks")
gem("minitest-reporters")
gem("pry-byebug")
gem("rubocop-shopify", require: false)
gem("rubocop-sorbet", ">= 0.4.1")
gem("rubocop-rspec", require: false)
gem("ruby-lsp", require: false)

group(:deployment, :development) do
gem("rake")
end

group(:development, :test) do
gem("smart_properties", require: false)
gem("frozen_record", require: false)
gem("sprockets", require: false)
gem("rails", "~> 6.1.0", require: false)
gem("state_machines", require: false)
gem("activerecord-typedstore", require: false)
gem("sqlite3")
gem("identity_cache", require: false)
gem(
"cityhash",
git: "https://github.com/csfrancis/cityhash.git",
ref: "3cfc7d01f333c01811d5e834f1495eaa29f87c36",
require: false
)
gem("activeresource", require: false)
gem("google-protobuf", require: false)
gem("graphql", require: false)
gem("shopify-money", require: false)
gem("sidekiq", require: false)
gem("nokogiri", require: false)
gem("config", github: "rubyconfig/config", branch: "master", require: false)
gem("aasm", require: false)
gem("bcrypt", require: false)
gem("xpath", require: false)

# net-smtp was removed from default gems in Ruby 3.1, but is used by the `mail` gem.
# So we need to add it as a dependency until `mail` is fixed:
# https://github.com/rails/rails/blob/0919aa97260ab8240150278d3b07a1547489e3fd/Gemfile#L178-L191
gem("net-smtp", "0.3.1", require: false)
end

group :test do
gem("webmock")
end

gem "kramdown", "~> 2.4"
eval_gemfile "../Gemfile"
56 changes: 3 additions & 53 deletions gemfiles/Gemfile-rails-main
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,56 +1,6 @@
# frozen_string_literal: true

source("https://rubygems.org")
gem("rails", github: "rails/rails", branch: "main", require: false)
@specified_rails = true

gemspec path: ".."

gem("minitest")
gem("minitest-hooks")
gem("minitest-reporters")
gem("pry-byebug")
gem("rubocop-shopify", require: false)
gem("rubocop-sorbet", ">= 0.4.1")
gem("rubocop-rspec", require: false)
gem("ruby-lsp", require: false)

group(:deployment, :development) do
gem("rake")
end

group(:development, :test) do
gem("smart_properties", require: false)
gem("frozen_record", require: false)
gem("sprockets", require: false)
gem("rails", github: "rails/rails", branch: "main", require: false)
gem("state_machines", require: false)
gem("activerecord-typedstore", require: false)
gem("sqlite3")
gem("identity_cache", require: false)
gem(
"cityhash",
git: "https://github.com/csfrancis/cityhash.git",
ref: "3cfc7d01f333c01811d5e834f1495eaa29f87c36",
require: false
)
gem("activeresource", require: false)
gem("google-protobuf", require: false)
gem("graphql", require: false)
gem("shopify-money", require: false)
gem("sidekiq", require: false)
gem("nokogiri", require: false)
gem("config", github: "rubyconfig/config", branch: "master", require: false)
gem("aasm", require: false)
gem("bcrypt", require: false)
gem("xpath", require: false)

# net-smtp was removed from default gems in Ruby 3.1, but is used by the `mail` gem.
# So we need to add it as a dependency until `mail` is fixed:
# https://github.com/rails/rails/blob/0919aa97260ab8240150278d3b07a1547489e3fd/Gemfile#L178-L191
gem("net-smtp", "0.3.1", require: false)
end

group :test do
gem("webmock")
end

gem "kramdown", "~> 2.4"
eval_gemfile "../Gemfile"
13 changes: 13 additions & 0 deletions gemfiles/Gemfile-sorbet-minimum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

# Extract minimum supported version from gemspec
minimum_sorbet_version = File.read("tapioca.gemspec")[
# spec.add_dependency("sorbet-static-and-runtime", ">= 1.2.3456")
# ^^^^^^^^
/"sorbet-static-and-runtime", ">= ([^"]+)"/,
1,
]

gem("sorbet-static-and-runtime", minimum_sorbet_version)

eval_gemfile "../Gemfile"
2 changes: 1 addition & 1 deletion tapioca.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Gem::Specification.new do |spec|
spec.add_dependency("netrc", ">= 0.11.0")
spec.add_dependency("parallel", ">= 1.21.0")
spec.add_dependency("rbi", "~> 0.0.0", ">= 0.0.16")
spec.add_dependency("sorbet-static-and-runtime", ">= 0.5.9892")
spec.add_dependency("sorbet-static-and-runtime", ">= 0.5.9896")
spec.add_dependency("spoom", "~> 1.1.0", ">= 1.1.11")
spec.add_dependency("thor", ">= 1.2.0")
spec.add_dependency("yard-sorbet")
Expand Down