Fixing ruby 2.7 warnings in third party gems

2020-05-17

Table of contents

I’m working on a Make or Break project, and it depends on some gems:

1
2
3
4
5
6
7
8
9
10
11
12
gem "activesupport"
gem "aws-sdk-s3"
gem "aws-sdk-sqs"
gem "dotenv"
gem "foreman"
gem "httparty"
gem "minitest"
gem "rack-test"
gem "rake"
gem "rlua", git: "https://github.com/whitequark/rlua", branch: "master"
gem "sinatra"
gem "zeitwerk"

I was using ruby 2.6 at the time, but since 2.7 is out, I bumped it. This led to some warnings when running tests:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/plugins/expect_100_continue.rb:16: warning: `if' at the end of line without an expression
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/plugins/http_200_errors.rb:30: warning: character class has duplicated range: /<[\w_]/

[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/resource.rb:75: warning: assigned but unused variable - resp
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/bucket.rb:523: warning: assigned but unused variable - resp
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/multipart_upload.rb:277: warning: assigned but unused variable - resp

[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/bucket.rb:9: warning: method redefined; discarding old initialize
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/bucket.rb:20: warning: previous definition of initialize was here

[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/object.rb:62: warning: method redefined; discarding old copy_from
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/object.rb:662: warning: previous definition of copy_from was here

[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/object_summary.rb:11: warning: method redefined; discarding old copy_from
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/object_summary.rb:413: warning: previous definition of copy_from was here

aws-sdk-ruby warnings

Small manual fixes

I checked the first file, to see if this was something hard to fix:

1
2
3
4
5
6
7
8
9
def call(context)
  if
    context.http_request.body &&
    context.http_request.body.size > 0
  then
    context.http_request.headers['expect'] = '100-continue'
  end
  @handler.call(context)
end

It seems like this one is easy. There are some “unused variable” warnings and “duplicated range” that should be easy to fix as well.

Before starting to mess with code, I checked the project’s master branch, issues and pull requests to see if someone had already mentioned or fixed some of these. I found nothing.

The first error was an easy fix. I changed that if statement to:

1
2
3
4
5
6
def call(context)
  if context.http_request.body && context.http_request.body.size > 0
    context.http_request.headers['expect'] = '100-continue'
  end
  @handler.call(context)
end

The second error was complaining that [\w_] was redundant. \w is equivalent to [a-zA-Z0-9_], so the underscore is already included. We can change this as well:

1
2
3
4
5
# from
!xml.match(/<[\w_]/)

# to
!xml.match(/<\w/)

The following three warnings were all the same: assigned but unused variable - resp. These came from code like this:

1
2
3
4
5
6
7
def create_bucket(options = {})
  resp = @client.create_bucket(options)
  Bucket.new(
    name: options[:bucket],
    client: @client
  )
end

I just removed the resp. Maybe here they’ll want to do something with resp, but if I end up creating a pull request, they’ll let me know.

Note: I’m about to find out that I shouldn’t be editing these files, and resp will start to make sense.

The following three warnings (six lines) are probably going to be a bit harder to fix. Let’s look at the first one:

1
2
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/bucket.rb:9: warning: method redefined; discarding old initialize
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/bucket.rb:20: warning: previous definition of initialize was here

From just looking at the errors, it seems like bucket.rb is defining an initializer and there’s an override, through a customizations mechanism. This might be because there’s some code generation going on. Not sure.

Yep. I was manually editing files that are automatically generated. Checking the header of aws-sdk-s3-1.64.0/lib/aws-sdk-s3/resource.rb:

1
2
3
4
5
6
# WARNING ABOUT GENERATED CODE
#
# This file is generated. See the contributing guide for more information:
# https://github.com/aws/aws-sdk-ruby/blob/master/CONTRIBUTING.md
#
# WARNING ABOUT GENERATED CODE

This explains a lot. So, most of the files here are generated by generic build tools, and their behavior gets overriden by the customizations/ directory.

This explains why the resp variable was being set. It was potentially generated by code that did not know in advance if the response was going to be used or not.

Unused variables in code generator

Let’s check the code generator. First, let’s see if I can find any information on how to run it.

1
2
3
4
5
6
7
8
9
10
11
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ ack build_tools --ignore-file=is:CHANGELOG.md
tasks/test.rake
24:  sh('bundle exec rspec build_tools/aws-sdk-code-generator/spec')

CONTRIBUTING.md
13:* Errors in the code generators. These are defined in [build_tools](https://github.com/aws/aws-sdk-ruby/blob/master/build_tools).

Rakefile
5:$:.unshift("#{$REPO_ROOT}/build_tools")
6:$:.unshift("#{$REPO_ROOT}/build_tools/aws-sdk-code-generator/lib")
12:require 'build_tools'

So there may be a rake task that handles this.

1
2
3
4
5
6
7
8
9
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ rake -T
rake build            # Generates the code for every service
rake build:aws-sdk-*  # Generates the code for one service, e.g
rake gems             # Builds service gems
rake test             # Runs unit and integration tests after rebuilding gems
rake test:features    # Executes integration tests
rake test:spec        # Execute spec tests
rake test:spec:*      # Executes specs for a single gem, e.g
rake test:spec:each   # Executes every spec file, one at a time

Let me try running the build task for aws-sdk-s3:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ bundle
[snip]
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ bundle exec rake build:aws-sdk-s3
replace gems/aws-sdk-s3/aws-sdk-s3.gemspec
replace gems/aws-sdk-s3/features/env.rb
skip gems/aws-sdk-s3/features/step_definitions.rb
replace gems/aws-sdk-s3/spec/spec_helper.rb
replace gems/aws-sdk-s3/features/smoke.feature
replace gems/aws-sdk-s3/features/smoke_step_definitions.rb
skip gems/aws-sdk-s3/VERSION
replace gems/aws-sdk-s3/lib/aws-sdk-s3.rb
skip gems/aws-sdk-s3/lib/aws-sdk-s3/customizations.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/types.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/event_streams.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/client_api.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/client.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/errors.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/waiters.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/resource.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_acl.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_cors.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_lifecycle.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_lifecycle_configuration.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_logging.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_notification.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_policy.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_request_payment.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_tagging.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_versioning.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/bucket_website.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_upload.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_upload_part.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/object.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/object_acl.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/object_summary.rb
replace gems/aws-sdk-s3/lib/aws-sdk-s3/object_version.rb

Running this did undo my changes to resource.rb, bucket.rb, and multipart_upload.rb.

After grepping for “resp =” and “@client” inside build_tools, I found AwsSdkCodeGenerator::ResourceClientRequest:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
# @option options [required, Hash] :request
# @option options [Boolean] :resp (false)
# @option options [Boolean] :merge (true)
def build(options)
  request = options.fetch(:request)
  merge = options.fetch(:merge, true)
  streaming = options.fetch(:streaming, false)
  params = ResourceClientRequestParams.new(params: request['params'])
  parts = []
  parts << request_options(params) if merge
  parts << assignment(options)
  parts << "@client."
  parts << operation_name(request)
  parts << arguments(merge, params, streaming)
  parts.join
end

def assignment(options)
  if options.fetch(:resp, false)
    'resp = '
  else
    ''
  end
end

It seems like this could be related to the code I’m looking for. It’s building a resource request, with an optional resp = .

Looking around for what might be passing this option, I found this:

1
2
3
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ ack 'resp:'
build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/resource_action_code.rb
31:      ResourceClientRequest.build(request: @request, resp: true, streaming: @streaming)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
module AwsSdkCodeGenerator
  class ResourceActionCode

    def initialize(options)
      action = options.fetch(:action)
      @request = action.fetch('request')
      @resource = action.fetch('resource', nil)
      @streaming = options.fetch(:streaming, false)
    end

    def build
      parts = []
      if batch_action?
        parts << 'batch = []'
        parts << client_request
        parts << ResourceBatchBuilder.new(resource: @resource).build
        parts << "#{resource_type}::Collection.new([batch], size: batch.size)"
      elsif resource_action?
        parts << client_request
        parts << ResourceBuilder.new(resource: @resource, request_made: true).build
      else
        parts << client_request
        parts << 'resp.data'
      end
      parts.join("\n").rstrip
    end

    private

    def client_request
      ResourceClientRequest.build(request: @request, resp: true, streaming: @streaming)
    end

    # ...
  end
end

Let me check if this is the code I’m supposed to be looking at. I’m going to add a random part and see what happens when I run the rake task again.

It worked. I changed this:

1
2
3
4
5
6
7
8
9
10
# from
elsif resource_action?
  parts << client_request
  parts << ResourceBuilder.new(resource: @resource, request_made: true).build

# to
elsif resource_action?
  parts << client_request
  parts << ResourceBuilder.new(resource: @resource, request_made: true).build
  parts << "hello"

And now bucket.rb:523 looks like this:

1
2
3
4
5
6
7
8
9
10
def put_object(options = {})
  options = options.merge(bucket: @name)
  resp = @client.put_object(options)
  Object.new(
    bucket_name: @name,
    key: options[:key],
    client: @client
  )
  hello
end

So this means that client_request generates resp = @client.put_object(options) and ResourceBuilder.new.build generates the Object.new statement.

client_request is used in all three branches: batch, resource, and other. I checked a batch example (multipart.rb#parts) and it does use resp. the else clause also uses resp.

Maybe resp: true should not be set in the resource_action? clause. Let m e check the ResourceBuilder code to see if it can potentially generate any code using resp.

Checked build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/resource_builder.rb, and I saw no references to resp. Let’s try this. I’ll make the following change:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
# from
def build
  parts = []
  if batch_action?
    parts << 'batch = []'
    parts << client_request
    parts << ResourceBatchBuilder.new(resource: @resource).build
    parts << "#{resource_type}::Collection.new([batch], size: batch.size)"
  elsif resource_action?
    parts << client_request
    parts << ResourceBuilder.new(resource: @resource, request_made: true).build
  else
    parts << client_request
    parts << 'resp.data'
  end
  parts.join("\n").rstrip
end

private

def client_request
  ResourceClientRequest.build(request: @request, resp: true, streaming: @streaming)
end

# to
def build
  parts = []
  if batch_action?
    parts << 'batch = []'
    parts << client_request
    parts << ResourceBatchBuilder.new(resource: @resource).build
    parts << "#{resource_type}::Collection.new([batch], size: batch.size)"
  elsif resource_action?
    parts << client_request(false)
    parts << ResourceBuilder.new(resource: @resource, request_made: true).build
  else
    parts << client_request
    parts << 'resp.data'
  end
  parts.join("\n").rstrip
end

private

def client_request(resp = true)
  ResourceClientRequest.build(request: @request, resp: resp, streaming: @streaming)
end

Running the rake task again and going a git diff showed me that it removed five resps, when it should only have removed three. Oops.

The two extra ones where in object.rb and object_summary.rb, both in the method initiate_multipart_upload:

1
2
3
4
5
6
7
8
9
10
11
12
13
def initiate_multipart_upload(options = {})
  options = options.merge(
    bucket: @bucket_name,
    key: @key
  )
  @client.create_multipart_upload(options)
  MultipartUpload.new(
    bucket_name: @bucket_name,
    object_key: @key,
    id: resp.data.upload_id,
    client: @client
  )
end

Where did that come from? Maybe I didn’t check resource_builder.rb thoroughly. Let me look again:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
module AwsSdkCodeGenerator
  class ResourceBuilder

    # @option options [required, Hash] :resource
    # @option options [required, Boolean] :request_made
    def initialize(options = {})
      @resource = options.fetch(:resource)
      @request_made = options.fetch(:request_made)
    end

    def build
      "#{resource_class}.new(#{constructor_options})"
    end

    private

    def resource_class
      @resource['type']
    end

    def constructor_options
      options = {}
      options.update(identifiers)
      options[:data] = data_path if @resource['path']
      options[:client] = "@client"
      HashFormatter.new(wrap:false).format(options)
    end

    def identifiers
      (@resource['identifiers'] || []).inject({}) do |hash, identifier|
        value = ResourceValueSource.new(identifier).to_s
        hash[Underscore.underscore(identifier['target']).to_sym] = value
        hash
      end
    end

    def data_path
      if @request_made
        ResourceValueSource.new('source' => 'response', 'path' => @resource['path'])
      else
        ResourceValueSource.new('source' => 'data', 'path' => @resource['path'])
      end
    end

  end
end

So we have an id option in the constructor, which seems to be generated from constructor_options. The id option probably comes from the identifiers method, which is generated based on @resource['identifiers']. Does ResourceValueSource.new(identifier).to_s always generate something based on resp?

Looking at build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/resource_value_source.rb.

This class inherits from String, which sounds super fun. It references resp.data in the param_response method, which seems to be called when the passed identifier has "source" => "response". Let’s use this knowledge to change the way we call client_request:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
def build
  parts = []
  if batch_action?
    parts << 'batch = []'
    parts << client_request
    parts << ResourceBatchBuilder.new(resource: @resource).build
    parts << "#{resource_type}::Collection.new([batch], size: batch.size)"
  elsif resource_action?
    parts << client_request(@resource['identifiers'].any? { |i| i['source'] == 'response' })
    parts << ResourceBuilder.new(resource: @resource, request_made: true).build
  else
    parts << client_request
    parts << 'resp.data'
  end
  parts.join("\n").rstrip
end

Running the rake task again restored those two stray resp = removals. I am only running this for aws-sdk-s3, I’m not sure if this won’t remove any resps in other gems. Let me try with sqs:

1
2
3
4
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ bundle exec rake build:aws-sdk-sqs
[...]
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ git diff gems/aws-sdk-sqs/
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$

This produced no changes. I’ll take this as a good sign. Let’s run the full thing:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ bundle exec rake build
[...]
[... taking a while ...]
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ git status -s
M build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/resource_action_code.rb
M gems/aws-sdk-autoscaling/lib/aws-sdk-autoscaling/auto_scaling_group.rb
M gems/aws-sdk-autoscaling/lib/aws-sdk-autoscaling/resource.rb
M gems/aws-sdk-cloudformation/lib/aws-sdk-cloudformation/resource.rb
M gems/aws-sdk-cloudwatch/lib/aws-sdk-cloudwatch/metric.rb
M gems/aws-sdk-dynamodb/lib/aws-sdk-dynamodb/table.rb
M gems/aws-sdk-ec2/lib/aws-sdk-ec2/resource.rb
M gems/aws-sdk-ec2/lib/aws-sdk-ec2/route_table.rb
M gems/aws-sdk-glacier/lib/aws-sdk-glacier/account.rb
M gems/aws-sdk-glacier/lib/aws-sdk-glacier/resource.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/group.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/resource.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/server_certificate.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/user.rb
M gems/aws-sdk-rds/lib/aws-sdk-rds/resource.rb
M gems/aws-sdk-s3/aws-sdk-s3.gemspec
M gems/aws-sdk-s3/lib/aws-sdk-s3/bucket.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_upload.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/expect_100_continue.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/http_200_errors.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/resource.rb

Let’s see if everything is correct.

It isn’t. It removed some data entries that are still being used. For example:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ git diff -U5 gems/aws-sdk-dynamodb/lib/aws-sdk-dynamodb/table.rb
diff --git a/gems/aws-sdk-dynamodb/lib/aws-sdk-dynamodb/table.rb b/gems/aws-sdk-dynamodb/lib/aws-sdk-dynamodb/table.rb
index be9d4a8cf..8dacfa214 100644
--- a/gems/aws-sdk-dynamodb/lib/aws-sdk-dynamodb/table.rb
+++ b/gems/aws-sdk-dynamodb/lib/aws-sdk-dynamodb/table.rb
@@ -1883,11 +1883,11 @@ module Aws::DynamoDB
     #
     #   [1]: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/globaltables.V2.html
     # @return [Table]
     def update(options = {})
       options = options.merge(table_name: @name)
-      resp = @client.update_table(options)
+      @client.update_table(options)
       Table.new(
         name: @name,
         data: resp.data.table_description,
         client: @client
       )

I fixed the identifiers conditional, but the data key also uses it. This is easier to fix, since I now understand how ResourceValueSource works, and I know where data: comes from:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
# resource_builder.rb:
def constructor_options
  options = {}
  options.update(identifiers)
  options[:data] = data_path if @resource['path']
  options[:client] = "@client"
  HashFormatter.new(wrap:false).format(options)
end

# ...
def data_path
  if @request_made
    ResourceValueSource.new('source' => 'response', 'path' => @resource['path'])
  else
    ResourceValueSource.new('source' => 'data', 'path' => @resource['path'])
  end
end

So I’ll tweak the boolean a bit more:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
def build
  parts = []
  if batch_action?
    parts << 'batch = []'
    parts << client_request
    parts << ResourceBatchBuilder.new(resource: @resource).build
    parts << "#{resource_type}::Collection.new([batch], size: batch.size)"
  elsif resource_action?
    parts << client_request(
      @resource['identifiers'].any? { |i| i['source'] == 'response' } || @resource['path']
    )
    parts << ResourceBuilder.new(resource: @resource, request_made: true).build
  else
    parts << client_request
    parts << 'resp.data'
  end
  parts.join("\n").rstrip
end

Running the full rake task again:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ git status -s
M build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/resource_action_code.rb
M gems/aws-sdk-autoscaling/lib/aws-sdk-autoscaling/auto_scaling_group.rb
M gems/aws-sdk-autoscaling/lib/aws-sdk-autoscaling/resource.rb
M gems/aws-sdk-cloudformation/lib/aws-sdk-cloudformation/resource.rb
M gems/aws-sdk-cloudwatch/lib/aws-sdk-cloudwatch/metric.rb
M gems/aws-sdk-ec2/lib/aws-sdk-ec2/resource.rb
M gems/aws-sdk-ec2/lib/aws-sdk-ec2/route_table.rb
M gems/aws-sdk-glacier/lib/aws-sdk-glacier/account.rb
M gems/aws-sdk-glacier/lib/aws-sdk-glacier/resource.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/group.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/resource.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/server_certificate.rb
M gems/aws-sdk-iam/lib/aws-sdk-iam/user.rb
M gems/aws-sdk-s3/aws-sdk-s3.gemspec
M gems/aws-sdk-s3/lib/aws-sdk-s3/bucket.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/multipart_upload.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/expect_100_continue.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/plugins/http_200_errors.rb
M gems/aws-sdk-s3/lib/aws-sdk-s3/resource.rb

Some of the changed files were restored, and I didn’t see anything weird when looking through git diff -U5. Looks good.

Redefinition warnings in generated code overrides

The three warnings left point to the customizations directory redefining methods created by the code generator:

1
2
3
4
5
6
7
8
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/bucket.rb:9: warning: method redefined; discarding old initialize
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/bucket.rb:20: warning: previous definition of initialize was here

[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/object.rb:62: warning: method redefined; discarding old copy_from
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/object.rb:662: warning: previous definition of copy_from was here

[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/customizations/object_summary.rb:11: warning: method redefined; discarding old copy_from
[...]/aws-sdk-s3-1.64.0/lib/aws-sdk-s3/object_summary.rb:413: warning: previous definition of copy_from was here

I’m guessing this customizations mechanism works by reopening the class, and it’s being required after the generated code is required. Let’s see how the mechanism works, in the bucket.rb case:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
# gems/aws-sdk-s3/lib/aws-sdk-s3.rb`

