-
Notifications
You must be signed in to change notification settings - Fork 183
Comparing changes
Open a pull request
base repository: Shopify/bootsnap
base: v1.10.3
head repository: Shopify/bootsnap
compare: v1.11.1
- 15 commits
- 22 files changed
- 4 contributors
Commits on Feb 7, 2022
-
Configuration menu - View commit details
-
Copy full SHA for 4d78e68 - Browse repository at this point
Copy the full SHA 4d78e68View commit details -
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
Configuration menu - View commit details
-
Copy full SHA for 88b8cd4 - Browse repository at this point
Copy the full SHA 88b8cd4View commit details
Commits on Feb 8, 2022
-
Merge pull request #403 from Shopify/drop-2.3-support
Drop Ruby 2.3 support.
Configuration menu - View commit details
-
Copy full SHA for 8b1b45f - Browse repository at this point
Copy the full SHA 8b1b45fView commit details -
Get rid of RealPathCache and the
Kernel.require_relative
decoratorRef: #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
Configuration menu - View commit details
-
Copy full SHA for fb857fe - Browse repository at this point
Copy the full SHA fb857feView commit details -
Merge pull request #402 from Shopify/skip-realpath-cache-if-useless
Get rid of RealPathCache and the `Kernel.require_relative` decorator
Configuration menu - View commit details
-
Copy full SHA for 5476c07 - Browse repository at this point
Copy the full SHA 5476c07View commit details
Commits on Feb 16, 2022
-
Configuration menu - View commit details
-
Copy full SHA for 8f9c7f1 - Browse repository at this point
Copy the full SHA 8f9c7f1View commit details -
Configuration menu - View commit details
-
Copy full SHA for 18b701a - Browse repository at this point
Copy the full SHA 18b701aView commit details -
Bind
rb_get_path
to better respect Kernel#require duck-typeKernel#require turns its argument into a path by calling `to_path` if it responds to it, and then into a String (the argument or the return value of `to_path`) by using implicit conversion (`to_str`). Co-authored-by: Étienne Barrié <etienne.barrie@gmail.com>
Configuration menu - View commit details
-
Copy full SHA for 671d1d4 - Browse repository at this point
Copy the full SHA 671d1d4View commit details -
Merge pull request #406 from Shopify/rb-get-path
Bind `rb_get_path` to better respect Kernel#require duck-type
Configuration menu - View commit details
-
Copy full SHA for cb1adae - Browse repository at this point
Copy the full SHA cb1adaeView commit details
Commits on Feb 22, 2022
-
Configuration menu - View commit details
-
Copy full SHA for 4f60408 - Browse repository at this point
Copy the full SHA 4f60408View commit details -
Fix: #404 But also it's annoying to have delayed `require` like this one.
Configuration menu - View commit details
-
Copy full SHA for 843c101 - Browse repository at this point
Copy the full SHA 843c101View commit details -
Merge pull request #407 from Shopify/no-file-utils
Drop dependency on FileUtils
Configuration menu - View commit details
-
Copy full SHA for ad68ea5 - Browse repository at this point
Copy the full SHA ad68ea5View commit details
Commits on Mar 8, 2022
-
Configuration menu - View commit details
-
Copy full SHA for c365322 - Browse repository at this point
Copy the full SHA c365322View commit details -
Fix
can't modify frozen Hash
error.Fix: #411 The load path cache can be mutated, so we need to unfreeze the hash when it happens.
Configuration menu - View commit details
-
Copy full SHA for 94e56f9 - Browse repository at this point
Copy the full SHA 94e56f9View commit details -
Configuration menu - View commit details
-
Copy full SHA for 5bb937a - Browse repository at this point
Copy the full SHA 5bb937aView commit details
This comparison is taking too long to generate.
Unfortunately it looks like we can’t render this comparison for you right now. It might be too big, or there might be something weird with your repository.
You can try running this command locally to see the comparison on your machine:
git diff v1.10.3...v1.11.1