[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Commented] (THRIFT-4256) Dependency on very old version of vector library
[ https://issues.apache.org/jira/browse/THRIFT-4256?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16092566#comment-16092566 ] Aki Sukegawa commented on THRIFT-4256: -- It is because of a change in show introduced in vector 0.11. Admittedly it does not need to be exact 0.10.2, though. https://github.com/haskell/vector/issues/87 Note that our CI uses GHC 7.6 or something old which does not support OverloadedLists. Those who are on newer GHC wouldn't experience the issue. We would need to put version check for GHC to switch vector version constraint in thrift.cabal. > Dependency on very old version of vector library > > > Key: THRIFT-4256 > URL: https://issues.apache.org/jira/browse/THRIFT-4256 > Project: Thrift > Issue Type: Improvement > Components: Haskell - Library >Reporter: Tom Lippincott >Priority: Minor > > Currently, Thrift.cabal has an exact dependency of vector==0.10.12.2, but > this version is much, much older than what other packages depend on. This > makes it necessary to enable "allow-newer", which effectively ignores the > dependency, and then breaks when a package is uploaded to hackage, and > prevents inclusion of thrift in a stack curated package set. > If there's no particular reason for it (and I've been successfully compiling > thrift with vector==0.12.0.2), could this dependency be set to a range, .e.g. > >=0.12.0? I could then enter a request for thrift to be added to stack's > curated package sets. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1309: Use build tags to support context.
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[jira] [Created] (THRIFT-4256) Dependency on very old version of vector library
Tom Lippincott created THRIFT-4256: -- Summary: Dependency on very old version of vector library Key: THRIFT-4256 URL: https://issues.apache.org/jira/browse/THRIFT-4256 Project: Thrift Issue Type: Improvement Components: Haskell - Library Reporter: Tom Lippincott Priority: Minor Currently, Thrift.cabal has an exact dependency of vector==0.10.12.2, but this version is much, much older than what other packages depend on. This makes it necessary to enable "allow-newer", which effectively ignores the dependency, and then breaks when a package is uploaded to hackage, and prevents inclusion of thrift in a stack curated package set. If there's no particular reason for it (and I've been successfully compiling thrift with vector==0.12.0.2), could this dependency be set to a range, .e.g. >=0.12.0? I could then enter a request for thrift to be added to stack's curated package sets. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
[jira] [Commented] (THRIFT-4253) Go generator assigns strings to field in const instead of pointers.
[
https://issues.apache.org/jira/browse/THRIFT-4253?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16091234#comment-16091234
]
Jens Geyer commented on THRIFT-4253:
The Go generator is
[this|https://git-wip-us.apache.org/repos/asf?p=thrift.git;a=blob;f=compiler/cpp/src/thrift/generate/t_go_generator.cc;hb=HEAD].
I'd probably start looking into [{{generate_const}} or
{{render_const_value}}|https://git-wip-us.apache.org/repos/asf?p=thrift.git;a=blob;f=compiler/cpp/src/thrift/generate/t_go_generator.cc;hb=HEAD#l114].
> Go generator assigns strings to field in const instead of pointers.
> ---
>
> Key: THRIFT-4253
> URL: https://issues.apache.org/jira/browse/THRIFT-4253
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
>Affects Versions: 0.10.0
> Environment: Generated using docker-thrift.
>Reporter: Davin Chia
>
> Given the follow thrift definition:
> {code:java}
> struct custom {
> 1: required string field_a;
> 2: optional string field_b;
> }
> {code}
> with the following constant map defined in the same file:
> {code:java}
> const map custom_map = {
> ...,
> 42 : { 'field_a':'www.testa.com', 'field_b':'www.testb.com'},
> ...,
> }
> {code}
> Thrift generates the following go struct in the main {code:java}x.go{code}
> file:
> {code:java}
> type Custom struct {
> FieldA string `thrift:"field_a,required" db:"field_a" json:"field_a"`
> FieldB *string `thrift:"field_b,2" db:"field_b" json:"field_b,omitempty"`
> }
> {code}
> with the corresponding assignments in the {code:java} X-consts.go {code} file:
> {code:java}
> CUSTOM_MAP = map[int]*Custom {
> ...,
> 42: &Custom {FieldA: "www.testa.com", FieldB: "www.testb.com"},
> ...,
> }
> {code}
> I assume field_b's pointer type is to allow for null values as per the
> optional. The generator should be assigning pointers instead of strings.
> I'm not sure how much effort it would take to fix this. I am encountering
> this problem at work right and would be very happy to starting working on
> this with some guidance from experts around here.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
