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

The gem name provided to the gapic-generator-ruby has to match the package name specified in protos otherwise the VERSION constant is incorrectly referenced in the generated code #325

Closed
viacheslav-rostovtsev opened this issue Jan 28, 2020 · 3 comments · Fixed by #341
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@viacheslav-rostovtsev
Copy link
Member

viacheslav-rostovtsev commented Jan 28, 2020

previously called

[Showcase] gapic-generator-cloud docker incorrectly places the Showcase::VERSION constant when generating showcase

the description on how to reproduce the bug at the end of this comment still applies but the issue is slightly wider.

The placement in the directory structure and modules generated for the version.rb file are governed by the gem name parameter supplied via the <service>.yml file in the CI tests or via the --ruby-cloud-gem-name parameter when generating with the docker image. ( See also #323 ).

The placement of the service-level files is governed by the package name parameter, derived from the protobuf file, from the package parameter (or ruby_package option if provided).

The gem name must partially map to the package name, e.g. for showcase gem name of google-showcase maps to the package name from package google.showcase.v1beta1 or for the grpc_service_config testing the gem name of testing-grpc_service_config will map to the package name from option ruby_package = "Testing::GrpcServiceConfig";. The mapping can be partial (excluding version).

If the mapping is wrong, like in the example below where we just give showcase as the gem name -- the version.rb file ends up in the wrong folder and the tests fail, unhelpfully.


Example of how to repro:

when generating showcase:

$ docker run --rm --user $UID \
  --mount type=bind,source=`pwd`/google/showcase/v1beta1,destination=/in/google/showcase/v1beta1,readonly \
  --mount type=bind,source=`pwd`/showcase-ruby,destination=/out \
  gcr.io/gapic-images/gapic-generator-ruby:latest --ruby-cloud-gem-name=showcase

if we run tests with

bundle exec rake ci

we get an error like this for every testing method:

Error:
Google::Showcase::V1beta1::Messaging::ClientTest#test_delete_room:                 
NameError: uninitialized constant Google::Showcase::VERSION                        
    /usr/local/google/home/virost/src/testplay/showcase-ruby/lib/google/showcase/v1beta1/messaging/client.rb:330:in `delete_room'                                      
    /usr/local/google/home/virost/src/testplay/showcase-ruby/test/google/showcase/v1beta1/messaging_test.rb:248:in `block in test_delete_room'            

There is a VERSION constant in generated code:

module Showcase
  VERSION = "0.0.1"
end

but it's in a file /lib/showcase/version.rb while the rest of the code is under lib/google/showcase.

@viacheslav-rostovtsev viacheslav-rostovtsev changed the title gapic-generator-cloud docker omits VERSION constant when generating showcase gapic-generator-cloud docker omits or incorrectly places the Showcase::VERSION constant when generating showcase Jan 28, 2020
@viacheslav-rostovtsev viacheslav-rostovtsev changed the title gapic-generator-cloud docker omits or incorrectly places the Showcase::VERSION constant when generating showcase [Showcase] gapic-generator-cloud docker incorrectly places the Showcase::VERSION constant when generating showcase Jan 28, 2020
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Jan 29, 2020
@dazuma dazuma added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Feb 3, 2020
@viacheslav-rostovtsev viacheslav-rostovtsev self-assigned this Feb 5, 2020
@dazuma
Copy link
Member

dazuma commented Feb 7, 2020

This is a good find. So:

  • I believe the generator is correct in using the gem name to determine the actual location of version.rb, by Ruby convention. Thus, if the gem name actually is showcase, then Showcase::VERSION in lib/showcase/version.rb is correct.
  • The gem name should usually but will not always map to the prefix of the package name. There will be exceptions related to branding concerns, etc.
  • Therefore, the generator needs to be updated so that the references of the VERSION constant (I believe used during generation of the X-Goog-API-Client header) use the gem-name-derived namespace rather than the package-name-implied namespace.

@blowmage
Copy link
Contributor

I agree that if the gem name is provided as showcase then the version constant should be defined as Showcase::VERSION in lib/showcase/version.rb.

@viacheslav-rostovtsev viacheslav-rostovtsev changed the title [Showcase] gapic-generator-cloud docker incorrectly places the Showcase::VERSION constant when generating showcase The gem name provided to the gapic-generator-ruby has to match the package name specified in protos Feb 11, 2020
@viacheslav-rostovtsev viacheslav-rostovtsev changed the title The gem name provided to the gapic-generator-ruby has to match the package name specified in protos The gem name provided to the gapic-generator-ruby has to match the package name specified in protos otherwise the VERSION constant is incorrectly referenced in the generated code Feb 11, 2020
@viacheslav-rostovtsev
Copy link
Member Author

viacheslav-rostovtsev commented Feb 11, 2020

I agree that if the gem name is provided as showcase then the version constant should be defined as Showcase::VERSION in lib/showcase/version.rb.

The good news is that is what is currently happening...

I've edited the issue title to correctly identify the problem; the placement of the constant seems to be correct, the references to it are not and that causes the breakage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants