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.

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

Conversation

sambostock
Copy link
Contributor

Motivation

In #1046 (comment), there was a proposal to use a syntax that would not have been supported on the version of Sorbet we list as our minimum. It occurred to me that something in CI should exist to catch that sort of thing.

Therefore, this PR's goal is to extend the CI matrix to run against the minimum version of Sorbet which we claim to support. This way, we're able to detect incompatibilities as they are introduced, and either alter the implementation or version requirement, keeping the gemspec accurate.

Implementation

  • The existing gemfiles are refactored to reduce duplication.
  • A new gemfile is introduced which specifies an exact version of sorbet-static-and-runtime matching the minimum version we list in gemspec.
    • Ideally this would be dynamic somehow, so we don't have to update it.

Tests

No new tests should need to be introduced, but this branch actually already discovers that the version we specify as our minimum doesn't actually exist.

I suspect we were previously specifying matching versions of sorbet, sorbet-static, and sorbet-runtime, which we then migrated to sorbet-static-and-runtime (keeping the same version number), without realizing the version number started later than our "current" version.

@@ -0,0 +1,5 @@
# frozen_string_literal: true

gem("sorbet-static-and-runtime", "0.5.9204")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted in my description, this matches the minimum version we specify in tapioca.gemspec (">= 0.5.9204").

  1. This version does not exist.
    We should either switch to a version number that does exist, or fallback to specifying Sorbet dependencies individually.
  2. Ideally we'd do this dynamically so this file doesn't need to be kept in sync.
    Perhaps simplest to just do string matching on the Gemfile and extract the version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For 1. I think we should first create a PR that bumps the version to an existing one. The first version for sorbet-static-and-runtime is v0.5.9519 which is almost a year old. We can bump to something more recent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened #1260 which starts by bumping to the first version.

What version do you think we should use as the requirement? We could go with the latest, but I'm not sure if we want to force the update for consumers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep the one you used in #1260 for the time being 👍

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To tie up loose ends, #1260 ended up bumping us to the first version that allows us to remove our feature flag checks (because all features are supported).

spec.add_dependency("sorbet-static-and-runtime", ">= 0.5.9892")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I had to nudge it forward again to 0.5.9896, because 0.5.9892 doesn't actually exist either 😅

@@ -21,7 +21,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

The main Gemfile specifies 0.3.2, but the other two specify 0.3.1.
Rather than duplicate the entire file, we can use eval_gemfile.
@sambostock sambostock force-pushed the ci-sorbet-compatibility branch 2 times, most recently from ec00d0a to 7e081ce Compare November 8, 2022 19:40
0.5.9896 is the first published version that is actually >= 0.5.9892
Extract minimum Sorbet version from gemspec and run against that to make
sure we're actually compatible.
@sambostock sambostock marked this pull request as ready for review November 8, 2022 20:06
@sambostock sambostock requested a review from a team as a code owner November 8, 2022 20:06
@@ -21,7 +21,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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants