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

Fix frozen string literal issue for ruby 3.4 #300

Merged
merged 2 commits into from
Aug 1, 2024
Merged

Conversation

chaadow
Copy link
Contributor

@chaadow chaadow commented Jun 15, 2024

@chaadow
Copy link
Contributor Author

chaadow commented Jun 16, 2024

@geemus

  • I've decided to fix all frozen related issues in fog-core since I believe this is the gem that is most common to all fog dependencies.

  • I also fixed some mini test warnings displayed by the test suite ( not all warnings)

  • I had the same issue for my personal gem, but I think you need to enable CI again since it's disabled automatically by Github after 60 days of inactifvy https://github.com/fog/fog-core/actions/workflows/ci.yml

  • I was wondering, since CI targets only ruby 3.0+, would you mind changing the target ruby version of rubocop.yml?

    • The main change is to add # frozen_string_literal: true to all files
      • Since this PR is related to this subject would you mind me adding it to all files, or a separate PR perhaps?

@geemus
Copy link
Member

geemus commented Jun 17, 2024

Thanks for your continued efforts here.

  • Makes sense to focus on fixing fog-core.
  • Thanks for the minitest fixes.
  • Yes, I didn't realize they disabled things after 60 days inactivity, I've reactivated it.
  • I bumped the target in rubocop to 3.0 for now (we'll eventually want 3.3 I imagine, but I thought this would be a good next step).
  • Feel free to add the string literal magic comment in this PR or a separate one, whatever is easier for you.

@@ -12,5 +12,21 @@ permissions:
contents: read

jobs:
Shared:
uses: fog/.github/.github/workflows/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

Once this has settled, I think we'll want to go back to the shared action probably, so just noting that here so I don't forget.

@geemus
Copy link
Member

geemus commented Jun 17, 2024

Looks good so far, just let me know when you need me to take another look.

@voxik
Copy link
Contributor

voxik commented Jun 24, 2024

I was about submitting PR to drop the base64 and found it here (would be nice if it was independent PR IMHO). And I just wanted to point out that while fog-core does not use base64 bits, other libraries from the ecosystem might not load it by itself. However, those projects CIs are likely broken by this anyway, so just heads up.

@geemus
Copy link
Member

geemus commented Jul 16, 2024

@chaadow I'm forgetting now exactly where we left this (sorry). Is this where you wanted it to be, or was there still some work in progress? Just trying to figure out who should be waiting for whom since I can't quite remember. Thanks again for the help!

Copy link

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Reviewing this change, it looks good to me. The only thing I can't answer is on the CI side.

The difference is that it passes RUBYOPT="--enable-frozen-string-literal" which the shared flow doesn't. There's a bit of a chicken-and-egg situation, though head is currently allowed to fail. So you could submit the change already. head is broken anyway (as can be seen in fog-libvirt) so it probably wouldn't get worse.

Other than that, it doesn't trigger CI. That is because it uses a schedule in the same job, so it gets deactivated if there's no change for 60 days.

Looks like it's enabled again so I'd at least advise rebasing it.

@geemus
Copy link
Member

geemus commented Aug 1, 2024

@ekohl thanks for taking another look. I rebased and force pushed, which did in fact trigger CI, which passed. So I think we are (hopefully) in good shape now. I'll bring this in, just let me know if you'd like to work on any of the other libraries on related things, otherwise I suspect someone will get to it in time (and/or the upcoming Rubies will force the point early next year probably).

@geemus geemus merged commit d60548a into fog:master Aug 1, 2024
5 checks passed
@chaadow
Copy link
Contributor Author

chaadow commented Aug 1, 2024

@geemus Hey, sorry i was on vacation, thanks for merging. I will take a look on the other related PRs that I've opened this weekend probably. Thanks again for your time!

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.

4 participants