# [...]
require_relative 'aws-sdk-s3/bucket'
# [... more requires ...]
require_relative 'aws-sdk-s3/customizations'
require_relative 'aws-sdk-s3/event_streams'


# gems/aws-sdk-s3/lib/aws-sdk-s3/customizations.rb

# [...]

require 'aws-sdk-s3/customizations/bucket'
require 'aws-sdk-s3/customizations/object'
require 'aws-sdk-s3/customizations/object_summary'
require 'aws-sdk-s3/customizations/multipart_upload'
require 'aws-sdk-s3/customizations/types/list_object_versions_output'


# gems/aws-sdk-s3/lib/aws-sdk-s3/customizations/bucket.rb`

require 'uri'

module Aws
  module S3
    class Bucket
      # Save the old initialize method so that we can call 'super'.
      old_initialize = instance_method(:initialize)
      # Define a new initialize method that extracts out a bucket ARN.
      define_method(:initialize) do |*args|
        old_initialize.bind(self).call(*args)
        bucket_name, region, arn = Plugins::BucketARN.resolve_arn!(
          name,
          client.config.region,
          client.config.s3_use_arn_region
        )
        @name = bucket_name
        @client.config.region = region
        @arn = arn
      end

      [...]
    end
  end
end

So aws-sdk-s3 starts by loading the generated class, aws-sdk-s3/bucket.rb and then loads its customization, which calls define_method(:initialize), with initialize already defined. It does grab the previously existing method in old_initialize = instance_method(:initialize) so that it can call it in the redefinition.

