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 ruby to 3.0 #8452

Merged
merged 44 commits into from May 3, 2022
Merged

Update ruby to 3.0 #8452

merged 44 commits into from May 3, 2022

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Oct 31, 2021

🎩 What? Why?

Upgrade Decidim to Ruby 3.0.2

♥️ Thank you!

@andreslucena andreslucena mentioned this pull request Dec 20, 2021
12 tasks
@alecslupu alecslupu force-pushed the ruby-3.0 branch 2 times, most recently from e45ea1c to 357f504 Compare January 3, 2022 15:06
@andreslucena andreslucena changed the title Upgrade Decidim to Ruby 3.0 Update ruby to 3.0 Jan 4, 2022
ferblape
ferblape previously approved these changes Jan 5, 2022
Copy link
Contributor

@ferblape ferblape left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

@alecslupu You asked me previously about the problem with the attributes of type Rails::Engine. These changes need to be reverted (or changed to **{} in Ruby 3) for these attributes to work correctly.

This is explained in the CHANGELOG notes if you search for Rails::Engine from there.

There is a limitation in the Ruby language that if the method has default values for the previous arguments and defines keyword arguments, the last argument will always receive a respond_to?(:to_hash) call to it which doesn't work for Rails::Engine (you can try it out in the Rails console by calling Rails::Engine.respond_to?(:to_hash)).

EDIT:
Actually in Ruby 3 we just need to pass the hash as an empty keyword argument list as **{}. I'll modify my comments below.

@alecslupu alecslupu force-pushed the ruby-3.0 branch 3 times, most recently from dea01d7 to a9f7555 Compare March 14, 2022 06:58
@ahukkanen
Copy link
Contributor

@alecslupu Can you rebase this with develop please? 🙏

I'd like to get rid of the deprecation messages to review this properly.

Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

I'm not sure what to do about these but I would rather try to push these changes to the upstream repositories of these gems.

The Origami gem seems stale at the moment, though...

@andreslucena andreslucena added this to Pending review from Product in Maintainers via automation Mar 21, 2022
@alecslupu alecslupu force-pushed the ruby-3.0 branch 2 times, most recently from e62f98b to e6e4a38 Compare March 22, 2022 21:01
@ahukkanen
Copy link
Contributor

I can reproduce this.

Thanks @andreslucena for confirming!

I believe this has something to do with the version of Webpacker we are using not being thread safe.

If I do the following change at development_app/config/puma.rb:

# Comment this out:
# threads min_threads_count, max_threads_count
# Add this instead:
threads 1, 1

Then I cannot make this break anymore.

It has something to do webpacker trying to process the javascript_pack_tag or stylesheet_pack_tag concurrently in multiple threads.

@alecslupu
Copy link
Contributor Author

I can reproduce this.

Thanks @andreslucena for confirming!

I believe this has something to do with the version of Webpacker we are using not being thread safe.

If I do the following change at development_app/config/puma.rb:

# Comment this out:
# threads min_threads_count, max_threads_count
# Add this instead:
threads 1, 1

Then I cannot make this break anymore.

It has something to do webpacker trying to process the javascript_pack_tag or stylesheet_pack_tag concurrently in multiple threads.

Wow, Nice catch.

It seems that I already had the number of threads set to 1, 1, and this was the reason i could not replicate it.

@ahukkanen, Webpacker hits us again >:) ... Maybe it would make sense to continue the discussion about the import maps or alternatives.

@ahukkanen
Copy link
Contributor

The chdir issue seems to be Ruby 3 related, so we need to resolve this.

The reason why it hasn't failed "hard" with Ruby 2.x is this:
ruby/ruby#3591

They changed the conflicting chdir from warning to a RuntimeError in Ruby 3.

This is further explained here:
rails/webpacker#2801

I also found a commit in Shakapacker which fixes this issue (at least judging by the commit message):
shakacode/shakapacker@f2dc437

This fix seems to be similar to what is explained in that webpacker issue linked above.

@ahukkanen ahukkanen mentioned this pull request Apr 28, 2022
12 tasks
@ahukkanen
Copy link
Contributor

@ahukkanen, Webpacker hits us again >:) ... Maybe it would make sense to continue the discussion about the import maps or alternatives.

@alecslupu @andreslucena I am adding the proposed monkey patch in the related issue at #9203. Once we get that merged, we should be finally able to merge this one.

@andreslucena
Copy link
Member

@alecslupu @andreslucena I am adding the proposed monkey patch in the related issue at #9203. Once we get that merged, we should be finally able to merge this one.

I've just reviewed and merged it.

ahukkanen
ahukkanen previously approved these changes Apr 29, 2022
Copy link
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

This is good to go now after we got that thread safety issue resolved.

Can you also give your review @andreslucena to indicate @alecslupu that this is ready to be merged and we can revert the lines that we need to revert before merging.

andreslucena
andreslucena previously approved these changes May 2, 2022
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I've just give it a last spin locally and it's fine for me 👍🏽

@andreslucena
Copy link
Member

@alecslupu you can revert the lines anytime you can

@alecslupu alecslupu dismissed stale reviews from andreslucena and ahukkanen via 588460a May 2, 2022 14:14
@alecslupu alecslupu requested a review from ahukkanen May 2, 2022 14:20
@ahukkanen
Copy link
Contributor

@alecslupu Sorry there is still missing specs because of another recently merged PR: #8866.

The problem is on this line:

I'll fix it in another PR shortly.

@alecslupu
Copy link
Contributor Author

I was just looking on it ... and fixing from my end... but if you want to merge, to let it become Core, then i am okay with that

@ahukkanen ahukkanen mentioned this pull request May 2, 2022
12 tasks
@ahukkanen
Copy link
Contributor

I was just looking on it ... and fixing from my end... but if you want to merge, to let it become Core, then i am okay with that

Should be fixed by #9218.

@ahukkanen
Copy link
Contributor

@alecslupu Can you merge with develop please? Let's see that the CI is green and then we can finally merge this. 🤞

@alecslupu
Copy link
Contributor Author

@ahukkanen the pipeline is Green :)

@ahukkanen ahukkanen merged commit f402b6e into decidim:develop May 3, 2022
Maintainers automation moved this from To Review by Core to Done May 3, 2022
@ahukkanen
Copy link
Contributor

Enormous work here @alecslupu, many thanks to you and everyone involved! And thanks for the great patience with all the related issues and changes.

I've just merged with develop and let's see that the generators CI also passes after this.

@alecslupu alecslupu deleted the ruby-3.0 branch May 3, 2022 09:40
@ahukkanen ahukkanen mentioned this pull request May 10, 2022
12 tasks
@alecslupu alecslupu added this to the 0.27.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file or issues that talk about updating dependencies module: core release: v0.27 Issues or PRs that need to be tackled for v0.27 target: developer-experience
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants