Skip to content

Conversation

davidgm0
Copy link

@davidgm0 davidgm0 commented Jun 4, 2025

Partially addresses #639. It runs tests with frozen strings, but allows modifying strings for those files with frozen_string_literal: false. This should allow using fog-google in projects that use RUBYOPT="--enable-frozen-string-literal" without errors. Next step would involve removing the strings modifications. This is more involved, so I thought that it would make sense to do this first.

@dgmora
Copy link

dgmora commented Jun 4, 2025

I gave this a try and now I am not getting FrozenError: can't modify frozen String: "/"

@davidgm0
Copy link
Author

@Temikus Is this something you could take a look at?

@@ -36,6 +36,8 @@ concurrency:
jobs:
test:
runs-on: fog-arc-runner
env:
RUBYOPT: "--enable-frozen-string-literal"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we expect these tests to fail without fixes for the frozen strings?

Copy link
Author

@davidgm0 davidgm0 Aug 27, 2025

Choose a reason for hiding this comment

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

To those files that modify strings I added:

# frozen_string_literal: false

That overrides the RUBYOPT for those files. The final behaviour should be that by default for all files the strings are frozen and for those few files the string literals are not frozen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, that's unexpected, though; usually I see # frozen_string_literal: true, or it's omitted entirely because false is the default.

I'm not opposed to this approach as a step 1. However, at the moment these integration tests need to be run by hand. I suspect just fixing the frozen string issue is relatively straightforward that we might as well try to fix them on a file-by-file basis, such as:

diff --git a/lib/fog/google/storage/storage_json/real.rb b/lib/fog/google/storage/storage_json/real.rb
index 23c86c5..ab99372 100644
--- a/lib/fog/google/storage/storage_json/real.rb
+++ b/lib/fog/google/storage/storage_json/real.rb
@@ -45,7 +45,7 @@ DATA
           end
           string_to_sign << canonical_google_headers.to_s
 
-          canonical_resource = "/"
+          canonical_resource = +"/"
           if subdomain = params.delete(:subdomain)
             canonical_resource << "#{CGI.escape(subdomain).downcase}/"
           end
diff --git a/lib/fog/google/storage/storage_xml/real.rb b/lib/fog/google/storage/storage_xml/real.rb
index 166e181..5c15bfc 100644
--- a/lib/fog/google/storage/storage_xml/real.rb
+++ b/lib/fog/google/storage/storage_xml/real.rb
@@ -57,7 +57,7 @@ DATA
           end
           string_to_sign << canonical_google_headers.to_s
 
-          canonical_resource = "/"
+          canonical_resource = +"/"
           if subdomain = params.delete(:subdomain)
             canonical_resource << "#{CGI.escape(subdomain).downcase}/"
           end

Copy link

Choose a reason for hiding this comment

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

I'd be happy if these changes could be applied directly, I just don't know the codebase so well and I saw there were a fair amount of modifications, so I went for the easy fix. That can still be applied after this PR.

Oh, right, that's unexpected, though; usually I see # frozen_string_literal: true, or it's omitted entirely because false is the default.

Yeah, but that default will eventually change. If someone tries to already enable --enable-frozen-string-literal (as I did), they will get errors. This PR at least fixes that

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, using +"..." was how we solved this for excon and it worked well (though it is a bit annoying initially).

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