eval_gemfile + LTS support #249

pboling · 2025-02-20T17:58:31Z

Adds back support for Ruby 1.8 - 2.2, but enhances the support with real, automated, tools to ensure syntax remains compatible with Ruby 1.8.

Also fixes several bugs / problem areas around the codebase that became apparent when adding back support. I'll go through this PR and annotate with comments, as it is very large. As much as I'd love to see this merged, I understand it may not be.

For my use case, with my version_gem gem, I need to use appraisal to test compatibility with Ruby v2.2 (and many of my other gems I plan to add appraisal to, particularly the RSpec plugins, support back to Ruby 1.8.7). I've tried a few other things, but they were all much worse solutions than doing this work. So if this doesn't get merged, an appraisal-lts gem fork might appear in the wild... :)

This is a combinataion of #248 and #250, which can be merged separately instead of this one.

NOTE: Hound is not paying attention to the RuboCop config. :(. Not very helpful.

- Fixes thoughtbot#154

spec/appraisal/utils_spec.rb

Gemfile

Rakefile

spec/acceptance/cli/install_spec.rb

spec/appraisal/appraisal_spec.rb

pboling · 2025-02-20T18:08:57Z

.rubocop_gradual.lock

+  "Gemfile:3311641552": [
+    [19, 3, 4, "Security/Eval: The use of `eval` is a serious security risk.", 2087429787],
+    [21, 3, 4, "Security/Eval: The use of `eval` is a serious security risk.", 2087429787],
+    [23, 3, 4, "Security/Eval: The use of `eval` is a serious security risk.", 2087429787]


I'm sure you are familiar with lockfiles, and how they are useful. Using a lockfile for RuboCop violations allows for gradually fixing violations in a way that the rubocop TODO pattern does not.

It's almost a perfect tool, and every project should be using it, IMO:
https://github.com/skryukov/rubocop-gradual

pboling · 2025-02-20T18:13:37Z

gemfiles/modular/style.gemfile

+# We run rubocop on the latest version of Ruby,
+#   but in support of the oldest supported version of Ruby
+
+gem "rubocop-lts", "~> 0.1", ">= 0.1.1" # Style and Linting support for Ruby >= 1.8


rubocop-lts is how I package all of the rules I've found to support every version of Ruby since v1.8, where every even version of the gem corresponds to a specific minor version of Ruby, (odd releases are deprecated, or non-extant).

See: https://rubocop-lts.gitlab.io/

pboling · 2025-02-20T18:14:34Z

lib/appraisal/appraisal.rb

@@ -150,7 +150,7 @@ def lockfile_path
    end

    def clean_name
-      name.gsub(/[^\w\.]/, "_")


. does not need to be escaped inside [] brakcets.

pboling · 2025-02-20T18:15:52Z

lib/appraisal/appraisal_file.rb

-      end
+      raise AppraisalsNotFound unless File.exist?(path)
+
+      run(File.read(path))


File.read is safer than IO.read (according to whoever made the rule for RuboCop).

pboling · 2025-02-20T18:17:44Z

lib/appraisal/bundler_dsl.rb

@@ -114,7 +139,7 @@ def ruby_version_entry

      case @ruby_version
      when String then "ruby #{@ruby_version.inspect}"
-      else "ruby(#{@ruby_version.inspect})"


This was a minor bug, as it was not calling Utils.format_string, as many of the other similar methods in this class do (and they are correct for so doing). Some of the variability in output between Ruby versions was due to this.

pboling · 2025-02-20T18:19:44Z

lib/appraisal/command.rb

-        end
-      end
+      ERROR
+      exit(1)


The control flow is much easier to understand now.

pboling · 2025-02-20T18:20:33Z

lib/appraisal/customize.rb

      @@single_quotes = single_quotes
    end

-    def self.heading(gemfile)
+    def self.heading(gemfile = nil)


The RSPec tests for this method were failing, and marked as pending because this was a bug!

pboling · 2025-02-20T18:21:15Z

lib/appraisal/customize.rb

-    def self.customize(heading, gemfile)
-      return nil unless heading
+    def self.customize(topper, gemfile)
+      return unless topper


Stop shadowing the heading method.

pboling · 2025-02-20T18:21:49Z

lib/appraisal/gemfile.rb

@@ -14,7 +14,7 @@ module Appraisal
  # Load bundler Gemfiles and merge dependencies
  class Gemfile < BundlerDSL
    def load(path)
-      run(IO.read(path), path) if File.exist?(path)
+      run(File.read(path), path) if File.exist?(path)


File.read is safer than IO.read (again).

pboling · 2025-02-20T18:24:03Z

lib/appraisal/ordered_hash.rb

+  # a complete implementation, but it covers Appraisal's specific needs.
+  class OrderedHash < ::Hash
+    # Hashes are ordered in Ruby 1.9.
+    if RUBY_VERSION < "1.9"


This is the only RUBY_VERSION check of any kind in the runtime code in this PR.

There are still some checks in the test code, so that the old versions of ruby can be properly tested.

Maintaining support for Ruby v1.8+ isn't (currently) hard at all, especially with the new syntax and linting support from rubocop-lts that autocorrects code (like hashes to hash rockets) to enforce Ruby 1.8 syntax. I run it on many gems supporting Ruby 1.8, and have submitted a number of related fixes upstream to both standard, and rubocop.

pboling · 2025-02-20T18:27:42Z

lib/appraisal/platform.rb

-        platforms #{Utils.format_arguments(@platform_names)} do
-        #{output_dependencies}
-        end
+      <<-OUTPUT.strip


<<~ Squiggly HEREDOC support was added in Ruby 2.3, so for now HEREDOCS are a bit ugly.

pboling · 2025-02-20T18:29:45Z

lib/appraisal/utils.rb

    end

    def self.join_parts(parts)
      parts.reject(&:nil?).reject(&:empty?).join("\n\n").strip
    end

    def self.prefix_path(path)
-      if path !~ /^(?:\/|\S:)/ && path !~ /^\S+:\/\// && path !~ /^\S+@\S+:/
-        cleaned_path = path.gsub(/(^|\/)\.(?:\/|$)/, "\\1")
+      if path !~ %r{^(?:/|\S:)} && path !~ %r{^\S+://} && path !~ /^\S+@\S+:/


Much simplified regexes, thanks to the %r{} syntax.

lib/appraisal/group.rb

lib/appraisal/source.rb

lib/appraisal/gemspec.rb

lib/appraisal/git.rb

lib/appraisal/utils.rb

spec/acceptance/bundle_with_custom_path_spec.rb

spec/appraisal/appraisal_spec.rb

spec/appraisal/customize_spec.rb

pboling · 2025-02-20T18:35:50Z

spec/acceptance/bundle_without_spec.rb

-  it "passes --without flag to Bundler on install" do
+RSpec.describe "Bundle without group" do
+  it "config set --local without group is honored by Bundler" do
+    pending "config set --local without group support seems broken, see: https://github.com/rubygems/rubygems/issues/8518"


Literally this is broken, reported bug to bundler:
rubygems/rubygems#8518

pboling · 2025-02-20T18:38:44Z

spec/support/dependency_helpers.rb

+      %x[git init . --initial-branch=master]
+      %x[git config user.email "appraisal@thoughtbot.com"]
+      %x[git config user.name "Appraisal"]
+      %x[git config commit.gpgsign false]


On my local machine I have git configured to sign all commits, which was breaking the test suite here, because it couldn't sign the commit two lines below. Adding this config to prevent signing for this ephemeral gem fixed the issue.

This reverts commit 92d5134.

lib/appraisal/appraisal.rb

lib/appraisal/cli.rb

n-rodriguez · 2025-02-20T19:50:20Z

@pboling thanks for your amazing work! 👍

Also fixes several bugs / problem areas around the codebase that became apparent when adding back support. I'll go through this PR and annotate with comments, as it is very large. As much as I'd love to see this merged, I understand it may not be.

Is it possible to split this PR in 2? one for eval_gemfile and one for LTS? thank you!

pboling · 2025-02-20T20:17:00Z

It sort of already is. This one sits on top of theeval_gemfile PR. The build failures in the other PR are not related to the PR, and are actually just a broken CI build, which I am sure would fail similarly on master branch. If that other PR is merged, then this one will shrink.

I'll make a new PR with the LTS changes that is not stacked. Then perhaps it could be merged first, and improve the CI build.

pboling · 2025-02-20T20:38:30Z

@n-rodriguez done, see: #248 & #250

n-rodriguez · 2025-02-20T20:33:59Z

spec/support/dependency_helpers.rb

-        puts "building gem: #{gem_name} #{version}" if ENV["VERBOSE"]
-        `gem build #{gemspec} #{redirect}`
+      puts "building gem: #{gem_name} #{version}" if ENV["VERBOSE"]
+      %x[gem build #{gemspec} #{redirect}]


Can you please use %x()? It reads better than %x[] or %x{}. Thank you!

Fixed in #250

n-rodriguez · 2025-02-20T20:34:13Z

spec/support/dependency_helpers.rb

-        puts "installing gem: #{gem_name} #{version}" if ENV["VERBOSE"]
-        `gem install -lN #{gem_name}-#{version}.gem -v #{version} #{redirect}`
+      puts "installing gem: #{gem_name} #{version}" if ENV["VERBOSE"]
+      %x[gem install -lN #{gem_name}-#{version}.gem -v #{version} #{redirect}]



Fixed in #250

n-rodriguez · 2025-02-20T20:34:28Z

spec/support/dependency_helpers.rb

+      %x[git config user.name "Appraisal"]
+      %x[git config commit.gpgsign false]
+      %x[git add .]
+      %x[git commit --all --no-verify --message "initial commit"]


Fixed in #250

pboling added 3 commits February 3, 2025 22:12

✨ Support eval_gemfile

0ffe705

- Fixes thoughtbot#154

🚨 Linting

bc81d5d

⬆️ Ruby 3.4

05d6d9c

hound bot reviewed Feb 20, 2025

View reviewed changes

pboling force-pushed the eval_gemfile-lts branch from e98c74d to 7fe7c2c Compare February 20, 2025 18:02

hound bot reviewed Feb 20, 2025

View reviewed changes

pboling commented Feb 20, 2025

View reviewed changes

lib/appraisal/command.rb

end

end

ERROR

exit(1)

Copy link

Contributor Author

pboling Feb 20, 2025

Choose a reason for hiding this comment

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

The control flow is much easier to understand now.

pboling commented Feb 20, 2025

View reviewed changes

pboling force-pushed the eval_gemfile-lts branch from 7fe7c2c to 6c9d0be Compare February 20, 2025 18:33

hound bot reviewed Feb 20, 2025

View reviewed changes

pboling commented Feb 20, 2025

View reviewed changes

Revert "Drop MRI 1.8.7-2.2 support (thoughtbot#158)"

dde608f

This reverts commit 92d5134.

pboling force-pushed the eval_gemfile-lts branch from 6c9d0be to dde608f Compare February 20, 2025 18:46

hound bot reviewed Feb 20, 2025

View reviewed changes

pboling mentioned this pull request Feb 20, 2025

✨ Support eval_gemfile #248

Open

n-rodriguez suggested changes Feb 20, 2025

View reviewed changes

pboling closed this Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eval_gemfile + LTS support #249

eval_gemfile + LTS support #249

pboling commented Feb 20, 2025 •

edited

Loading

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025 •

edited

Loading

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025 •

edited

Loading

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025

pboling Feb 20, 2025 •

edited

Loading

n-rodriguez commented Feb 20, 2025

pboling commented Feb 20, 2025 •

edited

Loading

pboling commented Feb 20, 2025 •

edited

Loading

n-rodriguez Feb 20, 2025

pboling Feb 20, 2025

n-rodriguez Feb 20, 2025

pboling Feb 20, 2025

n-rodriguez Feb 20, 2025

pboling Feb 20, 2025

eval_gemfile + LTS support #249

eval_gemfile + LTS support #249

Conversation

pboling commented Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboling Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboling Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboling Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

n-rodriguez commented Feb 20, 2025

pboling commented Feb 20, 2025 • edited Loading

pboling commented Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pboling commented Feb 20, 2025 •

edited

Loading

pboling Feb 20, 2025 •

edited

Loading

pboling Feb 20, 2025 •

edited

Loading

pboling Feb 20, 2025 •

edited

Loading

pboling commented Feb 20, 2025 •

edited

Loading

pboling commented Feb 20, 2025 •

edited

Loading