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.

Sign up for GitHub

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

Jump to bottom

Drop Ruby < 2.3 support #436

Merged
merged 39 commits into from Dec 15, 2020
Merged

Drop Ruby < 2.3 support #436

merged 39 commits into from Dec 15, 2020

Conversation

pirj
Copy link
Member

@pirj pirj commented Nov 14, 2020 •
edited

Better viewed ignoring indentation changes https://github.com/rspec/rspec-support/pull/436/files?diff=split&w=1

As per https://github.com/rspec/rspec/issues/61#issuecomment-687290045

Sibling PRs:

Minimal changes to rspec-core and rspec-mocks were required to run rspec-support's own spec suite, pushed to branches with the same name.

What has been done

Done:

  • clean up RubyFeatures
  • check respond_to?
  • check method_defined?
  • check send (where we call private methods that later became public)
  • cleanup Gemfile
  • update required_ruby_version
  • update VERSION
  • check RUBY_VERSION usages
  • check if we require_rspec_support ruby_features where RubyFeatures are not used
  • [-] check define_method where def would suffice
  • check (class << self; self; end) usages for possibility to use singleton_class (some can't be replaced as singleton_class does create a singleton class if it doesn't exist yet)
  • check if Java::JavaLang::String.instance_method(:char_at).arity == -1
  • [?] check Java::JavaLang::String.instance_method(:char_at).parameters == [] - still the case
  • check 1.8, 1.9, 2.0, 2.1, jruby, 9.0, 9.1, mri
  • check requires for support's mutex
  • check eval
  • check "work around", "workaround" - there are two cases: check if this was fixed in JRuby 9.2
  • check docs
  • [-] WithKeywordsWhenNeeded - will be addressed separately
  • gsub/unindent where squiggly heredoc can be used
  • [-] remove ripper_supported? - it has poor support in JRuby 9.1

@pirj pirj self-assigned this Nov 14, 2020
@pirj pirj changed the base branch from main to 4-0-dev November 14, 2020 18:22
@pirj pirj mentioned this pull request Nov 14, 2020
Closed
19 tasks
@pirj pirj changed the title Better viewed ignoring indentation changes https://github.com/rspec/rspec-support/pull/435/files?diff=split&w=1 Drop Ruby < 2.3 Nov 14, 2020
@pirj pirj marked this pull request as draft November 14, 2020 19:49
@pirj pirj force-pushed the drop-old-rubies branch 4 times, most recently from 062aebe to fb70e0b Compare November 14, 2020 20:53
@pirj
Copy link
Member Author

pirj commented Nov 14, 2020

For some reason, builds stopped triggering.
Going to switch to "ready for review" temporarily just to figure out if it's the case (quite unlikely though).

@pirj pirj marked this pull request as ready for review November 14, 2020 20:54
@pirj pirj marked this pull request as draft November 15, 2020 01:34
@pirj pirj force-pushed the drop-old-rubies branch 3 times, most recently from 7b6618b to f0a1592 Compare November 15, 2020 15:11
@benoittgt
Copy link
Member

benoittgt commented Nov 15, 2020 •
edited

For the build issue it is because of travisci config. 1129730

JonRowe
JonRowe reviewed Nov 15, 2020
View reviewed changes
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

Theres a whole bunch of unnecessary changes, I don't feel we need to switch to 1.9 style keys just because we can, similarly with the trailing dot changes.

I also think we should keep the mutex method binding, the reasoning behind it is sound, we just don't need the vendored implementation.

lib/rspec/support/reentrant_mutex.rb Show resolved Hide resolved
lib/rspec/support/source/token.rb Outdated Show resolved Hide resolved
lib/rspec/support/spec/library_wide_checks.rb Outdated Show resolved Hide resolved
lib/rspec/support/spec/library_wide_checks.rb Outdated Show resolved Hide resolved
lib/rspec/support/spec/shell_out.rb Show resolved Hide resolved
lib/rspec/support/with_keywords_when_needed.rb Show resolved Hide resolved
rspec-support.gemspec Show resolved Hide resolved
script/functions.sh Outdated Show resolved Hide resolved
spec/rspec/support/mutex_spec.rb Show resolved Hide resolved
.rubocop_rspec_base.yml Outdated Show resolved Hide resolved
@JonRowe JonRowe mentioned this pull request Nov 15, 2020
Merged
19 tasks
@pirj
Copy link
Member Author

pirj commented Nov 15, 2020

@JonRowe Thanks for the review.

I've outlined the things to discuss in the description, including:

[?] extract rubocop changes into a separate pull request

I'll extract this into a separate follow-up pull request to 4-0-dev.

@pirj
Copy link
Member Author

pirj commented Nov 15, 2020

@JonRowe @benoittgt Appreciate if you can take a look at Discuss section of the description, there are some important questions, let me cite below:

  1. introduce https://github.com/ruby/ruby2_keywords dependency and remove respond_to?(:ruby2_keywords, true) check
  2. cleanup of build functions/predicates - any objections? (for sure in a separately pull request, and coming from rspec-dev)
  3. drop Ruby 2.3 - problematic'ish due to a dependency on outdated OpenSSL, EOL'ed, not supported by RuboCop, still used by 8% of users
  4. tweak clone_repo to accept a branch, use maintenance-branch's branch for main repos, and 4-0-maintenance for cloning rspec-rails since it's alienated anyway?

