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

Update specs for 3.4 behavior changes #1207

Closed
headius opened this issue Oct 30, 2024 · 28 comments
Closed

Update specs for 3.4 behavior changes #1207

headius opened this issue Oct 30, 2024 · 28 comments

Comments

@headius
Copy link
Contributor

headius commented Oct 30, 2024

ruby/spec is currently not being run on Ruby 3.4, and as a result there are a number of failures. Most of these are related to the new Hash#inspect formatting (preferring a: 1 over :a => 1 with some spacing changes) but there are others.

We will need to update the specs and add version guards for cosmetic changes like this, and audit the remaining specs for real behavior changes that should be guarded or added.

@eregon
Copy link
Member

eregon commented Oct 30, 2024

Ruby 3.4 is not yet released, so it doesn't make sense to test here against it as it keeps changing.
ruby/spec is run in ruby/ruby's CI.
So basically this will be solved the next time specs are synchronized by @andrykonchin.
https://github.com/ruby/spec?tab=readme-ov-file#synchronization-with-ruby-implementations

@eregon
Copy link
Member

eregon commented Oct 30, 2024

For example see ruby/ruby@c94815b which will be mirrored here at the next synchronization.

@headius
Copy link
Contributor Author

headius commented Oct 30, 2024

this will be solved the next time specs are synchronized

Sounds good.

@eregon
Copy link
Member

eregon commented Oct 30, 2024

If you mean creating an item list like #1016 for 3.3 or 3.4, feel free to create it.
It's just the NEWS file with some extra text at the beginning and some reformatting so the checkboxes are at the top-level.

@headius
Copy link
Contributor Author

headius commented Oct 31, 2024

We are interested in syncing the specs sooner than whatever schedule @andrykonchin does it. I'm reading through the instructions for upstreaming and downstreaming now.

@headius
Copy link
Contributor Author

headius commented Oct 31, 2024

I attempted to sync from MRI and got an error about the last merge being too long ago?

To /Users/headius/work/rubyspec
 * [new branch]            specs-2024-10-31 -> mri
