diff --git a/.github/workflows/annocheck.yml b/.github/workflows/annocheck.yml index 8286cb41edb3de..ac1b338af5f713 100644 --- a/.github/workflows/annocheck.yml +++ b/.github/workflows/annocheck.yml @@ -73,7 +73,7 @@ jobs: builddir: build makeup: true - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/auto_review_pr.yml b/.github/workflows/auto_review_pr.yml index 88e7ba49cec31e..e92ad563dd62a8 100644 --- a/.github/workflows/auto_review_pr.yml +++ b/.github/workflows/auto_review_pr.yml @@ -3,6 +3,12 @@ on: pull_request_target: types: [opened, ready_for_review, reopened] branches: [master] + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to review' + required: true + type: number permissions: contents: read @@ -11,7 +17,7 @@ jobs: auto-review-pr: name: Auto Review PR runs-on: ubuntu-latest - if: ${{ github.repository == 'ruby/ruby' && github.base_ref == 'master' }} + if: ${{ github.repository == 'ruby/ruby' && (github.base_ref == 'master' || github.event_name == 'workflow_dispatch') }} permissions: pull-requests: write @@ -23,7 +29,7 @@ jobs: with: persist-credentials: false - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.4' bundler: none @@ -32,4 +38,4 @@ jobs: run: ruby tool/auto_review_pr.rb "$GITHUB_PR_NUMBER" env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }} + GITHUB_PR_NUMBER: ${{ github.event.pull_request.number || github.event.inputs.pr_number }} diff --git a/.github/workflows/baseruby.yml b/.github/workflows/baseruby.yml index d2c9e9a204cc99..2002a49414c920 100644 --- a/.github/workflows/baseruby.yml +++ b/.github/workflows/baseruby.yml @@ -48,7 +48,7 @@ jobs: - ruby-3.3 steps: - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: ${{ matrix.ruby }} bundler: none diff --git a/.github/workflows/bundled_gems.yml b/.github/workflows/bundled_gems.yml index 75719580246e4c..c7231c9d0e1e33 100644 --- a/.github/workflows/bundled_gems.yml +++ b/.github/workflows/bundled_gems.yml @@ -38,7 +38,7 @@ jobs: with: token: ${{ (github.repository == 'ruby/ruby' && !startsWith(github.event_name, 'pull')) && secrets.MATZBOT_AUTO_UPDATE_TOKEN || secrets.GITHUB_TOKEN }} - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: 4.0 diff --git a/.github/workflows/check_dependencies.yml b/.github/workflows/check_dependencies.yml index 820d4f2aadcefe..0f1f6d9f97f50a 100644 --- a/.github/workflows/check_dependencies.yml +++ b/.github/workflows/check_dependencies.yml @@ -42,7 +42,7 @@ jobs: - uses: ./.github/actions/setup/directories - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/check_misc.yml b/.github/workflows/check_misc.yml index 518b246f72f453..863655cd0682e0 100644 --- a/.github/workflows/check_misc.yml +++ b/.github/workflows/check_misc.yml @@ -23,7 +23,7 @@ jobs: token: ${{ (github.repository == 'ruby/ruby' && !startsWith(github.event_name, 'pull')) && secrets.MATZBOT_AUTO_UPDATE_TOKEN || secrets.GITHUB_TOKEN }} persist-credentials: false - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: head diff --git a/.github/workflows/modgc.yml b/.github/workflows/modgc.yml index ad7928e050174a..f96648c06b70ed 100644 --- a/.github/workflows/modgc.yml +++ b/.github/workflows/modgc.yml @@ -62,7 +62,7 @@ jobs: uses: ./.github/actions/setup/ubuntu if: ${{ contains(matrix.os, 'ubuntu') }} - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/parse_y.yml b/.github/workflows/parse_y.yml index 318e5699b45de4..ca9b7e0cb40e40 100644 --- a/.github/workflows/parse_y.yml +++ b/.github/workflows/parse_y.yml @@ -59,7 +59,7 @@ jobs: - uses: ./.github/actions/setup/ubuntu - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 5489f95af60248..83ae60f248ca56 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -19,7 +19,7 @@ jobs: with: persist-credentials: false - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: 3.3.4 diff --git a/.github/workflows/spec_guards.yml b/.github/workflows/spec_guards.yml index 54a657d290aaa5..6301f02bf2a2a4 100644 --- a/.github/workflows/spec_guards.yml +++ b/.github/workflows/spec_guards.yml @@ -49,7 +49,7 @@ jobs: with: persist-credentials: false - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: ${{ matrix.ruby }} bundler: none diff --git a/.github/workflows/sync_default_gems.yml b/.github/workflows/sync_default_gems.yml index a16595f5e88694..72203e10f57c32 100644 --- a/.github/workflows/sync_default_gems.yml +++ b/.github/workflows/sync_default_gems.yml @@ -36,7 +36,7 @@ jobs: with: token: ${{ github.repository == 'ruby/ruby' && secrets.MATZBOT_AUTO_UPDATE_TOKEN || secrets.GITHUB_TOKEN }} - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.4' bundler: none diff --git a/.github/workflows/ubuntu.yml b/.github/workflows/ubuntu.yml index 9525e1800ce41f..76035a73963c13 100644 --- a/.github/workflows/ubuntu.yml +++ b/.github/workflows/ubuntu.yml @@ -70,7 +70,7 @@ jobs: with: arch: ${{ matrix.arch }} - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/wasm.yml b/.github/workflows/wasm.yml index dffb30c6ce652a..616bacc32021bd 100644 --- a/.github/workflows/wasm.yml +++ b/.github/workflows/wasm.yml @@ -99,7 +99,7 @@ jobs: run: | echo "WASI_SDK_PATH=/opt/wasi-sdk" >> $GITHUB_ENV - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 6a49d60178ea84..8ece56f86210b6 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -59,7 +59,7 @@ jobs: - run: md build working-directory: - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: # windows-11-arm has only 3.4.1, 3.4.2, 3.4.3, head ruby-version: ${{ !endsWith(matrix.os, 'arm') && '3.1' || '3.4' }} diff --git a/.github/workflows/yjit-ubuntu.yml b/.github/workflows/yjit-ubuntu.yml index a69e48624c6ad2..5df617998ac732 100644 --- a/.github/workflows/yjit-ubuntu.yml +++ b/.github/workflows/yjit-ubuntu.yml @@ -133,7 +133,7 @@ jobs: - uses: ./.github/actions/setup/ubuntu - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none diff --git a/.github/workflows/zjit-macos.yml b/.github/workflows/zjit-macos.yml index e91c2e48f2a05d..e084855d3d6ef7 100644 --- a/.github/workflows/zjit-macos.yml +++ b/.github/workflows/zjit-macos.yml @@ -92,7 +92,7 @@ jobs: rustup install ${{ matrix.rust_version }} --profile minimal rustup default ${{ matrix.rust_version }} - - uses: taiki-e/install-action@de6bbd1333b8f331563d54a051e542c7dfef81c3 # v2.68.34 + - uses: taiki-e/install-action@94a7388bec5d4c8dd93e3ebf09e0ff448f3f6f4d # v2.68.35 with: tool: nextest@0.9 if: ${{ matrix.test_task == 'zjit-check' }} diff --git a/.github/workflows/zjit-ubuntu.yml b/.github/workflows/zjit-ubuntu.yml index b51be529900246..67b16a2ee82d31 100644 --- a/.github/workflows/zjit-ubuntu.yml +++ b/.github/workflows/zjit-ubuntu.yml @@ -114,12 +114,12 @@ jobs: - uses: ./.github/actions/setup/ubuntu - - uses: ruby/setup-ruby@c984c1a20bb35a1cbda04477c816cea024418be9 # v1.294.0 + - uses: ruby/setup-ruby@319994f95fa847cf3fb3cd3dbe89f6dcde9f178f # v1.295.0 with: ruby-version: '3.1' bundler: none - - uses: taiki-e/install-action@de6bbd1333b8f331563d54a051e542c7dfef81c3 # v2.68.34 + - uses: taiki-e/install-action@94a7388bec5d4c8dd93e3ebf09e0ff448f3f6f4d # v2.68.35 with: tool: nextest@0.9 if: ${{ matrix.test_task == 'zjit-check' }} diff --git a/lib/rubygems/commands/sources_command.rb b/lib/rubygems/commands/sources_command.rb index 7e5c2a2465e6f3..b399af2bd3d5a0 100644 --- a/lib/rubygems/commands/sources_command.rb +++ b/lib/rubygems/commands/sources_command.rb @@ -50,11 +50,8 @@ def initialize end def add_source(source_uri) # :nodoc: - check_rubygems_https source_uri - - source = Gem::Source.new source_uri - - check_typo_squatting(source) + source = build_new_source(source_uri) + source_uri = source.uri.to_s begin if Gem.sources.include? source @@ -76,11 +73,8 @@ def add_source(source_uri) # :nodoc: end def append_source(source_uri) # :nodoc: - check_rubygems_https source_uri - - source = Gem::Source.new source_uri - - check_typo_squatting(source) + source = build_new_source(source_uri) + source_uri = source.uri.to_s begin source.load_specs :released @@ -103,11 +97,8 @@ def append_source(source_uri) # :nodoc: end def prepend_source(source_uri) # :nodoc: - check_rubygems_https source_uri - - source = Gem::Source.new source_uri - - check_typo_squatting(source) + source = build_new_source(source_uri) + source_uri = source.uri.to_s begin source.load_specs :released @@ -141,6 +132,19 @@ def check_typo_squatting(source) end end + def normalize_source_uri(source_uri) # :nodoc: + # Ensure the source URI has a trailing slash for proper RFC 2396 path merging + # Without a trailing slash, the last path segment is treated as a file and removed + # during relative path resolution (e.g., "/blish" + "gems/foo.gem" = "/gems/foo.gem") + # With a trailing slash, it's treated as a directory (e.g., "/blish/" + "gems/foo.gem" = "/blish/gems/foo.gem") + uri = Gem::URI.parse(source_uri) + uri.path = uri.path.gsub(%r{/+\z}, "") + "/" if uri.path && !uri.path.empty? + uri.to_s + rescue Gem::URI::Error + # If parsing fails, return the original URI and let later validation handle it + source_uri + end + def check_rubygems_https(source_uri) # :nodoc: uri = Gem::URI source_uri @@ -273,7 +277,8 @@ def execute end def remove_source(source_uri) # :nodoc: - source = Gem::Source.new source_uri + source = build_source(source_uri) + source_uri = source.uri.to_s if configured_sources&.include? source Gem.sources.delete source @@ -328,4 +333,16 @@ def configured_sources def config_file_name Gem.configuration.config_file_name end + + def build_source(source_uri) + source_uri = normalize_source_uri(source_uri) + Gem::Source.new(source_uri) + end + + def build_new_source(source_uri) + source = build_source(source_uri) + check_rubygems_https(source.uri.to_s) + check_typo_squatting(source) + source + end end diff --git a/spec/bundler/spec_helper.rb b/spec/bundler/spec_helper.rb index 0ea191751dd042..55953f09d42ff4 100644 --- a/spec/bundler/spec_helper.rb +++ b/spec/bundler/spec_helper.rb @@ -131,6 +131,14 @@ def self.ruby=(ruby) ENV["XDG_CACHE_HOME"] = nil ENV["GEMRC"] = nil + # Prevent tests from modifying the user's global git config. + # GIT_CONFIG_GLOBAL and GIT_CONFIG_NOSYSTEM are available since Git 2.32. + git_version = `git --version`[/(\d+\.\d+\.\d+)/, 1] + if Gem::Version.new(git_version) >= Gem::Version.new("2.32") + ENV["GIT_CONFIG_GLOBAL"] = File.join(ENV["HOME"], ".gitconfig") + ENV["GIT_CONFIG_NOSYSTEM"] = "1" + end + # Don't wrap output in tests ENV["THOR_COLUMNS"] = "10000" diff --git a/spec/bundler/support/filters.rb b/spec/bundler/support/filters.rb index c6b60b5d52fe3a..6c9127cd950eff 100644 --- a/spec/bundler/support/filters.rb +++ b/spec/bundler/support/filters.rb @@ -18,10 +18,13 @@ def inspect end end +git_version = Gem::Version.new(`git --version`[/(\d+\.\d+\.\d+)/, 1]) + RSpec.configure do |config| config.filter_run_excluding realworld: true config.filter_run_excluding rubygems: RequirementChecker.against(Gem.rubygems_version) + config.filter_run_excluding git: RequirementChecker.against(git_version) config.filter_run_excluding ruby_repo: !ENV["GEM_COMMAND"].nil? config.filter_run_excluding no_color_tty: Gem.win_platform? || !ENV["GITHUB_ACTION"].nil? config.filter_run_excluding permissions: Gem.win_platform? diff --git a/test/ruby/test_thread_cv.rb b/test/ruby/test_thread_cv.rb index 81201f134f0c71..e5fd513c5c3ffa 100644 --- a/test/ruby/test_thread_cv.rb +++ b/test/ruby/test_thread_cv.rb @@ -70,7 +70,7 @@ def test_condvar_wait_and_broadcast end end end - sleep 0.1 + Thread.pass until threads.all?(&:stop?) mutex.synchronize do result << "P1" condvar.broadcast diff --git a/test/rubygems/test_gem_commands_sources_command.rb b/test/rubygems/test_gem_commands_sources_command.rb index 00eb9239940e81..71c6d5ce166828 100644 --- a/test/rubygems/test_gem_commands_sources_command.rb +++ b/test/rubygems/test_gem_commands_sources_command.rb @@ -60,6 +60,82 @@ def test_execute_add assert_equal "", @ui.error end + def test_execute_add_without_trailing_slash + setup_fake_source("https://rubygems.pkg.github.com/my-org") + + @cmd.handle_options %W[--add https://rubygems.pkg.github.com/my-org] + + use_ui @ui do + @cmd.execute + end + + assert_equal [@gem_repo, "https://rubygems.pkg.github.com/my-org/"], Gem.sources + + expected = <<-EOF +https://rubygems.pkg.github.com/my-org/ added to sources + EOF + + assert_equal expected, @ui.output + assert_equal "", @ui.error + end + + def test_execute_add_multiple_trailing_slash + setup_fake_source("https://rubygems.pkg.github.com/my-org/") + + @cmd.handle_options %W[--add https://rubygems.pkg.github.com/my-org///] + + use_ui @ui do + @cmd.execute + end + + assert_equal [@gem_repo, "https://rubygems.pkg.github.com/my-org/"], Gem.sources + + expected = <<-EOF +https://rubygems.pkg.github.com/my-org/ added to sources + EOF + + assert_equal expected, @ui.output + assert_equal "", @ui.error + end + + def test_execute_append_without_trailing_slash + setup_fake_source("https://rubygems.pkg.github.com/my-org") + + @cmd.handle_options %W[--append https://rubygems.pkg.github.com/my-org] + + use_ui @ui do + @cmd.execute + end + + assert_equal [@gem_repo, "https://rubygems.pkg.github.com/my-org/"], Gem.sources + + expected = <<-EOF +https://rubygems.pkg.github.com/my-org/ added to sources + EOF + + assert_equal expected, @ui.output + assert_equal "", @ui.error + end + + def test_execute_prepend_without_trailing_slash + setup_fake_source("https://rubygems.pkg.github.com/my-org") + + @cmd.handle_options %W[--prepend https://rubygems.pkg.github.com/my-org] + + use_ui @ui do + @cmd.execute + end + + assert_equal ["https://rubygems.pkg.github.com/my-org/", @gem_repo], Gem.sources + + expected = <<-EOF +https://rubygems.pkg.github.com/my-org/ added to sources + EOF + + assert_equal expected, @ui.output + assert_equal "", @ui.error + end + def test_execute_append setup_fake_source(@new_repo) @@ -530,17 +606,14 @@ def test_execute_add_https_rubygems_org @cmd.handle_options %W[--add #{https_rubygems_org}] - ui = Gem::MockGemUi.new "n" - - use_ui ui do - assert_raise Gem::MockGemUi::TermError do - @cmd.execute - end + use_ui @ui do + @cmd.execute end - assert_equal [@gem_repo], Gem.sources + assert_equal [@gem_repo, https_rubygems_org], Gem.sources expected = <<-EXPECTED +#{https_rubygems_org} added to sources EXPECTED assert_equal expected, @ui.output @@ -554,17 +627,14 @@ def test_execute_append_https_rubygems_org @cmd.handle_options %W[--append #{https_rubygems_org}] - ui = Gem::MockGemUi.new "n" - - use_ui ui do - assert_raise Gem::MockGemUi::TermError do - @cmd.execute - end + use_ui @ui do + @cmd.execute end - assert_equal [@gem_repo], Gem.sources + assert_equal [@gem_repo, https_rubygems_org], Gem.sources expected = <<-EXPECTED +#{https_rubygems_org} added to sources EXPECTED assert_equal expected, @ui.output @@ -583,7 +653,7 @@ def test_execute_add_bad_uri assert_equal [@gem_repo], Gem.sources expected = <<-EOF -beta-gems.example.com is not a URI +beta-gems.example.com/ is not a URI EOF assert_equal expected, @ui.output @@ -602,7 +672,26 @@ def test_execute_append_bad_uri assert_equal [@gem_repo], Gem.sources expected = <<-EOF -beta-gems.example.com is not a URI +beta-gems.example.com/ is not a URI + EOF + + assert_equal expected, @ui.output + assert_equal "", @ui.error + end + + def test_execute_prepend_bad_uri + @cmd.handle_options %w[--prepend beta-gems.example.com] + + use_ui @ui do + assert_raise Gem::MockGemUi::TermError do + @cmd.execute + end + end + + assert_equal [@gem_repo], Gem.sources + + expected = <<-EOF +beta-gems.example.com/ is not a URI EOF assert_equal expected, @ui.output @@ -778,6 +867,31 @@ def test_execute_remove_redundant_source_trailing_slash Gem.configuration.sources = nil end + def test_execute_remove_without_trailing_slash + source_uri = "https://rubygems.pkg.github.com/my-org/" + + Gem.configuration.sources = [source_uri] + + setup_fake_source(source_uri) + + @cmd.handle_options %W[--remove https://rubygems.pkg.github.com/my-org] + + use_ui @ui do + @cmd.execute + end + + assert_equal [], Gem.sources + + expected = <<-EOF +#{source_uri} removed from sources + EOF + + assert_equal expected, @ui.output + assert_equal "", @ui.error + ensure + Gem.configuration.sources = nil + end + def test_execute_update @cmd.handle_options %w[--update] @@ -888,6 +1002,6 @@ def setup_fake_source(uri) Marshal.dump specs, io end - @fetcher.data["#{uri}/specs.#{@marshal_version}.gz"] = specs_dump_gz.string + @fetcher.data["#{uri.chomp("/")}/specs.#{@marshal_version}.gz"] = specs_dump_gz.string end end diff --git a/tool/auto_review_pr.rb b/tool/auto_review_pr.rb index 07c98c7e0aed9e..a8f64fab9d1f79 100755 --- a/tool/auto_review_pr.rb +++ b/tool/auto_review_pr.rb @@ -34,46 +34,95 @@ class AutoReviewPR REPO = 'ruby/ruby' COMMENT_USER = 'github-actions[bot]' - COMMENT_PREFIX = 'The following files are maintained in the following upstream repositories:' - COMMENT_SUFFIX = 'Please file a pull request to the above instead. Thank you!' + + UPSTREAM_COMMENT_PREFIX = 'The following files are maintained in the following upstream repositories:' + UPSTREAM_COMMENT_SUFFIX = 'Please file a pull request to the above instead. Thank you!' + + FORK_COMMENT_PREFIX = 'It looks like this pull request was filed from a branch in ruby/ruby.' + FORK_COMMENT_BODY = <<~COMMENT + #{FORK_COMMENT_PREFIX} + + Since ruby/ruby is bi-directionally mirrored with the official git repository at git.ruby-lang.org, \ + having topic branches in ruby/ruby makes it harder to manage the mirror. + + Could you please close this pull request and re-file it from a branch in your personal fork instead? \ + You can fork https://github.com/ruby/ruby, push your branch there, and open a new pull request from it. + + Thank you for your contribution! + COMMENT def initialize(client) @client = client end def review(pr_number) - # Fetch the list of files changed by the PR - changed_files = @client.get("/repos/#{REPO}/pulls/#{pr_number}/files").map { it.fetch(:filename) } + existing_comments = fetch_existing_comments(pr_number) + review_non_fork_branch(pr_number, existing_comments) + review_upstream_repos(pr_number, existing_comments) + end - # Build a Hash: { upstream_repo => files, ... } - upstream_repos = SyncDefaultGems::Repository.group(changed_files) - upstream_repos.delete(nil) # exclude no-upstream files - upstream_repos.delete('prism') if changed_files.include?('prism_compile.c') # allow prism changes in this case - if upstream_repos.empty? - puts "Skipped: The PR ##{pr_number} doesn't have upstream repositories." + private + + def fetch_existing_comments(pr_number) + comments = @client.get("/repos/#{REPO}/issues/#{pr_number}/comments") + comments.map { [it.fetch(:user).fetch(:login), it.fetch(:body)] } + end + + def already_commented?(existing_comments, prefix) + existing_comments.any? { |user, comment| user == COMMENT_USER && comment.start_with?(prefix) } + end + + def post_comment(pr_number, comment) + result = @client.post("/repos/#{REPO}/issues/#{pr_number}/comments", { body: comment }) + puts "Success: #{JSON.pretty_generate(result)}" + end + + # Suggest re-filing from a fork if the PR branch is in ruby/ruby itself + def review_non_fork_branch(pr_number, existing_comments) + if already_commented?(existing_comments, FORK_COMMENT_PREFIX) + puts "Skipped: The PR ##{pr_number} already has a fork branch comment." return end - # Check if the PR is already reviewed - existing_comments = @client.get("/repos/#{REPO}/issues/#{pr_number}/comments") - existing_comments.map! { [it.fetch(:user).fetch(:login), it.fetch(:body)] } - if existing_comments.any? { |user, comment| user == COMMENT_USER && comment.start_with?(COMMENT_PREFIX) } - puts "Skipped: The PR ##{pr_number} already has an automated review comment." + pr = @client.get("/repos/#{REPO}/pulls/#{pr_number}") + head_repo = pr.dig(:head, :repo, :full_name) + if head_repo != REPO + puts "Skipped: The PR ##{pr_number} is already from a fork (#{head_repo})." return end - # Post a comment - comment = format_comment(upstream_repos) - result = @client.post("/repos/#{REPO}/issues/#{pr_number}/comments", { body: comment }) - puts "Success: #{JSON.pretty_generate(result)}" + author = pr.dig(:user, :login) + if author == 'dependabot[bot]' + puts "Skipped: The PR ##{pr_number} is from dependabot." + return + end + + post_comment(pr_number, FORK_COMMENT_BODY) end - private + # Suggest filing PRs to upstream repositories for files that have one + def review_upstream_repos(pr_number, existing_comments) + if already_commented?(existing_comments, UPSTREAM_COMMENT_PREFIX) + puts "Skipped: The PR ##{pr_number} already has an upstream repos comment." + return + end + + changed_files = @client.get("/repos/#{REPO}/pulls/#{pr_number}/files").map { it.fetch(:filename) } + + upstream_repos = SyncDefaultGems::Repository.group(changed_files) + upstream_repos.delete(nil) + upstream_repos.delete('prism') if changed_files.include?('prism_compile.c') + if upstream_repos.empty? + puts "Skipped: The PR ##{pr_number} doesn't have upstream repositories." + return + end + + post_comment(pr_number, format_upstream_comment(upstream_repos)) + end - # upstream_repos: { upstream_repo => files, ... } - def format_comment(upstream_repos) + def format_upstream_comment(upstream_repos) comment = +'' - comment << "#{COMMENT_PREFIX}\n\n" + comment << "#{UPSTREAM_COMMENT_PREFIX}\n\n" upstream_repos.each do |upstream_repo, files| comment << "* https://github.com/ruby/#{upstream_repo}\n" @@ -82,7 +131,7 @@ def format_comment(upstream_repos) end end - comment << "\n#{COMMENT_SUFFIX}" + comment << "\n#{UPSTREAM_COMMENT_SUFFIX}" comment end end diff --git a/zjit/src/backend/x86_64/mod.rs b/zjit/src/backend/x86_64/mod.rs index e2d0185bb8d77a..476670ee4922ff 100644 --- a/zjit/src/backend/x86_64/mod.rs +++ b/zjit/src/backend/x86_64/mod.rs @@ -378,8 +378,8 @@ impl Assembler { } /// If a given operand is Opnd::Mem and it uses MemBase::Stack, lower it to MemBase::Reg(NATIVE_BASE_PTR). - /// For MemBase::StackIndirect, load the pointer from the stack slot into a scratch register. - fn split_stack_membase(asm: &mut Assembler, opnd: Opnd, scratch_opnd: Opnd, stack_state: &StackState) -> Opnd { + /// In general, `out` operand is a `VReg`, so it may use MemBase::Stack, but not MemBase::StackIndirect. + fn lower_stack_membase(opnd: Opnd, stack_state: &StackState) -> Opnd { match opnd { Opnd::Mem(Mem { base: stack_membase @ MemBase::Stack { .. }, disp: opnd_disp, num_bits: opnd_num_bits }) => { // Convert MemBase::Stack to MemBase::Reg(NATIVE_BASE_PTR) with the @@ -389,6 +389,18 @@ impl Assembler { let Mem { base, disp: stack_disp, .. } = stack_state.stack_membase_to_mem(stack_membase); Opnd::Mem(Mem { base, disp: stack_disp + opnd_disp, num_bits: opnd_num_bits }) } + Opnd::Mem(Mem { base: MemBase::StackIndirect { .. }, .. }) => { + unreachable!("lower_stack_membase expects {opnd:?} to not have MemBase::StackIndirect") + } + _ => opnd, + } + } + + /// If a given operand is Opnd::Mem and it uses MemBase::Stack, lower it to MemBase::Reg(NATIVE_BASE_PTR). + /// For MemBase::StackIndirect, load the pointer from the stack slot into a scratch register. + fn split_stack_membase(asm: &mut Assembler, opnd: Opnd, scratch_opnd: Opnd, stack_state: &StackState) -> Opnd { + match opnd { + Opnd::Mem(Mem { base: MemBase::Stack { .. }, .. }) => lower_stack_membase(opnd, stack_state), Opnd::Mem(Mem { base: MemBase::StackIndirect { stack_idx }, disp: opnd_disp, num_bits: opnd_num_bits }) => { // The spilled value (a pointer) lives at a stack slot. Load it // into a scratch register, then use the register as the base. @@ -473,7 +485,7 @@ impl Assembler { *left = split_if_both_memory(asm, *left, *right, SCRATCH0_OPND); *right = split_stack_membase(asm, *right, SCRATCH1_OPND, &stack_state); *right = split_64bit_immediate(asm, *right, SCRATCH1_OPND); - *out = split_stack_membase(asm, *out, SCRATCH1_OPND, &stack_state); + *out = lower_stack_membase(*out, &stack_state); let (out, left) = (*out, *left); asm.push_insn(insn); @@ -484,6 +496,7 @@ impl Assembler { *left = split_if_both_memory(asm, *left, *right, SCRATCH0_OPND); *right = split_stack_membase(asm, *right, SCRATCH1_OPND, &stack_state); *right = split_64bit_immediate(asm, *right, SCRATCH1_OPND); + *out = lower_stack_membase(*out, &stack_state); // imul doesn't have (Mem, Reg) encoding. Swap left and right in that case. if let (Opnd::Mem(_), Opnd::Reg(_)) = (&left, &right) { @@ -544,7 +557,7 @@ impl Assembler { *left = split_stack_membase(asm, *left, SCRATCH1_OPND, &stack_state); *right = split_stack_membase(asm, *right, SCRATCH0_OPND, &stack_state); *right = split_if_both_memory(asm, *right, *left, SCRATCH0_OPND); - *out = split_stack_membase(asm, *out, SCRATCH1_OPND, &stack_state); + *out = lower_stack_membase(*out, &stack_state); let mem_out = split_memory_write(out, SCRATCH0_OPND); asm.push_insn(insn); if let Some(mem_out) = mem_out { @@ -553,7 +566,7 @@ impl Assembler { } Insn::Lea { opnd, out } => { *opnd = split_stack_membase(asm, *opnd, SCRATCH0_OPND, &stack_state); - *out = split_stack_membase(asm, *out, SCRATCH1_OPND, &stack_state); + *out = lower_stack_membase(*out, &stack_state); let mem_out = split_memory_write(out, SCRATCH0_OPND); asm.push_insn(insn); if let Some(mem_out) = mem_out { @@ -570,7 +583,7 @@ impl Assembler { Insn::LoadInto { dest: out, opnd } => { *opnd = split_stack_membase(asm, *opnd, SCRATCH0_OPND, &stack_state); // Split stack membase on out before checking for memory write - *out = split_stack_membase(asm, *out, SCRATCH1_OPND, &stack_state); + *out = lower_stack_membase(*out, &stack_state); let mem_out = split_memory_write(out, SCRATCH0_OPND); asm.push_insn(insn); if let Some(mem_out) = mem_out { diff --git a/zjit/src/codegen_tests.rs b/zjit/src/codegen_tests.rs index a65949aa2ed397..c3b67b9ff89865 100644 --- a/zjit/src/codegen_tests.rs +++ b/zjit/src/codegen_tests.rs @@ -1905,6 +1905,55 @@ fn test_opt_neq_fixnum() { "), @"[false, true]"); } +#[test] +fn test_opt_neq_string_nil() { + assert_snapshot!(inspect(r#" + def test(str) = str != nil + test("x") # profile opt_neq + [test("x"), test(nil)] + "#), @"[true, false]"); +} + +#[test] +fn test_opt_neq_string_same_operand() { + assert_snapshot!(inspect(r#" + def test(s) = s != s + test("x") # profile opt_neq + [test("x"), test("y")] + "#), @"[false, false]"); + assert_contains_opcode("test", YARVINSN_opt_neq); +} + +#[test] +fn test_opt_neq_string_distinct_literals() { + assert_snapshot!(inspect(r#" + def test = "a" != "b" + test # profile opt_neq + [test, test] + "#), @"[true, true]"); + assert_contains_opcode("test", YARVINSN_opt_neq); +} + +#[test] +fn test_opt_neq_string_one_side_known_literal() { + assert_snapshot!(inspect(r#" + def test(s) = "a" != s + test("a") # profile opt_neq + [test("a"), test("b")] + "#), @"[false, true]"); + assert_contains_opcode("test", YARVINSN_opt_neq); +} + +#[test] +fn test_opt_neq_string_distinct_objects() { + assert_snapshot!(inspect(r#" + def test(s, t) = s != t + test("x", "x") # profile opt_neq + [test("x", "x"), test("x", "y")] + "#), @"[false, true]"); + assert_contains_opcode("test", YARVINSN_opt_neq); +} + #[test] fn test_opt_eq_string_same_operand() { assert_snapshot!(inspect(r#" diff --git a/zjit/src/cruby_methods.rs b/zjit/src/cruby_methods.rs index 675e1308b526da..d39f38028743c5 100644 --- a/zjit/src/cruby_methods.rs +++ b/zjit/src/cruby_methods.rs @@ -19,6 +19,7 @@ unsafe extern "C" { fn rb_jit_ary_at_end(ec: EcPtr, self_: VALUE, index: VALUE) -> VALUE; fn rb_jit_ary_at(ec: EcPtr, self_: VALUE, index: VALUE) -> VALUE; fn rb_jit_fixnum_inc(ec: EcPtr, self_: VALUE, num: VALUE) -> VALUE; + fn rb_str_equal(str1: VALUE, str2: VALUE) -> VALUE; } pub struct Annotations { @@ -543,8 +544,7 @@ fn inline_string_append(fun: &mut hir::Function, block: hir::BlockId, recv: hir: None } -fn inline_string_eq(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, args: &[hir::InsnId], state: hir::InsnId) -> Option { - let &[other] = args else { return None; }; +fn try_inline_string_equal(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, other: hir::InsnId, state: hir::InsnId) -> Option { if fun.likely_a(recv, types::String, state) && fun.likely_a(other, types::String, state) { let recv = fun.coerce_to(block, recv, types::String, state); let other = fun.coerce_to(block, other, types::String, state); @@ -554,6 +554,11 @@ fn inline_string_eq(fun: &mut hir::Function, block: hir::BlockId, recv: hir::Ins None } +fn inline_string_eq(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, args: &[hir::InsnId], state: hir::InsnId) -> Option { + let &[other] = args else { return None; }; + try_inline_string_equal(fun, block, recv, other, state) +} + fn inline_module_eqq(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, args: &[hir::InsnId], _state: hir::InsnId) -> Option { let &[other] = args else { return None; }; if fun.is_a(recv, types::Class) { @@ -737,12 +742,36 @@ fn inline_basic_object_not(fun: &mut hir::Function, block: hir::BlockId, recv: h None } +fn try_inline_string_not_equal(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, other: hir::InsnId, state: hir::InsnId) -> Option { + if !fun.likely_a(recv, types::String, state) || !fun.likely_a(other, types::String, state) { + return None; + } + let recv_class = fun.type_of(recv).runtime_exact_ruby_class()?; + + // String#!= is lowered to #==. Keep this specialization only while #== + // still resolves to rb_str_equal. + if !fun.assume_expected_cfunc(block, recv_class, ID!(eq), rb_str_equal as _, state) { + return None; + } + + let eq_result = try_inline_string_equal(fun, block, recv, other, state)?; + // StringEqual always returns a Ruby boolean (Qtrue/Qfalse), + // so `!=` can be lowered to `eq_result != Qtrue`. + let true_val = fun.push_insn(block, hir::Insn::Const { val: hir::Const::Value(Qtrue) }); + let not_equal = fun.push_insn(block, hir::Insn::IsBitNotEqual { left: eq_result, right: true_val }); + Some(fun.push_insn(block, hir::Insn::BoxBool { val: not_equal })) +} + fn inline_basic_object_neq(fun: &mut hir::Function, block: hir::BlockId, recv: hir::InsnId, args: &[hir::InsnId], state: hir::InsnId) -> Option { let &[other] = args else { return None; }; - let result = try_inline_fixnum_op(fun, block, &|left, right| hir::Insn::FixnumNeq { left, right }, BOP_NEQ, recv, other, state); - if result.is_some() { - return result; + if let Some(result) = try_inline_fixnum_op(fun, block, &|left, right| hir::Insn::FixnumNeq { left, right }, BOP_NEQ, recv, other, state) { + return Some(result); } + + if let Some(result) = try_inline_string_not_equal(fun, block, recv, other, state) { + return Some(result); + } + let recv_class = fun.type_of(recv).runtime_exact_ruby_class()?; if !fun.assume_expected_cfunc(block, recv_class, ID!(eq), rb_obj_equal as _, state) { return None; diff --git a/zjit/src/hir/opt_tests.rs b/zjit/src/hir/opt_tests.rs index 5df49c5ae6ba8f..aa706f7ddaeadd 100644 --- a/zjit/src/hir/opt_tests.rs +++ b/zjit/src/hir/opt_tests.rs @@ -11579,7 +11579,79 @@ mod hir_opt_tests { } #[test] - fn test_not_fold_string_not_equal_same_operand_false() { + fn opt_neq_string_nil_falls_back_to_basic_object_neq() { + eval(r#" + def test(str) + str != nil + end + + test("x") + test("x") + "#); + assert_snapshot!(hir_string("test"), @" + fn test@:3: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :str@0x1000 + Jump bb3(v1, v3) + bb2(): + EntryPoint JIT(0) + v6:BasicObject = LoadArg :self@0 + v7:BasicObject = LoadArg :str@1 + Jump bb3(v6, v7) + bb3(v9:BasicObject, v10:BasicObject): + v15:NilClass = Const Value(nil) + PatchPoint NoSingletonClass(String@0x1008) + PatchPoint MethodRedefined(String@0x1008, !=@0x1010, cme:0x1018) + v27:StringExact = GuardType v10, StringExact + v28:BoolExact = CCallWithFrame v27, :BasicObject#!=@0x1040, v15 + CheckInterrupts + Return v28 + "); + } + + #[test] + fn test_inline_string_not_equal_distinct_objects() { + eval(r#" + def test(s, t) = s != t + test("x", "x") + test("x", "x") + "#); + assert_contains_opcode("test", YARVINSN_opt_neq); + assert_snapshot!(hir_string("test"), @" + fn test@:2: + bb1(): + EntryPoint interpreter + v1:BasicObject = LoadSelf + v2:CPtr = LoadSP + v3:BasicObject = LoadField v2, :s@0x1000 + v4:BasicObject = LoadField v2, :t@0x1001 + Jump bb3(v1, v3, v4) + bb2(): + EntryPoint JIT(0) + v7:BasicObject = LoadArg :self@0 + v8:BasicObject = LoadArg :s@1 + v9:BasicObject = LoadArg :t@2 + Jump bb3(v7, v8, v9) + bb3(v11:BasicObject, v12:BasicObject, v13:BasicObject): + PatchPoint NoSingletonClass(String@0x1008) + PatchPoint MethodRedefined(String@0x1008, !=@0x1010, cme:0x1018) + v29:StringExact = GuardType v12, StringExact + PatchPoint MethodRedefined(String@0x1008, ==@0x1040, cme:0x1048) + v33:String = GuardType v13, String + v34:BoolExact = StringEqual v29, v33 + v35:TrueClass = Const Value(true) + v36:CBool = IsBitNotEqual v34, v35 + v37:BoolExact = BoxBool v36 + CheckInterrupts + Return v37 + "); + } + + #[test] + fn test_fold_string_not_equal_same_operand_false() { eval(r#" def test(s) = s != s test("x") @@ -11603,9 +11675,14 @@ mod hir_opt_tests { PatchPoint NoSingletonClass(String@0x1008) PatchPoint MethodRedefined(String@0x1008, !=@0x1010, cme:0x1018) v26:StringExact = GuardType v10, StringExact - v27:BoolExact = CCallWithFrame v26, :BasicObject#!=@0x1040, v10 + PatchPoint MethodRedefined(String@0x1008, ==@0x1040, cme:0x1048) + v30:String = GuardType v10, String + v35:TrueClass = Const Value(true) + v32:TrueClass = Const Value(true) + v33:CBool = IsBitNotEqual v35, v32 + v34:BoolExact = BoxBool v33 CheckInterrupts - Return v27 + Return v34 "); }