-
Notifications
You must be signed in to change notification settings - Fork 4k
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
THRIFT-4513: Fixing java thrift compiler to generate constants in stable order. #1505
Conversation
Please add the issue number to your commit message and PR title. See here (point 7 under Github). |
6a280b4
to
d0cab60
Compare
d0cab60
to
32e4c53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have broken a number of builds; see:
32e4c53
to
c9eb18a
Compare
@jeking3 Yes, thanks, PR had some assumption on Map keys being only primitive for constant maps, which is not true in general. Latest revision should no longer rely on it. |
c9eb18a
to
10371f5
Compare
Updated approach: t_const_value now defines ordering. |
10371f5
to
9be9492
Compare
Please squash and rebase on master, I just merged 4 commits that stabilize the CI builds. |
13f2cc7
to
49b77be
Compare
@jeking3 done, thanks, let's see if it passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this locally I get a unit test failure in lib/java:
:testJar
:test
org.apache.thrift.TestEnumContainers > testEnumConstants FAILED
junit.framework.ComparisonFailure at TestEnumContainers.java:75
171 tests completed, 1 failed
:test FAILED
The test failure is:
junit.framework.ComparisonFailure: expected:<[lightning bol]t> but was:<[triden]t>
at junit.framework.Assert.assertEquals(Assert.java:100)
at junit.framework.Assert.assertEquals(Assert.java:107)
at junit.framework.TestCase.assertEquals(TestCase.java:269)
at org.apache.thrift.TestEnumContainers.testEnumConstants(TestEnumContainers.java:75)
It looks like something is broken.
…ble order. See https://issues.apache.org/jira/browse/THRIFT-4513 - Implementing deep comparison in t_const_value to allow stability of generated code with complex map keys.
49b77be
to
9e3944d
Compare
Fix is in, the only failure left is this one, I'm not sure if it is instability of tests or real problem: https://travis-ci.org/apache/thrift/jobs/353050154 |
Looks like a dlang issue that pops up from time to time. Just to satisfy myself this change is okay I need to look at generated thrift file comparisons from before and after this change. I'll do that locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have been better to add some typedefs for the constant map declarations, but this works. I looked at the generated output. The change affected all languages, but only the order in which these things were initialized; the end result is the same. Previously the pointer address of the item as a key in the map was the comparator so I can see how it was effectively a random order.
Thank you for the review! |
- testing double rendering in JS, Java, Python and C++ - a comment added (about setprecision) - BOOST_TEST -> BOOST_CHECK double rendering in erlang, fixed (some additional tests added) less than operator - correction in erlang test style changes, some fixes in erlang test some fixes in erlang test some fixes in erlang test - no need for including lists library more style changes - something overlooked expected 2 lines before main - fixed (not seen locally) noticed that test cases should be added manually in Python DoubleConstantTest thrift compilation directive is now given in generate.cmake render_double_to_string -> emit_double_as_string added assertion error messages to see the problems inside Java tests in more detail RDP access to AppVeyor build workers THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 duplicate key problem in appveyor settings ignore the test cases with (-+)1.7e+308 in Java tests ignore the test cases with (-+)1.7e+308 in Java tests - 2 pyflakes8 style changes disabling the test named 'TestRenderedDoubleConstants' on machines with the MSVC2010 configuration msvc.profile parameter existence check passing "msvc profile" on build stage "MSVC_PROFILE" as a cache variable wrong usage of cache variables let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether TestRenderedDoubleConstants is excluded or not msvc.profile access - fixed msvc.profile access - fixed THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 TestRenderedDoubleConstants (in Java/Python) has been disabled on MSVC2013 builders THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 reverting changes on disabling a test depending on the MSVC compiler double emitting operation depending on the compiler version THRIFT-4474: Use PSR-4 autoloader by default Client: php This closes apache#1479 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs This closes apache#1508 emit_double_as_string refactored unnecessary carat has been removed
- testing double rendering in JS, Java, Python and C++ - a comment added (about setprecision) - BOOST_TEST -> BOOST_CHECK double rendering in erlang, fixed (some additional tests added) less than operator - correction in erlang test style changes, some fixes in erlang test some fixes in erlang test some fixes in erlang test - no need for including lists library more style changes - something overlooked expected 2 lines before main - fixed (not seen locally) noticed that test cases should be added manually in Python DoubleConstantTest thrift compilation directive is now given in generate.cmake render_double_to_string -> emit_double_as_string added assertion error messages to see the problems inside Java tests in more detail RDP access to AppVeyor build workers THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 duplicate key problem in appveyor settings ignore the test cases with (-+)1.7e+308 in Java tests ignore the test cases with (-+)1.7e+308 in Java tests - 2 pyflakes8 style changes disabling the test named 'TestRenderedDoubleConstants' on machines with the MSVC2010 configuration msvc.profile parameter existence check passing "msvc profile" on build stage "MSVC_PROFILE" as a cache variable wrong usage of cache variables let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether TestRenderedDoubleConstants is excluded or not msvc.profile access - fixed msvc.profile access - fixed THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 TestRenderedDoubleConstants (in Java/Python) has been disabled on MSVC2013 builders THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 reverting changes on disabling a test depending on the MSVC compiler double emitting operation depending on the compiler version THRIFT-4474: Use PSR-4 autoloader by default Client: php This closes apache#1479 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs This closes apache#1508 emit_double_as_string refactored unnecessary carat has been removed THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 THRIFT-4474: Use PSR-4 autoloader by default Client: php This closes apache#1479 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs This closes apache#1508 THRIFT-4523 TStreamTransportImpl.GetOutputStream broken Client: Delphi Patch: Jens Geyer THRIFT-4515: cross server test improvement: graceful test server shutdown This closes apache#1509 THRIFT-82: move to SBCL 1.4.5 (hopefully will address 1.4.4 sporadic build errors)
THRIFT-4490 Allow a default service as fallback for multiplex processors connected by old clients Client: Delphi Patch: Jens Geyer THRIFT-4492 protected ExceptionType type member of TApplicationException cannot be accessed Client: C# Patch: Jens Geyer This closes apache#1493 THRIFT-4352: update artful to use haxe 3.4.4 which fixes a core in haxe THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 Add nodejs test cases for unix domain socket Indicate that nodejs now supports unix domain socket THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 move the newly supported socket to the correct programming lnguage THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 remove uds type; use --domain-socket only implement createHttpUDSConnection API
For description of the issue fixed, see: https://issues.apache.org/jira/browse/THRIFT-4513