Skip to content

Conversation

@johnboiles
Copy link
Contributor

@johnboiles johnboiles commented Jun 13, 2019

See THRIFT-4797 for details. In short, Golang compilation fails when a Thrift file includes two files that have the same final namespace component (e.g. namespace go packagea.common and namespace go packageb.common)

Somewhat based on fbthrift's implementation though their implementation simply always adds an alias with a unique numerical prefix.

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? (not required for trivial changes)
  • Did you squash your changes to a single commit?
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"

@johnboiles johnboiles force-pushed the golang-fix-import-collisions branch 2 times, most recently from b19d8f1 to 2f87ca9 Compare June 13, 2019 23:29
@johnboiles
Copy link
Contributor Author

Just found another case to handle here. I should also detect collisions with the golang packages that get imported like bytes context reflect

@dcelasun
Copy link
Member

For stdlib collisions like context etc, it would be great if the import list was a vector instead of a hardcoded string so we could reuse it everywhere.

@johnboiles
Copy link
Contributor Author

@dcelasun I made it use a vector for system imports can you take a look? I’ll squash it together if/when it looks good to you!

@dcelasun
Copy link
Member

Looks good!

@johnboiles johnboiles force-pushed the golang-fix-import-collisions branch from a2bd8b0 to 47685dc Compare June 27, 2019 17:46
@johnboiles
Copy link
Contributor Author

@dcelasun squashed and ready to merge!

@johnboiles
Copy link
Contributor Author

Actually hold on merging for a bit. I may have found another edge case

@johnboiles
Copy link
Contributor Author

johnboiles commented Jun 28, 2019

Yep, confirmed. If the package has a service, and that package’s last namespace component conflicts with a previously imported package, it won’t get aliased. Hopefully I can get a chance to fix tomorrow

@johnboiles
Copy link
Contributor Author

@dcelasun one more commit to review to fix the issue with the remote package import collisions! Hopefully this is it

@johnboiles johnboiles force-pushed the golang-fix-import-collisions branch from e940bfa to fddab5d Compare June 29, 2019 00:22
@dcelasun
Copy link
Member

LGTM. Tried testing a few different scenarios locally, everything works fine. Thanks for the PR!

@dcelasun dcelasun merged commit d9019fc into apache:master Jun 29, 2019
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.

2 participants