Skip to content

Conversation

@nevans
Copy link
Contributor

@nevans nevans commented Dec 11, 2025

@eregon
Copy link
Member

eregon commented Dec 11, 2025

Could you also cherry-pick the tests from #761 and adapt them so this PR also fixes #760? (I think it already does but it's not tested)

@headius
Copy link
Contributor

headius commented Dec 11, 2025

I'm +1 for this change but I still think we should disallow anonymous Data types from being dumped. It doesn't do what anyone would expect and will create a new Data class for each such object in the stream. I'll put together a PR.

@nevans nevans force-pushed the replace-c-ext-init_data-with-Data-initialize-bind_call branch from d38dade to 6a82669 Compare December 11, 2025 21:57
@headius
Copy link
Contributor

headius commented Dec 11, 2025

FWIW, JRuby is not passing these tests. @nobu modified the build to pass even if JRuby is failing in #719. I'm not sure why.

@headius
Copy link
Contributor

headius commented Dec 11, 2025

The instance variable-related tests on JRuby will be fixed by jruby/jruby#9138.

Instance variables on Data subtypes is still pretty hacky and only supports immediate children, but the fix here passes all tests on this branch and instance variables on direct subtypes of a Data type work properly.

@nevans
Copy link
Contributor Author

nevans commented Dec 11, 2025

@eregon I cherry-picked your tests from #761. Your error messages were much nicer, but it's simpler to just use Data#initialize's exception. Also, I dropped the order-dependent test.

@nevans
Copy link
Contributor Author

nevans commented Dec 11, 2025

@headius I deleted that continue-on-error for this branch.

@headius
Copy link
Contributor

headius commented Dec 11, 2025

@nevans Thank you; I believe once my JRuby PR lands this should be green on jruby-head.

@headius
Copy link
Contributor

headius commented Dec 12, 2025

I restarted the failed jobs since I believe both JRuby and TR have pushed updated head builds.

@headius
Copy link
Contributor

headius commented Dec 12, 2025

Something very strange going on with the JRuby job on Windows but it's very unlikely to be related to this PR. I'll look into it tomorrow.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks great!

@eregon
Copy link
Member

eregon commented Dec 13, 2025

Nit: the comments in .github/workflows/test.yml should probably be updated.

Also if it's only JRuby on Windows failing I'd be inclined to exclude it for now, and reenable it when it's fixed (still better than no JRuby testing at all like on current master).

@eregon
Copy link
Member

eregon commented Dec 13, 2025

Re JRuby on Windows the failure is:
https://github.com/ruby/psych/actions/runs/20150642697/job/57857789416?pr=765#step:6:1925

   bad class file: D:\jruby-head\lib\jruby.jar(/org/jruby/Ruby.class)
    class file has wrong version 65.0, should be 61.0
    Please remove or make sure it appears in the correct subdirectory of the classpath.

Which means javac does not care about JAVA_HOME, only PATH (makes sense).
IIRC there was a discussion with @kou about this on some default gem, and I also found rake-compiler/rake-compiler#247 so maybe just a too old rake-compiler?

@eregon
Copy link
Member

eregon commented Dec 13, 2025

Actually rake-compiler is already 1.3.0 which has this PR:
https://github.com/ruby/psych/actions/runs/20150642697/job/57857789416?pr=765#step:5:32
I suspect JAVA_HOME: C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\21.0.9-10.0\x64 + File.join(java_home, "bin", "javac") might not do the right thing then, or somehow the File.exist? check fails.

@eregon
Copy link
Member

eregon commented Dec 13, 2025

I found the issue: rake-compiler/rake-compiler#247 (comment)
So I think we should exclude jruby on windows here and fix that separately, as it will likely need a fix in either jruby or rake-compiler.

@eregon
Copy link
Member

eregon commented Dec 13, 2025

I've rerun the jruby windows job after ruby/setup-ruby#836 so it should be green now, or at least proceed further than javac failing.

@eregon
Copy link
Member

eregon commented Dec 13, 2025

Nit: the comments in .github/workflows/test.yml should probably be updated.

I pushed a commit for that.

@tenderlove tenderlove merged commit 3ae76eb into ruby:master Dec 15, 2025
112 of 113 checks passed
@headius
Copy link
Contributor

headius commented Dec 15, 2025

Thanks for the extra fix in setup-ruby!

@nevans nevans deleted the replace-c-ext-init_data-with-Data-initialize-bind_call branch December 16, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants