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

Ruby3 Updates #438

Merged
merged 40 commits into from
May 14, 2022
Merged

Ruby3 Updates #438

merged 40 commits into from
May 14, 2022

Conversation

pderouen
Copy link
Member

@pderouen pderouen commented Apr 28, 2022

  • Minor updates to get tests passing with Ruby 3
  • Web compile is broken again in Windows, disabled that check for Windows GA
  • Web compile is also broken with Ruby 3 (Windows & Linux), disabled in GA
  • lint crashes when with Ruby 3 in GA, disabled that for now as well
  • gem install origen is not able to see anything newer than 0.30, so Ruby 3 GA builds then installs

@pderouen pderouen requested review from priyavadan and coreyeng April 28, 2022 18:07
@info-rchitect
Copy link
Member

@pderouen Any performance improvements noticed when running the specs and tests for 3.x?

@pderouen
Copy link
Member Author

@pderouen Any performance improvements noticed when running the specs and tests for 3.x?

Seems to be taking about the same amount of time when run with github actions

@pderouen
Copy link
Member Author

pderouen commented May 11, 2022

Assuming everybody is ok with the changes and the open issues, this PR can be merged. There's not a rush. But, it would be nice to have a version available to use with ruby 3.

@ginty
Copy link
Member

ginty commented May 11, 2022

It appears to be a 3.0 vs 3.1 issue. Rubocop 0.30.0 crashes for me with Ruby 3.1. It runs fine with 3.0.4. I tried moving to a slightly newer version of rubocop. All of the name spaces changed. I spend a bit of time making updates to the rules only to find that there's not really a 1 to 1 mapping of old rules/settings to new. How were those rule sets created in the first place?

The original rule sets were the defaults that Rubocop provided at the time.
I think I might have changed one or two because they were going against common idioms being used in our code base at the time.
I would recommend that you replace these with the new set of Rubocop defaults and then see what happens when you run it on our code base, maybe you'll be happy enough with any changes or it might drive you to tweak a couple of the rules.

@ginty
Copy link
Member

ginty commented May 11, 2022

Should the tests not also be run on Ruby 2, or is the intention to only support R3 from here?

@pderouen
Copy link
Member Author

pderouen commented May 11, 2022

Should the tests not also be run on Ruby 2, or is the intention to only support R3 from here?

The tests for Ruby 2.5, 2.6, and 2.7 still run. There are 4 work flows now because of the various issues (raised above):

  1. gem install origen with Ruby 3 is installing an ancient version (leads to errors during boot). So, for now there are "_Ruby3" versions of the work flows to build the gem and install locally (which wasn't working right with Ruby 2 and the bundler version).
  2. The web compile checks fail under Windows GA. So, there are separate windows work flows that skip the web compile check.

Windows Ruby 2 checks
Linux Ruby 2 checks
Windows Ruby 3 checks
Linux Ruby 3 checks

@pderouen
Copy link
Member Author

@ginty I think I'll be able to use just the 1 workflow by adding if conditionals. That should make it less convoluted to figure out what's going on with the test setups.

@ginty
Copy link
Member

ginty commented May 11, 2022

I'm not sure if I just missed the scroll bar when you show the checks below, or if it wasn't there before, but I can certainly see all the jobs now, thanks!

@priyavadan
Copy link
Member

I'm not sure if I just missed the scroll bar when you show the checks below, or if it wasn't there before, but I can certainly see all the jobs now, thanks!

I've noticed that some times it just take a bit longer to add all the jobs to the queue.

@pderouen
Copy link
Member Author

ok, I like this better. Just 1 workflow and it's easy to see what's being run for which conditions.

Copy link
Member

@ginty ginty left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@coreyeng coreyeng left a comment

Choose a reason for hiding this comment

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

What is the TL;DR on the 3.1 support? We have 3.1.2 on the network here but no 3.0. But I only see regressions run on 3.0. Can we add a 3.1 to it?

@pderouen
Copy link
Member Author

What is the TL;DR on the 3.1 support? We have 3.1.2 on the network here but no 3.0. But I only see regressions run on 3.0. Can we add a 3.1 to it?

The rubocop version (0.30.0) is not compatible with Ruby 3.1:

wrong number of arguments (given 2, expected 1)
C:/Ruby31-x64/lib/ruby/3.1.0/psych.rb:323:in `safe_load'
C:/Users/nxa18793/.origen/gems/ruby/3.1.0/gems/rubocop-0.30.0/lib/rubocop/config_loader.rb:129:in `yaml_safe_load'

That's the only issue with 3.1 (I think). I can add 3.1 to the checks and have it skip lint.

@pderouen
Copy link
Member Author

@coreyeng I can take a look at updating rubocop over the next week or so. I spent a few hours on it when I first ran into it and discovered that we just need all new rule sets if moving to a newer version.

@coreyeng
Copy link
Member

Sounds good. IMO, its not really Ruby 3 ready if we need to axe one of the main commands and checks to make it work.

I can probably help out too towards the end of the month if its still a lingering problem.

@pderouen
Copy link
Member Author

@coreyeng I've been sifting through the new lint violations (>6000 with the new default rules) and refining the rules to be closer to what we had before. It's looking like there will be quite a few code changes for lint (100's). I would like to go ahead and start a separate branch just for the lint upgrade. I will get the new branch pushed up to the repo in the next few days.

@coreyeng
Copy link
Member

Sounds good!

@pderouen pderouen merged commit e531b9c into master May 14, 2022
@pderouen pderouen deleted the ruby31_wip branch May 14, 2022 02:15
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.

5 participants