Skip to content

Commit

Permalink
Drop Ruby 2.3 support.
Browse files Browse the repository at this point in the history
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
1 parent 4d78e68 commit 88b8cd4
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 69 deletions.
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

0 comments on commit 88b8cd4

Please sign in to comment.