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

Remove Gemfile, add script for Rubocop #1204

Merged
merged 3 commits into from
Oct 14, 2024
Merged

Remove Gemfile, add script for Rubocop #1204

merged 3 commits into from
Oct 14, 2024

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Oct 13, 2024

This addresses the issues from #1180.
I've opted for using bundler/inline, so we can still force a specific version. The script itself is fully compatible with Rubocop, so we can things like bin/rubocop --autocorrect work too.
I've also updated the CI job to Ruby 3.3, it was still using 3.0. It should probably work with every supported Ruby version.

bin/rubocop Outdated
gem 'rubocop', '= 1.66.1'
end

# Copied from `exe/rubocop` from the Rubocop gem
Copy link
Member

Choose a reason for hiding this comment

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

I think you should be able to use require Gem.bin_path('rubocop', 'rubocop') instead to avoid having to copy the logic from exe/rubocop.

Copy link
Member Author

Choose a reason for hiding this comment

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

This did result in a LoadError, even though the file does exist (probably because it expects a .rb extension). Using Kernel.exec works.

Copy link
Member

Choose a reason for hiding this comment

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

Right, require is picky about the extension.
load would likely work then.

I think exec might lose the state from bundler above and potentially run a different version of RuboCop, so could you try load?

Alternatively we could exec but pass the version too like:
exec(Gem.bin_path('rubocop', 'rubocop'), '_1.66.1_', *ARGV)
but that probably won't work if bundle path config is set (which I have).

Copy link
Member Author

Choose a reason for hiding this comment

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

The bin_path includes the full version in the path (so in my case it's something like ~/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rubocop-1.66.1/exe/rubocop). I've tried it with switching to a different version and running bin/rubocop -v, and this shows the correct version.

Copy link
Member

Choose a reason for hiding this comment

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

You're right it seems to be correct:

$ cat /home/eregon/.rubies/ruby-3.2.2/lib/ruby/gems/3.2.0/gems/rubocop-1.56.3/exe/rubocop 
#!/usr/bin/env ruby
# frozen_string_literal: true

$LOAD_PATH.unshift("#{__dir__}/../lib")
...

because of that $LOAD_PATH modification.
Gems are not supposed to do that but whatever, it helps here.

The ultra correct way would be to replicate what bin/rubocop does:

cat ~/.rubies/ruby-3.2.2/bin/rubocop 
#!/home/eregon/.rubies/ruby-3.2.2/bin/ruby
#
# This file was generated by RubyGems.
#
# The application 'rubocop' is installed as part of a gem, and
# this file is here to facilitate running it.
#

require 'rubygems'

Gem.use_gemdeps

version = ">= 0.a"

str = ARGV.first
if str
  str = str.b[/\A_(.*)_\z/, 1]
  if str and Gem::Version.correct?(str)
    version = str
    ARGV.shift
  end
end

if Gem.respond_to?(:activate_bin_path)
load Gem.activate_bin_path('rubocop', 'rubocop', version)
else
gem "rubocop", version
load Gem.bin_path("rubocop", "rubocop", version)
end

Which explicitly activates the gem with the optionally-given version (and otherwise picks latest).

Anyway, the exec seems fine for now, we can always improve this later if needed.

bin/rubocop Outdated Show resolved Hide resolved
@eregon eregon merged commit 0bd43d8 into ruby:master Oct 14, 2024
14 checks passed
@herwinw herwinw deleted the rubocop branch October 14, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants