You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by binaryseed <gi...@git.apache.org> on 2016/07/14 21:31:31 UTC
[GitHub] thrift pull request #1049: Fix erlang compact protocol boolean representatio...
GitHub user binaryseed opened a pull request:
https://github.com/apache/thrift/pull/1049
Fix erlang compact protocol boolean representation
I've noticed that when using the compact protocol in `erlang`, the boolean flags are inverted.
It appears the we've simply defined the constants wrong. For reference, here are a few other languages which appear to constantly define `true` as `0x01` and `false` as `0x02`:
* https://github.com/apache/thrift/blob/master/lib/rb/lib/thrift/protocol/compact_protocol.rb#L36-L37
* https://github.com/apache/thrift/blob/master/lib/py/src/protocol/TCompactProtocol.py#L84-L85
* https://github.com/apache/thrift/blob/master/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java#L101-L102
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/binaryseed/thrift fix-erl-bool-compact
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1049.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 #1049
----
commit 067440f83e746f13906c05c8eeed31c5e33e9dbc
Author: Vince Foley <vi...@newrelic.com>
Date: 2016-07-14T21:21:00Z
Fix erlang compact protocol boolean representation
----
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/1049
@binaryseed thanks, I've verified your patch locally and it does fix the problem. It'll be merged soon.
I'm also working on extending cross tests so that we can assert true/false and endianess which cannot be verified by current round trip 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 issue #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/1049
The failures are from tentative re-activation submitted later as a separate commit
I've excluded it from the merge.
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean r...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1049
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1049
Aren't we supposed to have a clean build before merging?
---
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 #1049: Fix erlang compact protocol boolean representation
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1049
In order to get this into the project we need an Apache Jira ticket to be opened against the THRIFT project and the pull request to ideally identify this ticket. I agree with your assessment and your fix. I would also recommend that as part of this fix we also attempt to re-enable the erlang compact protocol "make cross" tests which appear to be disabled:
jking@dvm-4:~/thrift/test$ grep 'erl' known_failures_Linux.json | grep compact
"csharp-erl_compact_buffered-ip-ssl",
"csharp-erl_compact_framed-ip-ssl",
"erl-cpp_compact_buffered-ip",
"erl-cpp_compact_buffered-ip-ssl",
"erl-cpp_compact_framed-ip",
"erl-cpp_compact_framed-ip-ssl",
"erl-nodejs_compact_buffered-ip",
"erl-rb_compact_buffered-ip",
"erl-rb_compact_framed-ip",
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by binaryseed <gi...@git.apache.org>.
Github user binaryseed commented on the issue:
https://github.com/apache/thrift/pull/1049
Unfortunately, adding back in the cross language tests still causes failures :( Seems like there are other bugs lurking beyond the boolean one here
```
*** Following 9 failures were unexpected ***:
csharp-erl compact buffered-ip-ssl failure(1)
csharp-erl compact framed-ip-ssl failure(1)
erl-nodejs compact buffered-ip failure(timeout)
erl-cpp compact buffered-ip failure(1)
erl-cpp compact buffered-ip-ssl failure(1)
erl-cpp compact framed-ip failure(1)
erl-cpp compact framed-ip-ssl failure(1)
erl-rb compact buffered-ip failure(1)
erl-rb compact framed-ip failure(1)
```
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/1049
In my environment d150f8b41ef3a3f63fadcc595a99bc38628cd636 looks exactly same as 49cefb9563a18bc1cc7ac70ad0f1830019a3cd65 ...
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1049
Good catch!
What about other languages? Did you check all of them?
=> If yes, what was the result?
=> If not, why not? ;-) Could you do 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 issue #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1049
Would I be able to tell that happened from any log or history that is public? From here it looks like the tests were re-enabled because you merged the hash with them enabled (d150f8b). I think it is important to have a clean and visible history so everyone knows what's going on. If in effect you simply merged 49cefb9 by reverting the changes in d150f8b before pushing, that makes sense.
---
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 #1049: THRIFT-3880 Fix erlang compact protocol boolean represen...
Posted by binaryseed <gi...@git.apache.org>.
Github user binaryseed commented on the issue:
https://github.com/apache/thrift/pull/1049
I didn't check out all the languages, just a handful to verify that the spec is: true = 1, false = 2
---
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.
---