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

Drop Ruby 2.3 support. #403

Merged
merged 1 commit into from
Feb 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Drop Ruby 2.3 support.
Ref: #402
Ref: https://bugs.ruby-lang.org/issues/10222
Ref: ruby/ruby@5754f15
Ref: ruby/ruby@b6d3927

Up to Ruby 2.3, `require` would resolve symlinks, but `require_relative` wouldn't:

```ruby
require 'fileutils'
FileUtils.mkdir_p("realpath")
File.write("realpath/a.rb", "p :a_loaded")
File.symlink("realpath", "symlink") rescue nil

$LOAD_PATH.unshift(File.realpath(__dir__) + "/symlink")
require "a.rb" # load symlink/a.rb in 2.3 and older, load realpath/a.rb on 2.4 and newer
require_relative "realpath/a.rb" # noop on 2.4+
```

This would easily cause double loading issue when `require` and `require_relative`
were mixed, but was fixed in 2.4 (https://bugs.ruby-lang.org/issues/10222).

The problem is that `Bootsnap` kinda negated this fix, because `realpath()`
wouldn't be applied to absolute paths:

```ruby
require 'fileutils'
FileUtils.mkdir_p("realpath")
File.write("realpath/a.rb", "p :a_loaded")
File.symlink("realpath", "symlink") rescue nil

$LOAD_PATH.unshift(File.realpath(__dir__) + "/symlink")
require File.expand_path("symlink/a.rb") # load symlink/a.rb in 3.0 and older, load realpath/a.rb on 3.1 and newer
require_relative "realpath/a.rb" # noop on 3.1+
```

And for performance reasons, Bootsnap tried really hard not to call `realpath`,
as it's a syscall, instead it used `expand_path`, which is entirely in use
space and doesn't reach to the file system. So if you had a `symlink` in
`$LOAD_PATH`, `bootcsnap` would perpetuate this bug, which led to the
addition of #136.

This was ultimately fixed in Ruby 3.1 (https://bugs.ruby-lang.org/issues/17885),
now `realpath` is applied even on absolute paths.

While `realpath` is indeed expensive, I think the performance impact is ok if
we only call it for `$LOAD_PATH` members, rather than for all requirable files.
So if you have X gems, it's going to be more or less X `realpath` calls.

It would stay a problem if a gem actually contained symlinks and used
`require_relative`, but it's quite the stretch, and with 3.1 now
handling it, it's not worth keeping such workaround.

See: #402
  • Loading branch information
byroot committed Feb 7, 2022
commit 88b8cd4464bc601667f8d9c90038895497e57e4e
4 changes: 2 additions & 2 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ jobs:
- uses: actions/checkout@v2
- uses: ruby/setup-ruby@v1
with:
ruby-version: '2.3'
ruby-version: '2.4'
bundler-cache: true
- run: bundle exec rubocop

rubies:
strategy:
matrix:
os: [ubuntu]
ruby: ['2.3', '2.4', '2.5', '2.6', '2.7', '3.0', '3.1', 'ruby-head', 'debug']
ruby: ['2.4', '2.5', '2.6', '2.7', '3.0', '3.1', 'ruby-head', 'debug']
runs-on: ${{ matrix.os }}-latest
steps:
- uses: actions/checkout@v2
Expand Down
125 changes: 65 additions & 60 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,17 @@ AllCops:
Exclude:
- 'vendor/**/*'
- 'tmp/**/*'
TargetRubyVersion: 2.3
TargetRubyVersion: 2.4

# This doesn't take into account retrying from an exception
Lint/SuppressedException:
Enabled: false

# allow String.new to create mutable strings
Style/EmptyLiteral:
Enabled: false

Style/EmptyMethod:
Bundler/OrderedGems:
Enabled: false

# allow the use of globals which makes sense in a CLI app like this
Style/GlobalVars:
Gemspec/OrderedDependencies:
Enabled: false

Style/PercentLiteralDelimiters:
Gemspec/DuplicatedAssignment:
Enabled: false

Style/TrailingCommaInHashLiteral:
EnforcedStyleForMultiline: comma

Style/TrailingCommaInArrayLiteral:
EnforcedStyleForMultiline: comma

Style/TrailingCommaInArguments:
EnforcedStyleForMultiline: comma

Layout/LineLength:
Max: 120

Metrics/AbcSize:
Enabled: false

Expand Down Expand Up @@ -65,33 +44,12 @@ Naming/MethodName:
Naming/RescuedExceptionsVariableName:
PreferredName: error

Bundler/OrderedGems:
Enabled: false

Gemspec/OrderedDependencies:
Enabled: false

Gemspec/DuplicatedAssignment:
Naming/VariableNumber:
Enabled: false

Layout/MultilineMethodCallIndentation:
EnforcedStyle: indented

Style/SymbolArray:
Enabled: false

Style/StderrPuts:
Enabled: false

Style/ModuleFunction:
Enabled: false

Style/IfUnlessModifier:
Enabled: false

Style/GuardClause:
Enabled: false

Layout/EndAlignment:
EnforcedStyleAlignWith: start_of_line

Expand All @@ -101,24 +59,78 @@ Layout/RescueEnsureAlignment:
Layout/FirstHashElementIndentation:
EnforcedStyle: consistent

Style/NumericPredicate:
Enabled: false

Layout/SpaceInsideHashLiteralBraces:
EnforcedStyle: no_space

Layout/SpaceAroundMethodCallOperator:
Enabled: true

Layout/LineLength:
Max: 120

# This doesn't take into account retrying from an exception
Lint/SuppressedException:
Enabled: false

Lint/AssignmentInCondition:
AllowSafeAssignment: true

Lint/UnusedMethodArgument:
AllowUnusedKeywordArguments: true

Lint/RaiseException:
Enabled: true

Lint/StructNewOverride:
Enabled: true

Security/MarshalLoad:
Enabled: false

Security/YAMLLoad:
Enabled: false

# allow String.new to create mutable strings
Style/EmptyLiteral:
Enabled: false

Style/EmptyMethod:
Enabled: false

# allow the use of globals which makes sense in a CLI app like this
Style/GlobalVars:
Enabled: false

Style/PercentLiteralDelimiters:
Enabled: false

Style/TrailingCommaInHashLiteral:
EnforcedStyleForMultiline: comma

Style/TrailingCommaInArrayLiteral:
EnforcedStyleForMultiline: comma

Style/TrailingCommaInArguments:
EnforcedStyleForMultiline: comma

Style/SymbolArray:
Enabled: false

Style/StderrPuts:
Enabled: false

Style/ModuleFunction:
Enabled: false

Style/IfUnlessModifier:
Enabled: false

Style/GuardClause:
Enabled: false

Style/NumericPredicate:
Enabled: false

Style/Alias:
EnforcedStyle: prefer_alias_method

Expand All @@ -131,22 +143,12 @@ Style/DoubleNegation:
Style/CommentedKeyword:
Enabled: false

Naming/VariableNumber:
Enabled: false

Style/Next:
Enabled: false

Style/StringLiterals:
EnforcedStyle: double_quotes


Lint/RaiseException:
Enabled: true

Lint/StructNewOverride:
Enabled: true

Style/HashEachMethods:
Enabled: true

Expand All @@ -161,3 +163,6 @@ Style/RedundantReturn:

Style/YodaCondition:
Enabled: false

Style/ExponentialNotation:
Enabled: true
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Unreleased

* Drop support for Ruby 2.3.

# 1.10.3

* Fix Regexp and Date type support in YAML compile cache. (#400)
Expand Down
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ gem "minitest", "~> 5.0"
gem "mocha", "~> 1.2"

group :development do
gem "rubocop", "0.81.0" # Ruby 2.3 support
gem "rubocop", "0.82.0" # Ruby 2.4 support
gem "byebug", platform: :ruby
end
2 changes: 1 addition & 1 deletion bootsnap.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Gem::Specification.new do |spec|
spec.bindir = "exe"
spec.executables = %w(bootsnap)

spec.required_ruby_version = ">= 2.3.0"
spec.required_ruby_version = ">= 2.4.0"

if RUBY_PLATFORM =~ /java/
spec.platform = "java"
Expand Down
4 changes: 1 addition & 3 deletions lib/bootsnap/compile_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ def self.permission_error(path)

def self.supported?
# only enable on 'ruby' (MRI), POSIX (darwin, linux, *bsd), Windows (RubyInstaller2) and >= 2.3.0
RUBY_ENGINE == "ruby" &&
RUBY_PLATFORM =~ /darwin|linux|bsd|mswin|mingw|cygwin/ &&
Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("2.3.0")
RUBY_ENGINE == "ruby" && RUBY_PLATFORM.match?(/darwin|linux|bsd|mswin|mingw|cygwin/)
end
end
end
4 changes: 2 additions & 2 deletions test/compile_cache_key_format_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ def test_key_ruby_revision
def test_key_size
key = cache_key_for_file(FILE)
exp = File.size(FILE)
act = key[R[:size]].unpack("Q")[0]
act = key[R[:size]].unpack1("Q")
assert_equal(exp, act)
end

def test_key_mtime
key = cache_key_for_file(FILE)
exp = File.mtime(FILE).to_i
act = key[R[:mtime]].unpack("Q")[0]
act = key[R[:mtime]].unpack1("Q")
assert_equal(exp, act)
end

Expand Down