8000 Revert "Drop MRI 1.8.7-2.2 support (#158)" by pboling · Pull Request #250 · thoughtbot/appraisal · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

< 8000 bdi class="js-issue-title markdown-title">Revert "Drop MRI 1.8.7-2.2 support (#158)" #250

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

pboling
Copy link
Contributor
@pboling pboling commented Feb 20, 2025

This reverts commit 92d5134.

Also fixes a few bugs.

  • indentation was inconsistent across various BundlerDSL types
  • Ruby <= 2.7 will never work with gems sourced from git on local filesystem, because old bundler (2.4.22) doesn't support it.
  • Adds Ruby 2.7, 3.4 to CI, and they are passing
  • Fixes CI for existing builds for Ruby 3.0, 3.1, 3.2, 3.3, ruby-head
  • Adds Ruby 2.6 to CI, and it is failing, as will any lower Ruby version. I propose we fix them in a separate PR so the CI is in a better state on the main branch.
  • Independently verified that these changes result in compatibility with every MRI Ruby 1.9.3+, and JRuby 9.1+ in a gem I pinned to use a branch on my fork that integrates these changes with the eval_gemfile feature.

Also fixes other CI consistency issues, and adds lint / style enforcement for Ruby 1.8+ via rubocop-lts.

end
end
ERROR
exit(1)
Copy link

Choose a reason for hiding this comment

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

Rails/Exit: Do not use exit in Rails applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow Hound is a mess. 🤯

bin/bundle Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to lint this file : it is generated and about to disappear rubygems/rubygems#8345

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 the complication is the linting is automated when running the default rake task now, and violations will clog up the new lint lockfile. This will encourage us to get rid of files we don't use!

run: |
case ${RUBY_VERSION} in
1.8|1.9|2.0|2.1|2.2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add EOL Rubies in matrix:ruby above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's my next task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding them one by one now...

@n-rodriguez
Copy link
Contributor

@pboling thank you! 👍
@nickcharlton LGTM

- Support debug for debugging with Ruby >= 2.7
- Support byebug for debugging with Ruby < 2.7
…d matrix

- all current jruby and truffleruby are compatible with Ruby 3.1 at minimum, thus are supported by latest bundler
- Use strip_heredoc uniformly in acceptance tests
- Differences in handling of whitespace between versions of Ruby are often seen as minor, and go unnoticed unless tests explicitly test white space as appraisal's test suite does.
@pboling
Copy link
Contributor Author
pboling commented Feb 21, 2025

@n-rodriguez @nickcharlton Getting the test suite working with Ruby 2.7 was a lot of work. The code had drifted away from compatibility with Ruby 2.7, since it wasn't in the test suite. I've only added Ruby 2.7 and 3.4 to the matrix here, and my next task will be to add each Ruby down the line in a discrete PR, while maintaining compatibility with the newer Rubies.

The latest spec failures are all related to whitespace on empty lines within block elements of the generated appraisals, and I'm working on a solution for that so the various Rubies don't have to match there exactly.

@pboling
Copy link
Contributor Author
pboling commented Feb 24, 2025

@n-rodriguez @nickcharlton I have independently verified that these changes result in compatibility with every MRI Ruby 1.9.3+, and JRuby 9.1+ in a gem I pinned to use a branch on my fork that integrates these changes with the eval_gemfile feature.

Ruby 1.8.7 can't be run any more on GHA, so we can't test it... I don't think that's a major issue. The RSpec project is in the same situation.

I was doing that so I could release a new version of that gem (rspec-pending_for) specifically to use in this gem's spec suite, to skip a spec on all engines except truffleruby, since it is passing here only on trufffleruby :/

Once I do that the whole CI will be green.

- Possibly due to bundler issue on MRI and JRuby engines
- rubygems/rubygems#8518
@pboling
Copy link
Contributor Author
pboling commented Feb 24, 2025

@n-rodriguez @nickcharlton This is green as a forest. :)

