fog / fog-aws

Module for the 'fog' gem to support Amazon Web Services http://aws.amazon.com/
MIT License
296 stars 353 forks source link

Fix frozen string literal issue for ruby 3.4.0 #709

Closed chaadow closed 3 weeks ago

chaadow commented 3 weeks ago

To prepare for ruby 3.4.0 and making strings frozen by default, I've stumbled upon this string mutation.

Let me know if you need anything ( I didn't add tests since I assume tests exist for this feature )

geemus commented 3 weeks ago

@chaadow Thanks for catching this and providing a potential fix. I think I might like a slightly different approach, commented above, happy to discuss further. Thanks!

chaadow commented 3 weeks ago

@geemus done, thanks for the review!

chaadow commented 3 weeks ago

I would have preferred to enable frozen string literal for the entire gem, but I see that the CI uses a shared one.

Here is an example PR that implements it for rails-i18n (using RUBYOPT) https://github.com/svenfuchs/rails-i18n/pull/1120/files

Maybe you'd be interested to implement it there?

geemus commented 3 weeks ago

I could definitely see how setting RUBYOPTS like that could be a good way to find out sooner rather than later where we will need to fix things. Do you know what happens with those options on presently? Is in warnings, errors, other?

It could make sense to directly update the shared stuff, or we could add separate things for fog-aws specifically around this potentially. As I type this, it feels like maybe just doing it here locally to start and work out any issues might be good (and then we could move it to the shared stuff once it was proven out a bit more). What do you think?

chaadow commented 3 weeks ago

I could definitely see how setting RUBYOPTS like that could be a good way to find out sooner rather than later where we will need to fix things. Do you know what happens with those options on presently? Is in warnings, errors, other?

short answer:

Back when frozen string literals were introduced ( in ruby 2.3.0 in dec. 2015 ) you could run an entire ruby script ( or a rails app ) with the --enable-frozen-string-literal flag Back then it was nearly and realistically impossible for any production app to enable this flag, as gems were not prepared.

However gems ( and gem maintainers), in isolation, could, in their CI only, make sure to run their unit tests with that flag, because probably soon ( with ruby 3.5 if i'm not mistaken ) it will be enabled by default.

Ruby 3.4 will start to emit deprecation warnings ( can already be tested with ruby 3.4.0-preview1 version) if the deprecation flag is enabled ( as described above)

==> here is a gist from the creator of zeitwerk and co-written by Jean Boussier, ruby commiter, and both rails core members that better explains the future of frozen string literals in ruby.

It could make sense to directly update the shared stuff, or we could add separate things for fog-aws specifically around this potentially. As I type this, it feels like maybe just doing it here locally to start and work out any issues might be good (and then we could move it to the shared stuff once it was proven out a bit more). What do you think?

Yes I agree, I think we can create a temporary .yml with the shared CI code + the RUBYOPTS part. ( we can test that it works by manually removing the +"" that i've added) and once the yml setup works, we can move it upstream to the shared CI, and create PRs for each gem of the fog ecosystem 👌🏼 @geemus

geemus commented 3 weeks ago

Thanks for the extra details, I hadn't been following this and had to read up on it around the original issue. Trying it out here before moving upstream sounds good to me. Do you have capacity to give that a try?

chaadow commented 2 weeks ago

@geemus Yes I do, I started a PR here #711