git checkout master
Already on 'master'
Your branch is up to date with 'origin/master'.
git pull
Already up to date.
git checkout mri
Switched to branch 'mri'
Last merge is 747957569427f35043aa74a4df610339464a9dad
sync-rubyspec.rb:156:in `block in rebase_commits': 121 days since last merge, probably wrong commit (RuntimeError)
	from sync-rubyspec.rb:126:in `chdir'
	from sync-rubyspec.rb:126:in `rebase_commits'
	from sync-rubyspec.rb:233:in `block in main'
	from sync-rubyspec.rb:229:in `each_pair'
	from sync-rubyspec.rb:229:in `main'
	from sync-rubyspec.rb:254:in `<main>'

This is with mspec and rubyspec as sibling directories both on master and following the instructions at https://github.com/ruby/spec/wiki/Merging-specs-from-JRuby-and-other-sources

@eregon
Copy link
Member

eregon commented Oct 31, 2024

There is an env var to skip that check, see the source of that script.

@headius
Copy link
Contributor Author

headius commented Oct 31, 2024

Yes, I saw that, but has it really been that long since the specs were synced? If so that's fine, I just thought I did something wrong.

@eregon
Copy link
Member

eregon commented Oct 31, 2024

Yes, the last sync was on July 1: ruby/ruby#11080 jruby/jruby#8311

@headius
Copy link
Contributor Author

headius commented Oct 31, 2024

I made an attempt here but a huge amount of pack and unpack specs are failing. They appear to have no version guards.

@headius
Copy link
Contributor Author

headius commented Oct 31, 2024

Ahh I see, there are mspec changes that need to be synced as well. I'll try to pick this up tomorrow.

@eregon
Copy link
Member

eregon commented Nov 1, 2024

Yes, MSpec should be sync'd first as documented in https://github.com/ruby/spec/wiki/Merging-specs-from-JRuby-and-other-sources#upstream-mspec-changes-in-mri-jruby-and-truffleruby
Please do all the steps as documented there (in order), otherwise it might be hard to fix it or understand what's going on with some later issue.

@headius
Copy link
Contributor Author

headius commented Nov 1, 2024

The first thing the document does is list the order in which things should be synced:

Synchronizing specs between the ruby/spec repository and Ruby/JRuby/TruffleRuby should be performed in the following order:

  • pushing changes made in Ruby implementations to ruby/spec (upstreaming)
  • pulling ruby/spec changes back from ruby/spec to Ruby implementations (downstreaming)

So that is where I started. Perhaps the document should be updated to make it clear what the actual sequence of steps is supposed to be.

@eregon
Copy link
Member

eregon commented Nov 1, 2024

That's the general overview of the approach, the expectation is steps are done in the order they are written.
Feel free to update the wiki if it's not clear.

@headius
Copy link
Contributor Author

headius commented Nov 5, 2024

It appears the issues were not entirely my fault. CRuby has made backward-incompatible changes to mspec that prevent current specs from passing: ruby/mspec#70

Specifically, they changed the way that some platform details are guarded, for example the platform bit width for native numeric types. Without mspec and rubyspec updating at the same time it's not possible to pass CI on either side.

Feel free to update the wiki if it's not clear.

Done. I will probably have additional edits.

@headius
Copy link
Contributor Author

headius commented Nov 5, 2024

With both mspec and ruby/spec changes from CRuby rebased, it does pass as expected.

@headius
Copy link
Contributor Author

headius commented Nov 5, 2024

I have restored the removed wordsize items as deprecated in ruby/mspec#70 and I am unblocked from proceeding.

@headius
Copy link
Contributor Author

headius commented Nov 5, 2024

It is unclear to me whether upstreaming changes should be one PR or multiple. The script creates separate branches for each impl.

@eregon
Copy link
Member

eregon commented Nov 5, 2024

The script updates your local master branch (unless there are conflicts, in which case you should rerun the script after fixing the conflicts).
Then you can create a single PR from your local master branch to ruby/spec.
Or push directly since anyway the script will run the specs locally on the oldest and newest CRuby.

@headius
Copy link
Contributor Author

headius commented Nov 6, 2024

Fixed by #1210.

@headius headius closed this as completed Nov 6, 2024
@eregon
Copy link
Member

eregon commented Nov 6, 2024

@headius Did you do the Downstream ruby/spec to MRI, JRuby and TruffleRuby part like the wiki says?
This is critical, otherwise it's basically impossible to sync next time (due to accumulation of conflicts and copies of ruby/spec diverging in those repos).
The fundamental issue there is we simply reclone mspec & ruby/spec in each Ruby implementation, overriding any changes (there are some safety check on the next sync, but they cannot catch everything).
So if there are any specs committed in these repo between sync'ing to ruby/spec and back, they need to be imported to ruby/spec first manually or with LAST_MERGE= and the script.

@eregon eregon reopened this Nov 6, 2024
@eregon
Copy link
Member

eregon commented Nov 6, 2024

I'll fix it, although I don't really have time for that today.
The whole sync'ing should happen in the same day, and creating the PR to the Ruby implementations too.
For CRuby the PR/push should be merged on the same day (because CRuby does not have merge commits), for JRuby/TruffleRuby they need to be created on the same day but can be merged later with a merge commit without issues.

@eregon
Copy link
Member

eregon commented Nov 6, 2024

Done, I used the script to extract the new upstream spec commits from ruby & truffleruby in between, by cherry-picking the changes from the mri and truffleruby branches: a230079...04a07e0
And pushed to CRuby and created jruby/jruby#8413 and a TruffleRuby PR.

@eregon eregon closed this as completed Nov 6, 2024
@headius
Copy link
Contributor Author

headius commented Nov 6, 2024

Did you do the Downstream

I did JRuby first because that's what I was most interested in catching up to 3.4 specs. I would have continued with the other two but you got to it while I was working on the JRuby PR.

@eregon
Copy link
Member

eregon commented Nov 7, 2024

Reflecting back on this, thank you for filing the issue, due to several events Andrii missed the few last syncs.
Based on #1207 (comment) you look like you wanted to sync specs ASAP (not even asking when Andrii or I would be able to do it) and I figured I should let you try it so you get an idea what's involved in that and maybe you can even help with it in the future.
But the net result is unfortunately I had to spend more time explaining things, fixing the sync because there were new specs in CRuby & TruffleRuby added in between, deal with the transient sleep spec and getting more stress than if I did the sync myself.
It would have been better to wait a few days for Andrii or me to do the sync.

@headius
Copy link
Contributor Author

headius commented Nov 7, 2024

It would have been better to wait a few days for Andrii or me to do the sync.

It had been months since the last sync and I needed it done, so I wanted to take this opportunity to try to learn the process so I could help do it as needed in the future. Three months is too long to go without syncing, and I'm sure many of the problems I ran into were because of that.

I will continue to do syncs as needed because the once-monthly sync was not keeping up with the specs we need to pull from CRuby for 3.4, and we need to stay on top of that through the final release. Thank you for helping me to learn how to handle issues during the sync process; I will also continue to update the wiki with details I learned along the way.

Regarding specific issues you mentioned...

I had to spend more time explaining things

I believe this is called teaching. I needed to learn the process and you taught me. Thank you. I assume you did the same for Andrii when he started out.

fixing the sync because there were new specs

I was not aware of the time-sensitive nature of syncing specs back to CRuby because it was not mentioned in the wiki document. Thank you for letting me know and helping to finish the process.

I notice you have updated the document to make clear that the entire sync process should happen the same day; perhaps that should be modified to include "as quickly as possible" since it is surprisingly problematic if more specs come in during the sync. I was planning to finish the sync all in the same day, but that clearly was not quick enough.

deal with the transient sleep spec

Again, the spec had been passing for JRuby for several weeks without a single failure, passed on my local system with CRuby, and has passed every single ruby/spec CI run on Linux and Windows after the merge.

The failure on Darwin seems like much more than a transient issue; the sleep times are off by a huge margin. I could not find where it was failing for CRuby since they generate thousands of CI runs per day, but I'd like to see why it failed.

However... the spec is written to ensure that sub-millisecond sleeps work correctly and actually do sleep; perhaps it's enough to make sure that 100 sleeps of 0.1ms adds up to at least 10ms and drop the less-than check. Any failure from that point would clearly mean the sleeps were not sleeping for at least 0.1ms and something is broken.

getting more stress than if I did the sync myself

Sounds like OSS to me. Delegating is hard.

@eregon
Copy link
Member

eregon commented Nov 7, 2024

I will continue to do syncs as needed because the once-monthly sync was not keeping up with the specs we need to pull from CRuby for 3.4, and we need to stay on top of that through the final release.

Why does JRuby suddenly want very frequent syncs where monthly was enough for many years?
Are you planning to release JRuby with 3.4 support on the same day than CRuby 3.4.0 or so maybe?

In any case, please let us know whenever you sync to avoid duplicated work (or let's design some schedule).
So far Andrii or I would sync around the beginning of the month.

I believe this is called teaching. I needed to learn the process and you taught me. Thank you. I assume you did the same for Andrii when he started out.

Yes, except I did it over a call with him so he knew all the details.
It's hard to document everything in the wiki.

I was not aware of the time-sensitive nature of syncing specs back to CRuby because it was not mentioned in the wiki document. Thank you for letting me know and helping to finish the process.
perhaps that should be modified to include "as quickly as possible"

Will do, also I'll edit the wiki to make it clear whoever makes the sync has to do everything themselves, it's not OK to ask others to help because that would add non-trivial delays, even more so since we are not in the same time zone.
Of course if there is a non-trivial problem it's good to mention it, but not ask for others to fix it.
Whenever I or Andrii found problematic specs we fixed them during the sync, it has to be like that to be fast enough and not introduce transients in many CIs. I see it as part of work of maintaining ruby/spec basically.

I appreciate your help with syncing, hopefully it will be smoother next time.

@headius
Copy link
Contributor Author

headius commented Nov 7, 2024

Are you planning to release JRuby with 3.4 support

JRuby 10 will be released with Ruby 3.4 support early in 2025.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants