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

Add Ruby 3.4 to CI #1233

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Add Ruby 3.4 to CI #1233

wants to merge 6 commits into from

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Dec 31, 2024

Most of it is rather straightforward, except for the changes to fiber/storage_spec. This has been a very recent change (https://bugs.ruby-lang.org/issues/20978), and is not mentioned in the changelog (nor in the documentation). This file differed on two locations from this version. I made a total of three changes:

  1. commit 98b8557 just copies the file from MRI over our local file
  2. commit b1431f9 updates this new version: There was a ruby_bug statement with the version constraint "3.2.3"..."3.4", but it failed at 3.2.2 too (and coincidentally the ruby/spec CI uses Ruby 3.2.2). The lower bound on the version constraint was superfluous, I removed that one and tested it with 3.2.2 and 3.2.3. (This feature has been introduced in Ruby 3.2, so 3.1 and 3.0 skip this file completely). It also updates the test to keep the Fiber variables locally, so they don't leak to the other tests.
  3. commit 697c22e expands upon those tests

I am not sure how this will interact with the periodic sync between this repo and the MRI repo, so if this will cause any conflicts in the sync, just let me know how to prevent that.

This removes one of our tests, and the new test is very minimal. This should be
extended, but at least it enables us to run the spec under Ruby 3.4.
* Remove the 3.2.3 lower bound, this spec fails on Ruby 3.2.0-3.2.2 (this file
  does not run on Ruby 3.1 and below)
* Run the test inside a new Fiber, we do not want to clutter the Fiber running
  the specs with additional keys.
But not anything else that responds to #to_sym. It does accept objects that
respond to #to_str.
This expands upon the ruby_bug block from upstream MRI. This also restores the
previously removed "can't use invalid keys" test, but with the String key
removed.

Reference: https://bugs.ruby-lang.org/issues/20978
@herwinw
Copy link
Member Author

herwinw commented Dec 31, 2024

I'm not sure what is happening with those leaking threads in the net-ftp folder: they don't seem to be stable, and they don't appear to show up in my fork (https://github.com/herwinw/spec/actions/runs/12559237976/job/35014669524). I can't reproduce the locally either

@herwinw herwinw force-pushed the ruby_3_4 branch 2 times, most recently from 5029b6d to 585b31c Compare December 31, 2024 12:22
@eregon
Copy link
Member

eregon commented Dec 31, 2024

I am not sure how this will interact with the periodic sync between this repo and the MRI repo, so if this will cause any conflicts in the sync, just let me know how to prevent that.

So normally we do the sync first, because then it means that version of the specs are passing on 3.4 & master, and then add 3.4 to CI here.
So let's wait for the next sync first.

BTW I'll also add 3.4 to https://github.com/ruby/ruby/blob/c0e2623966ea72b2b7781a13dab47ad50c362c98/.github/workflows/spec_guards.yml#L45 that can be done now: ruby/ruby#12492

This file has been copied from MRI (commit
341503d1a3a95253ddacdd3bd1c4a7dcdbbe49f2). This change should be removed once
we've synced the specs and rebase this branch.
Using a hostname does cause an occasional leaking thread in the Ruby socket
library when using Ruby 3.4.1. Switching to the IP address mitigates the issue.
@herwinw
Copy link
Member Author

herwinw commented Dec 31, 2024

I have mitigated the issue with the leaking file descriptors by switching the FTP stub server from localhost to 127.0.0.1. This resolves the issue for the specs, but might indicate an underlying problem in MRI that we're just ignoring now. I'll try to look into that later this week. (Note to self for i in {1..10}; do CHECK_LEAKS=true ../mspec/bin/mspec -j library/net-ftp; done seems to reproduce the issue on my system).

I will mark this PR as draft for now, and reactivate it once we've had the merge with upstream MRI.

@herwinw herwinw marked this pull request as draft December 31, 2024 16:48
@herwinw herwinw self-assigned this Dec 31, 2024
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