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

Use Rails 7.1 with Ruby head in turbo-rails CI #3320

Merged
merged 1 commit into from Feb 1, 2024

Conversation

joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Jan 17, 2024

Description

Context:

Experiment to see if this fixes a failing CI worflow since mutex_m was added to the relevant Rails gemfiles and included in the 7.1 release. It has also since been dropeed on Rails edge.

Even if this passes, the root cause is that it's missing from the gemfiles in https://github.com/ruby-concurrency/concurrent-ruby. I'm hoping to resolve that in ruby-concurrency/concurrent-ruby#1034.

However, we still need to point to a version of Rails that either requires mutex_m itelf or uses a version of concurrent-ruby that does, so this change is needed regardless of that patch.

I've also pointed the other rubies to 7.1 for consistency, please let me know if I should revert that.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@joshuay03 joshuay03 changed the title Use Rails 7.1 in turbo-rails CI Use Rails 7.1 in turbo-rails CI Jan 17, 2024
@joshuay03 joshuay03 force-pushed the fix-turbo-rails-ruby-edge-ci branch 2 times, most recently from 44a8f89 to 5b9a1c9 Compare January 17, 2024 23:34
@joshuay03 joshuay03 changed the title Use Rails 7.1 in turbo-rails CI Use Rails 7.1 with Ruby head in turbo-rails CI Jan 17, 2024
@joshuay03 joshuay03 marked this pull request as ready for review January 17, 2024 23:36
@eregon
Copy link
Contributor

eregon commented Jan 18, 2024

https://github.com/puma/puma/actions/runs/7529388305/job/20493515442 was using concurrent-ruby 1.2.2:
https://github.com/puma/puma/actions/runs/7529388305/job/20493515442#step:4:64
concurrent-ruby 1.2.3 no longer requires mutex_m.
From the stacktrace in that log though it seems to have nothing to do with concurrent-ruby, but active_support/notifications/fanout.rb seems to be the one requiring mutex_m.

@joshuay03
Copy link
Contributor Author

joshuay03 commented Jan 18, 2024

From the stacktrace in that log though it seems to have nothing to do with concurrent-ruby, but active_support/notifications/fanout.rb seems to be the one requiring mutex_m.

Ah yep my bad, the confusion was from me looking at the diff for the incorrect version. I thought it was from requiring concurrent/map here, but in fact it was requiring mutex_m directly, which has been removed here.

Thanks for having a look!

@MSP-Greg
Copy link
Member

@eregon Thanks for the research.

Note the last rows of https://github.com/ruby/ruby/blame/master/gems/bundled_gems. There have been more additions after mutex_m.

@joshuay03 - thanks for the PR. I haven't thought about it much (busy), but these jobs don't take long, wondering if there should be a mix of 7.0 and 7.1?

@joshuay03
Copy link
Contributor Author

joshuay03 commented Jan 18, 2024

@joshuay03 - thanks for the PR. I haven't thought about it much (busy), but these jobs don't take long, wondering if there should be a mix of 7.0 and 7.1?

Sounds good to me, I'll action when I get a chance 👍🏽

Edit: Actioned here.

@joshuay03 joshuay03 force-pushed the fix-turbo-rails-ruby-edge-ci branch 2 times, most recently from f254b9d to b410140 Compare January 19, 2024 00:39
@joshuay03
Copy link
Contributor Author

joshuay03 commented Jan 30, 2024

master is now failing cause it can't load base64. This should also fix that but I've rebased to confirm and it looks 👍🏽

@joshuay03
Copy link
Contributor Author

@dentarg @MSP-Greg Are you happy to merge this? The current turbo-rails failure is from upstream.

@dentarg dentarg merged commit c2aa737 into puma:master Feb 1, 2024
70 of 74 checks passed
@joshuay03 joshuay03 deleted the fix-turbo-rails-ruby-edge-ci branch February 1, 2024 07:13
@joshuay03
Copy link
Contributor Author

Thank you @dentarg

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

Successfully merging this pull request may close these issues.

None yet

4 participants