This was referenced Feb 25, 2025
@pboling
Copy link
Contributor Author
pboling commented Mar 19, 2025

@nickcharlton ping!

@nickcharlton
Copy link
Member

Ooh, fascinating. I'm only just starting to look at this.

My initial impression (when I saw the notification weeks ago…) was that going back this far was a bit silly, but I'm impressed by how you've been able to actually make it work.

Did you have any thoughts on running the really old versions of Ruby in the tests? Do Docker images still exist that could be used?

@thoughtbot thoughtbot deleted a comment from hound bot Mar 25, 2025
@thoughtbot thoughtbot deleted a comment from hound bot Mar 25, 2025
@thoughtbot thoughtbot deleted a comment from hound bot Mar 25, 2025
@thoughtbot thoughtbot deleted a comment from hound bot Mar 25, 2025
@thoughtbot thoughtbot deleted a comment from hound bot Mar 25, 2025
@thoughtbot thoughtbot deleted a comment from hound bot Mar 25, 2025
@nickcharlton
Copy link
Member

I'd like to split this PR into a few chunks:

  • Restoring MRI 1.8.7-2.2 support
    • this should include anything to get it to run, but not what should be in the next
  • Add Rubies 2.7, 2.6, 2.5, 2.4, 2.3, 2.2, 2.1, 2.0, 1.9, 1.8 to build matrix
    • if we can, it'd be good to do this in a few stages potentially, to keep the diff as small as possible
  • The main change in "Use .rspec to dry spec file headers" is really in the strip_heredoc, and that'd be best as it's own commit,
    • I'm less interested in the original change here, as I'm not sure it's that valuable, but I'm open to thoughts!
  • Probably multiple (?) PRs for different Ruby compatibilities, but I'm less sure how they fit together as I write this

I'd like to get to one commit (and also one PR) per logical change so it's easier to track back to what commit did what. Would you mind doing that? I know it's a lot of work, so I'll make sure we can merge these in close to when you open them. You might need to stack them on many branches, so potentially it's worth doing it over a couple of days so we don't make things incredibly confusing.

@pboling
Copy link
Contributor Author
pboling commented Mar 25, 2025

So, there are other projects that run tests against ancient Ruby, such as RSpec, which still tests against Ruby 1.8.7. They use a docker image for that.

GitHub actions can build back to Ruby 1.9.3, which means getting that far back is a mostly simple path. I didn't add it all here, because the PR was already so huge.

Regarding migrating smaller sets of Ruby compatibility, I have some thoughts.

  1. This is reverting a commit that removed the compat, so I'm not sure how I would break that apart.
  2. Much of the compatibility with old Ruby is automatic via the autocorrect and "style" settings in RuboCop that enforce compatibility with older Ruby. So I could step those changes one by one, but its busy work, just PRing the autocorrect for each version of rubocop-lts.
  3. I'm happy to do it, but I'll need more clarity on the path forward.

I'm going to restate, with some adjustments:

  1. PR to level up CI to test against all all current supported Ruby back to 2.7 (that is mostly what this PR is, from a LOC perspective)
  2. PR to restore Ruby 2.3 - 2.7 Support, extending CI testing back to 2.3
  3. PR to restore Ruby 1.8.7 - 2.2 Support, extending CI testing back to 1.9.3
  4. PR to test Ruby 1.8.7 in CI (this is a hard maybe).

I want to get the new CI in place structurally, so it can easily incorporate the older Rubies as I add them, so that's why the CI upgrade is the first PR in the list above.

Does that sound good @nickcharlton ? I may subdivide further if additional clean breaks present themselves, with a focus on small PRs.

I am making heavy use of my fork + branch that has all of this plus the eval_gemfile support on a lot of things that I have urgent need of (most recently my rewrite of the old masq gem as masq2), so it may take me a while to get to this. Perhaps not before RubyKaigi (my first ever Ruby conference!).

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.

3 participants
0