You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by taozle <gi...@git.apache.org> on 2017/07/17 16:44:23 UTC
[GitHub] thrift pull request #1309: Use build tags to support context.
GitHub user taozle opened a pull request:
https://github.com/apache/thrift/pull/1309
Use build tags to support context.
As the comment in https://issues.apache.org/jira/browse/THRIFT-4236, this PR use build tags to support context in go1.7 above.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/taozle/thrift context
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1309.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1309
----
commit 0df5965eb2a35793f19de1b2696e3c4afb6b5132
Author: taozle <zh...@gmail.com>
Date: 2017-07-17T16:40:42Z
Use build tags to support context.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r127775849
--- Diff: lib/go/thrift/simple_server2.go ---
@@ -1,180 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package thrift
-
-import (
- "context"
- "log"
- "runtime/debug"
- "sync"
-)
-
-/*
- * This is only a simple sample same as TSimpleServer but add context
- * usage support.
- */
-type TSimpleServer2 struct {
--- End diff --
Why was this removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on the issue:
https://github.com/apache/thrift/pull/1309
I will try to fix that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128154321
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
You do have an explicit dependency to the new `context` package [here](https://github.com/taozle/thrift/blob/0df5965eb2a35793f19de1b2696e3c4afb6b5132/compiler/cpp/src/thrift/generate/t_go_generator.cc#L887). What I'm suggesting is to use `x/net/context` there if it's Go<1.7, so you don't need `use_context` at all.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on the issue:
https://github.com/apache/thrift/pull/1309
Seems that the failed tests has nothing with this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1309
> I'll also be sending a PR to update the Go tutorial with the new handler signature.
Please do. Right now the tutorial does not build.
```
handler.go:30:2: cannot find package "golang.org/x/net/context" in any of:
C:\Go\src\golang.org\x\net\context (from $GOROOT)
D:\Workdir\src\golang.org\x\net\context (from $GOPATH)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128144782
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
i noticed that thrift has no dependency, so i just followed this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128159270
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
I don't think we should commit it. Right now, we don't have a `vendor` folder [in the library](https://github.com/apache/thrift/tree/master/lib/go). When people use `go get` (or other dependency tools) they'll get the `x/net/context` dependency automatically (if they are on Go<1.7).
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128144976
--- Diff: lib/go/thrift/simple_server2.go ---
@@ -1,180 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package thrift
-
-import (
- "context"
- "log"
- "runtime/debug"
- "sync"
-)
-
-/*
- * This is only a simple sample same as TSimpleServer but add context
- * usage support.
- */
-type TSimpleServer2 struct {
--- End diff --
This is just a sample copied from TSimpleServer with little difference, and it is hard to maintain two files with almost the same code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128158624
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
OK, i am glad to remove the `use_context`, but since then we have either to commit the `x/net/context` in vendor or keep this dep info using package manage tool (glide, govendor ...), i prefer commit the `x/net/context` into vendor since there is no widely adopted package manage tool by now, any advise?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128164607
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
Great, thanks for working on this!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
GitHub user taozle reopened a pull request:
https://github.com/apache/thrift/pull/1309
Use build tags to support context.
As the comment in https://issues.apache.org/jira/browse/THRIFT-4236, this PR use build tags to support context in go1.7 above.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/taozle/thrift context
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1309.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1309
----
commit 0df5965eb2a35793f19de1b2696e3c4afb6b5132
Author: taozle <zh...@gmail.com>
Date: 2017-07-17T16:40:42Z
Use build tags to support context.
commit e610d07370f55314fa674810cee4574006a15238
Author: taozle <zh...@gmail.com>
Date: 2017-07-19T15:47:35Z
Use x/net/context to support go<1.7.
commit 3d4eb9a967317cca7ea6f9e668e80fd06e0651ac
Author: taozle <zh...@gmail.com>
Date: 2017-07-19T16:13:59Z
Add legacy_context in go generator.
commit fdee27a80401107fc96c7aee7c2f8e14adaa425a
Author: taozle <zh...@gmail.com>
Date: 2017-07-19T16:37:29Z
Fix go test dependency.
commit 88d2fc00c2d8c0a3b6f48fbfea42b02fb7c6080c
Author: taozle <zh...@gmail.com>
Date: 2017-07-19T17:13:39Z
Fix GOPATH not set.
commit 8d052a6a97b295021277da29dbc87cf73deb3009
Author: taozle <zh...@gmail.com>
Date: 2017-07-20T02:51:11Z
Get x/net/context in lib/go.
commit acf418d6f6c0c75afd8ddaa6263f30e1987a6bfc
Author: taozle <zh...@gmail.com>
Date: 2017-07-20T06:13:26Z
Fix go tests.
commit 6451d1419747fb5bc011391872d1afdb6d1f3b1e
Author: taozle <zh...@gmail.com>
Date: 2017-07-20T08:39:36Z
Fix go tests in /lib/go/test/tests.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128145013
--- Diff: lib/go/thrift/multiplexed_processor.go ---
@@ -1,3 +1,5 @@
+// +build go1.7
--- End diff --
Agreed, i will fix it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128156419
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
Sorry for my poor expression, the *no dependency* i mean is that *no thirdparty dependency*. `use_context` can't be removed because adding context support breaks the backward compatibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1309
Ah, now I see it. The Makefile for the tutorial has the `legacy_context` flag hardcoded, since it's pinned to the Go version included in the Docker container. If you are running it outside the container (and hence with Go 1.7+) you need to remove `legacy_context` and `thirdparty-dep` from the Makefile.
@taozle Maybe there is a way to check the go version before adding the flag?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128159321
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
And its also painful to use build tags in the generated code, i think a control flag is still required to decided whether to use `context` or `x/net/context`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1309
go version go1.8.3 windows/amd64
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1309
Hmm, turns out @taozle already [updated](https://github.com/apache/thrift/blob/c0d384a38c2b43ee47cef86b1cd054e3f84dc909/tutorial/go/Makefile.am#L37) the Makefile for the tutorial, I just missed it somehow. The `check` target already includes `thirdparty-dep`, so it should work with `make -k check` when you are running Go<1.7. What were your exact steps that resulted in this error? What's your Go version?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r127774140
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
Can we support `x/net/context` for go<1.7?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128156875
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
Breaking BC can be ok (happened before, see my commits) but if we remove `use_context` we can make sure all RPCs will have the same signature starting with `(ctx *Context, ...`.
To be more clear: I think adding context support is *very* worthwhile and it's worth breaking BC for it (especially for a new release), but it would be better to support `context` for all Go versions, not just Go>1.7
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128160543
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
Sounds good, i will update this PR later today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1309
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on the issue:
https://github.com/apache/thrift/pull/1309
Yep, they are unrelated. Thanks for working on this!
@Jens-G what do you think? With this PR, we'll have `context` support for pre-1.7 Go versions as well.
I'll also be sending a PR to update the Go tutorial with the new handler signature.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by taozle <gi...@git.apache.org>.
Github user taozle closed the pull request at:
https://github.com/apache/thrift/pull/1309
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r127776518
--- Diff: lib/go/thrift/multiplexed_processor.go ---
@@ -1,3 +1,5 @@
+// +build go1.7
--- End diff --
Maybe this file should be named `multiplexed_protocol_go17.go` or something like that to be consistent with `TMultiplexedProcessor` in `multiplexed_protocol.go`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128159593
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3746,4 +3746,4 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
" use_context\n"
- " Make service method receive a context as first argument.\n")
+ " Make service method receive a context as first argument, only go1.7+ is supported.\n")
--- End diff --
> i think a control flag is still required to decided whether to use `context` or `x/net/context`.
Hmm, I think you are right. Maybe something like `legacy_context`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128301321
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -3745,5 +3720,5 @@ THRIFT_REGISTER_GENERATOR(go, "Go",
" Disable automatic spelling correction of initialisms (e.g. \"URL\")\n" \
" read_write_private\n"
" Make read/write methods private, default is public Read/Write\n" \
- " use_context\n"
- " Make service method receive a context as first argument.\n")
+ " legacy_context\n"
+ " Use lagacy x/net/context instead of context in go<1.7.\n")
--- End diff --
Typo s/lagacy/legacy
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
Posted by dcelasun <gi...@git.apache.org>.
Github user dcelasun commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1309#discussion_r128154458
--- Diff: lib/go/thrift/simple_server2.go ---
@@ -1,180 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-
-package thrift
-
-import (
- "context"
- "log"
- "runtime/debug"
- "sync"
-)
-
-/*
- * This is only a simple sample same as TSimpleServer but add context
- * usage support.
- */
-type TSimpleServer2 struct {
--- End diff --
Ok, but as I said in the other comment, if we use `x/net/context` for Go<1.7, then we can have a single `TSimpleServer` with context support for all Go versions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---