I searched if there is a redefine_method I could use, and rails does have a helper method for this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
def redefine_method(method, &block)
  visibility = method_visibility(method)
  silence_redefinition_of_method(method)
  define_method(method, &block)
  send(visibility, method)
end

def silence_redefinition_of_method(method)
  if method_defined?(method) || private_method_defined?(method)
    # This suppresses the "method redefined" warning; the self-alias
    # looks odd, but means we don't need to generate a unique name
    alias_method method, method
  end
end

So calling alias_method :initialize, :initialize before the define_method call should silence the warning. That’s kind of weird.

What’s another way of doing this? I could use prepend, but I bet that the project needs to support older versions. prepend came out in 2.0.0. That’s quite old. Let’s check .travis.yml:

1
2
3
4
5
6
7
8
9
10
11
12
rvm:
  - 1.9.3
  - 2.0.0
  - 2.1
  - 2.2
  - 2.3
  - 2.4
  - 2.5
  - 2.6
  - 2.7
  - jruby-1.7.27
  - jruby

Of course they need to support 1.9.3. I’ll add the alias_method trick and see if it works.

Ensuring everything is fine

Before I make the pull request, I want to make sure that the warnings go away, and I probably want to run the test suite (although I might let travis handle that part). I’ll run the build task again, and change my project to use this version instead of the rubygems.org version:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
hugopeixoto@laptop:~/w/m/ai-competition$ git diff gems.rb
diff --git a/gems.rb b/gems.rb
index 7ad8d16..dc69e9e 100644
--- a/gems.rb
+++ b/gems.rb
@@ -1,7 +1,7 @@
 source "https://rubygems.org"

 gem "activesupport"
