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

Drop Ruby 2.3 support. #403

merged 1 commit into from
Feb 8, 2022

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented Feb 7, 2022

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:

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:

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, bootsnap 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.

Solution

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.

#402

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
@casperisfine casperisfine merged commit 8b1b45f into main Feb 8, 2022
@casperisfine casperisfine deleted the drop-2.3-support branch February 8, 2022 08:19
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems February 8, 2022 08:22 Inactive
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.

2 participants