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 head and YJIT to CI #2030

Merged

Conversation

reeganviljoen
Copy link
Collaborator

What are you trying to accomplish?

I created this pr as a continuation of #2012 as a separate branch so @joelhawksley and anyone else can add to this branch

Add ruby head to CI as well as YJIT as discussed in #2010 by me and @joelhawksley

Anything you want to highlight for special attention from reviewers?

Ruby head seems to have to be failing on CI which needs further investigation into whether it is a ruby bug or if it really is a breaking change

@reeganviljoen
Copy link
Collaborator Author

@joelhawksley I have created this pr for the reasons listed in the description, can you please take another look at this when you can, I also know we are starting to fail on rails head so I will investigate that when I can

@camertron
Copy link
Contributor

Thanks for working on this @reeganviljoen! What would you think about always enabling YJIT? Is there a reason we would want to disable it, other than a particular Ruby version not supporting it?

We def need to fix those test failures for Ruby head and Rails head. I wonder if those could be separate PRs?

@reeganviljoen
Copy link
Collaborator Author

reeganviljoen commented May 23, 2024

@camertron I think it would be wise to enable both as some users still use MRI without YJIT, I am just a bit worried a bug in MRI is missed because we only test against YJIT

We def need to fix those test failures for Ruby head and Rails head. I wonder if those could be separate PRs?

I have already started working ion the rails head issues, spoiler the backtrace is working very differently now, but am having issues building ruby dev locally

@camertron
Copy link
Contributor

I think it would be wise to enable both as some users still use MRI without YJIT, I am just a bit worried a bug in MRI is missed because we only test against YJIT

Ok that's fair, I just worry that our build matrix is getting awfully large 😵

I have already started working ion the rails head issues, spoiler the backtrace is working very differently now, but am having issues building ruby dev locally

Awesome thank you! Let me know if I can help re: build issues.

@reeganviljoen
Copy link
Collaborator Author

@camertron i do agree it's getting large, maybe we can remove all unsupported rails and ruby version from the matrix

@camertron
Copy link
Contributor

maybe we can remove all unsupported rails and ruby version from the matrix

Yeah, I've considered that... I feel bad about it tho since so many people aren't using the latest versions, and we've always sort of said we'd support older Rails/Ruby versions as long as they were easy to support. So I guess we keep them in the matrix for now.

@reeganviljoen
Copy link
Collaborator Author

@camertron maybe as a compromise we can have a sunsetting deadline for older version support ?

@camertron
Copy link
Contributor

@reeganviljoen yeah, I can bring it up again at our next sync meeting.

@reeganviljoen
Copy link
Collaborator Author

reeganviljoen commented May 27, 2024

Awesome thank you! Let me know if I can help re: build issues.

@camertron Thanks much appreciated turns out I was missing a few build dependencies, it works lovely now, the ruby head issue is a mismatched regexp encoding(ASCII) with a matched string(UTF-8) I believe this is a bug, I will report it tomorrow to https://bugs.ruby-lang.org/

@reeganviljoen
Copy link
Collaborator Author

reeganviljoen commented May 30, 2024

@joelhawksley Rails Head is passing locally for me but not in CI, I suspect a caching issue or something
nevermind, I see you fixed it

@reeganviljoen
Copy link
Collaborator Author

reeganviljoen commented Jun 3, 2024

@joelhawksley @camertron @boardfish @Spone everything is finally passing on this PR can we get a look at it please 🙏

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@joelhawksley joelhawksley enabled auto-merge (squash) June 3, 2024 16:33
@joelhawksley joelhawksley disabled auto-merge June 3, 2024 16:51
@joelhawksley joelhawksley merged commit cc40fb8 into ViewComponent:main Jun 3, 2024
69 checks passed
@joelhawksley
Copy link
Member

@reeganviljoen thanks again, this is lovely ❤️

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.

3 participants