-gem "aws-sdk-s3"
+gem "aws-sdk-s3", path: "/home/hugopeixoto/work/contrib/aws-sdk-ruby/gems/aws-sdk-s3"
 gem "aws-sdk-sqs"
 gem "byebug"
 gem "dotenv"

hugopeixoto@laptop:~/w/m/ai-competition$ bundle update
Fetching https://github.com/whitequark/rlua
Fetching gem metadata from https://rubygems.org/...........
Resolving dependencies...
Bundler could not find compatible versions for gem "aws-sdk-core":
  In gems.rb:
    aws-sdk-s3 was resolved to 1.64.0, which depends on
      aws-sdk-core (~> 3, >= 3.96.1)

    aws-sdk-sqs was resolved to 1.25.0, which depends on
      aws-sdk-core (~> 3, >= 3.71.0)

Could not find gem 'aws-sdk-core (~> 3, >= 3.96.1)', which is required by gem 'aws-sdk-s3', in any of the sources.

Well, that was unexpected. It seems that by running the generator on aws-sdk-s3, I bumped the aws-sdk-core version to an unreleased version:

1
2
3
4
5
6
7
8
9
10
11
12
13
hugopeixoto@laptop:~/w/c/aws-sdk-ruby$ git diff gems/aws-sdk-s3/aws-sdk-s3.gemspec
diff --git a/gems/aws-sdk-s3/aws-sdk-s3.gemspec b/gems/aws-sdk-s3/aws-sdk-s3.gemspec
index cd7652f65..fa34b5130 100644
--- a/gems/aws-sdk-s3/aws-sdk-s3.gemspec
+++ b/gems/aws-sdk-s3/aws-sdk-s3.gemspec
@@ -25,6 +25,6 @@ Gem::Specification.new do |spec|

   spec.add_dependency('aws-sdk-kms', '~> 1')
   spec.add_dependency('aws-sigv4', '~> 1.1')
