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

Switch to Ruby 3.4.2 and 3.2.7 #1245

Merged
merged 2 commits into from
Feb 17, 2025
+1 −7
Merged

Switch to Ruby 3.4.2 and 3.2.7 #1245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7e52a5
Switch CI to Ruby 3.2.7 and 3.4.2
herwinw Feb 15, 2025
c61a0dc
Update finalizer spec to run with Ruby 3.4.2
herwinw Feb 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
fail-fast: false
matrix:
os: [ ubuntu, macos, windows ]
ruby: [ 3.1.6, 3.2.6, 3.3.7, 3.4.1 ]
ruby: [ 3.1.6, 3.2.7, 3.3.7, 3.4.2 ]
rubyopt: [""]
include:
- os: ubuntu
Expand Down
6 changes: 0 additions & 6 deletions optional/capi/ext/finalizer_spec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,7 @@ static VALUE define_finalizer(VALUE self, VALUE obj, VALUE finalizer) {
}

static VALUE undefine_finalizer(VALUE self, VALUE obj) {
// Ruby 3.4.0 and 3.4.1 have a bug where rb_undefine_finalizer is missing
// See: https://bugs.ruby-lang.org/issues/20981
#if RUBY_API_VERSION_CODE == 30400 && (RUBY_VERSION_TEENY == 0 || RUBY_VERSION_TEENY == 1)
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 we shouldn't need to change this, i.e. this spec should now work on 3.4.2+

Copy link
Member

Choose a reason for hiding this comment

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

Ah the commit message explains the problem

Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/ruby/spec/blob/master/optional/capi/ext/rubyspec.h which uses RUBY_VERSION_MAJOR/MINOR/TEENY.

I wonder if we fall into

spec/optional/capi/ext/rubyspec.h

Line 40 in 6f2ec17

#define RUBY_VERSION_TEENY RUBY_API_VERSION_TEENY
and then indeed it would be 0 due to RUBY_API_VERSION_* being the ABI version, which doesn't change on minor or patch releases.

Copy link
Member

Choose a reason for hiding this comment

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

I asked on the CRuby Slack.
See https://github.com/ruby/ruby/blob/23c4ac9559a05d7be4534d8e552d8dc95e272867/version.h#L12 and https://github.com/ruby/ruby/blob/master/include/ruby/version.h.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/ruby/ruby/blob/ruby_3_4/version.h#L12 so it should work.
Maybe the problem is the extension didn't recompile locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested this with an rbenv install, and the version.h (the one in the root, so not the ruby/version.h does not get installed into the include path. So all we have here is the API version

~/.rbenv/versions/3.4.2/include/ruby-3.4.0$ grep -r TEENY
ruby/oniguruma.h:#define ONIGURUMA_VERSION_TEENY ONIGMO_VERSION_TEENY
ruby/onigmo.h:#define ONIGMO_VERSION_TEENY   3
ruby/version.h:#define RUBY_API_VERSION_TEENY 0
ruby/version.h:#define RUBY_API_VERSION_CODE (RUBY_API_VERSION_MAJOR*10000+RUBY_API_VERSION_MINOR*100+RUBY_API_VERSION_TEENY)

(And ruby-3.4.0 is the only thing in the include folder).

Like I said in the commit message: I would be perfectly fine with enabling this spec on 3.4.2, but I simply cannot find a way to do so without breaking other things. So unless we want to do something like adding a configure script to the ext folder, I'm afraid we have to skip this one on every 3.4 release.

Copy link
Member

@eregon eregon Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I discussed it with @peterzhu2118 and @byroot, one idea is to add a -D with the real tiny version to the generated extconf.rb here:

spec/optional/capi/spec_helper.rb

Lines 71 to 88 in 6f2ec17

if run_mkmf_in_process
required = require 'mkmf'
# Reinitialize mkmf if already required
init_mkmf unless required
create_makefile(ext, tmpdir)
else
File.write("extconf.rb", <<-RUBY)
require 'mkmf'
$ruby = ENV.values_at('RUBY_EXE', 'RUBY_FLAGS').join(' ')
# MRI magic to consider building non-bundled extensions
$extout = nil
append_cflags '-Wno-declaration-after-statement'
create_makefile(#{ext.inspect})
RUBY
output = ruby_exe("extconf.rb")
raise "extconf failed:\n#{output}" unless $?.success?
$stderr.puts output if debug
end

Like $defs << "-DRUBY_SPEC_RUBY_PATCH_VERSION=#{RUBY_VERSION.split('.')[2]}" (or maybe RUBY_SPEC_RUBY_TEENY_VERSION).
Could you try that?

Also add a comment that RUBY_VERSION_TEENY can't be used because that's not defined for extensions, only in https://github.com/ruby/ruby/blob/ruby_3_4/version.h#L12 which is an internal header not exposed for C extensions.

BTW we should cleanup

spec/optional/capi/ext/rubyspec.h

Lines 37 to 41 in 6f2ec17

#ifndef RUBY_VERSION_MAJOR
#define RUBY_VERSION_MAJOR RUBY_API_VERSION_MAJOR
#define RUBY_VERSION_MINOR RUBY_API_VERSION_MINOR
#define RUBY_VERSION_TEENY RUBY_API_VERSION_TEENY
#endif
and not rely on RUBY_VERSION_* as they only exists within CRuby repo. We should just use RUBY_API_VERSION_* instead below. And probably drop the 3rd argument teeny since it's pointless as always 0. I can do that part.

Copy link
Member

@eregon eregon Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually there is also a simpler solution: since ruby/spec only supports the latest release of each release branch we could unconditionally use rb_undefine_finalizer(), and let 3.4.0/3.4.1 fail as they shouldn't be used.
It seems worth the simplification in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I love simple solutions ;)

Updated to the simple version. I left the version guard for 3.4.0-3.4.1 in the Ruby code, running it with these versions will show a compile warning, but will not break.

Copy link
Member

Choose a reason for hiding this comment

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

BTW we should cleanup

Done in 52178dd

return Qnil;
#else
return rb_undefine_finalizer(obj);
#endif
}

void Init_finalizer_spec(void) {
Expand Down