@JonRowe
Copy link
Member

JonRowe commented Nov 15, 2020

[?] introduce https://github.com/ruby/ruby2_keywords dependency and remove respond_to?(:ruby2_keywords, true) check

No, we can't introduce dependencies.

[?] drop Ruby 2.3 - problematic'ish due to a dependency on outdated OpenSSL, EOL'ed, not supported by RuboCop, still used by 8% of users

What Rubocop supports does not drive us. 2.3 is not really problematic.

[?] tweak clone_repo to accept a branch, use maintenance-branch's branch for main repos, and 4-0-maintenance for cloning rspec-rails since it's alienated anyway?

It already does this? Its just hardcoded based on the branch?

[?] extract rubocop changes into a separate pull request

Just remove them, they're unneeded.

@pirj

This comment has been minimized.

Sign in to view
@pirj
Copy link
Member Author

pirj commented Nov 18, 2020 •
edited

I'm getting stuck with removing WithKeywordsWhenNeeded and dealing with kwargs delegation related failures in combination with dropping support for old rubies.
Removal of WithKeywordsWhenNeeded is tangential to the removal of rubies, and I'm going to remove all eval hacks, but not kwargs delegation hacks.
I intend to address the removal of WithKeywordsWhenNeeded and kwargs delegation issues and hacks in a separate series of pull requests.

@pirj pirj force-pushed the drop-old-rubies branch 2 times, most recently from b16deab to 7f70218 Compare November 18, 2020 08:11
@pirj
Copy link
Member Author

pirj commented Dec 14, 2020

Update: combined ReentrantMutex with Mutex.

JonRowe
JonRowe reviewed Dec 14, 2020
View reviewed changes
Changelog.md Outdated Show resolved Hide resolved
lib/rspec/support/recursive_const_methods.rb Show resolved Hide resolved
lib/rspec/support/recursive_const_methods.rb Show resolved Hide resolved
@pirj
Remove const_defined? workaround
69288f3
@pirj
Remove version-specific details from a doc
1b2467f
@pirj
Remove old JRuby workaround
b66b7c8
@pirj
Remove usages of jruby_9000
571a692
@pirj
Remove non_mri?
0c3c74e
@pirj
Add Changelog entry
a92dc58
@pirj
Get rid of evals
b69244f
@pirj
Fix offences
fdd801c
@pirj
Use Mutex from core, prevent it from being stubbed
d181a28
@pirj
Remove StdErrSplitter workarounds
dd56e6f 
Kept gems warning muted due to:

1.

      RuntimeError:
       Warnings were generated: /home/runner/work/rspec-support/bundle/ruby/2.5.0/gems/actioncable-6.0.3.4/lib/action_cable/channel/test_case.rb:178: warning: method redefined; discarding old connection
     Shared Example Group: "an rspec-rails example group mixin" called from ./spec/rspec/rails/example/channel_example_group_spec.rb:6
     # /home/runner/work/rspec-support/rspec-support/lib/rspec/support/spec/stderr_splitter.rb:38:in `verify_no_warnings!'

2.
       RuntimeError:
        Warnings were generated: /home/runner/work/rspec-support/bundle/ruby/2.5.0/gems/actionpack-6.0.3.4/lib/action_dispatch/routing/route_set.rb:560: warning: instance variable @_routes not initialized
      Shared Example Group: "an rspec-rails example group mixin" called from ./spec/rspec/rails/example/view_example_group_spec.rb:5
      # /home/runner/work/rspec-support/rspec-support/lib/rspec/support/spec/stderr_splitter.rb:38:in `verify_no_warnings!'
@pirj
Remove redundant boilerplate
38885cf
@pirj
Remove BasicObject checks
a9ab522
@pirj
Remove filtering for unsupported rubies
048eedd
@pirj
Fix code doc
a787fa2
@pirj
Downgrade ffi due to their usage of Gem
0d41dfa
JonRowe
JonRowe requested changes Dec 15, 2020
View reviewed changes
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

Almost there, just the branch pinning to drop and see if it builds 😂

@pirj pirj requested a review from JonRowe December 15, 2020 08:48
@pirj
Copy link
Member Author

pirj commented Dec 15, 2020

Update: reset the temporary maintenance-branch commit.

Green! 💚

@JonRowe JonRowe merged commit 1a26b09 into 4-0-dev Dec 15, 2020
@JonRowe JonRowe deleted the drop-old-rubies branch December 15, 2020 09:30
@pirj
Copy link
Member Author

pirj commented Dec 15, 2020

🎉

@pirj pirj mentioned this pull request Dec 18, 2020
Open
53 tasks
@pirj pirj added the RSpec 4 label Dec 25, 2020
@pirj pirj added this to the 4.0 milestone Dec 25, 2020
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
@JonRowe
[support] Merge pull request rspec/rspec-support#436 from rspec/drop-…
0e2f958 
…old-rubies

Drop Ruby < 2.3 support

---
This commit was imported from rspec/rspec-support@1a26b09.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoittgt benoittgt Awaiting requested review from benoittgt

@JonRowe JonRowe Awaiting requested review from JonRowe

Assignees

@pirj pirj

Labels
RSpec 4
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pirj @benoittgt @JonRowe