-  spec.add_dependency('aws-sdk-core', '~> 3', '>= 3.83.0')
+  spec.add_dependency('aws-sdk-core', '~> 3', '>= 3.96.1')

 end

I am assuming that using 3.96.0 or 3.96.1 won’t make that much of a difference for the things I changed, so I changed the gemspec manually to .0 and I was able to run the tests:

1
2
3
4
5
6
7
8
9
10
hugopeixoto@laptop:~/w/m/ai-competition$ bundle exec rake
Run options: --seed 19466

# Running:

..................................

Finished in 0.477091s, 71.2653 runs/s, 222.1800 assertions/s.

34 runs, 106 assertions, 0 failures, 0 errors, 0 skips

Looks clean. I’ll run the full build one last time, to make sure that there are no manual files changed, and make a pull request.

I decided to run the tests, and got a failure:

1
2
3
4
5
6
7
8
9
10
11
12
13
1) ensure no hard-coded region /home/hugopeixoto/work/contrib/aws-sdk-ruby/gems/aws-sdk-s3/lib/aws-sdk-s3/customizations/bucket.rb has no hard-coded region
   Failure/Error: expect(val).not_to match(/(us|eu|ap|sa|ca)-[a-zA-Z]+-\d+/)

     expected "        if (client.config.region == 'us-east-1') &&\n" not to match /(us|eu|ap|sa|ca)-[a-zA-Z]+-\d+/
     Diff:
     @@ -1,2 +1,2 @@
     -/(us|eu|ap|sa|ca)-[a-zA-Z]+-\d+/
     +        if (client.config.region == 'us-east-1') &&

   # ./build_tools/spec/region_spec.rb:48:in `block (5 levels) in <top (required)>'
   # ./build_tools/spec/region_spec.rb:38:in `each'
   # ./build_tools/spec/region_spec.rb:38:in `each_with_index'
   # ./build_tools/spec/region_spec.rb:38:in `block (4 levels) in <top (required)>'

Checking build_tools/spec/region_spec.rb, there’s a list of file/line pairs that can contain a hardcoded region. Since I changed that file, I need to adjust the line number.

Adjusting the line number made the basic test suite pass, but I had a bunch of cucumber errors, and it was taking forever. I’ll let travis handle this.

Here’s the pull request:

https://github.com/aws/aws-sdk-ruby/pull/2308

Sinatra

I had warnings in sinatra as well:

1
2
3
4
5
6
7
hugopeixoto@zephos:~/w/m/ai-competition$ bundle exec bin/web
/home/hugopeixoto/work/contrib/asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/sinatra-2.0.8.1/lib/sinatra/base.rb:1526: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/hugopeixoto/work/contrib/asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/rack-2.2.2/lib/rack/handler/webrick.rb:26: warning: The called method `run' is defined here
[2020-05-17 19:52:27] INFO  WEBrick 1.6.0
[2020-05-17 19:52:27] INFO  ruby 2.7.0 (2019-12-25) [x86_64-linux]
== Sinatra (v2.0.8.1) has taken the stage on 4567 for development with backup from WEBrick
[2020-05-17 19:52:27] INFO  WEBrick::HTTPServer#start: pid=1278442 port=4567

Looking through the github issues, these seem to have been addressed already, but not released:

https://github.com/sinatra/sinatra/issues/1590

Summary

I upgraded a personal project to ruby 2.7, and got some warnings in a third party gem: aws-sdk-s3.

Some of those warnings came from generated code, but it was still fixable.

I’m not sure if they’ll accept my PR, but there’s no reason not to do it. If you’re not sure you’re doing things right, they’ll let you know.