THRIFT-4811: Add CMake config file and targets. (see #4752)#1748
THRIFT-4811: Add CMake config file and targets. (see #4752)#1748jeking3 merged 5 commits intoapache:masterfrom soroshsabz:master
Conversation
|
It is some similar to PR #1709 |
|
@jeking3 why test failed with below error? error: RPC failed; HTTP 502 curl 22 The requested URL returned error: 502 Bad Gateway is it for my commit?! |
|
@Jens-G why your JOB="Autotools (Ubuntu Bionic)" SCRIPT="autotools.sh" is passed and does not clone git clone https://go.googlesource.com/net /thrift/src/test/go/src/golang.org/x/net and does not see any error like error: RPC failed; HTTP 502 curl 22 The requested URL returned error: 502 Bad Gateway? How to request retest this PR? thanks |
|
@jeking3 I think the error on travis because for internal problem, I recheck it in my master and travis does not report any issue. did you can retest it? thanks |
|
One of the appveyor jobs failed (the cygwin one): |
jeking3
left a comment
jeking3
left a comment
There was a problem hiding this comment.
Need to resolve build issues and some minor comments here.
* Add bin variable into cmake Config module * Exclude cygwin * Resolved some review problem
* Resolved some review problem
Change error message. Co-Authored-By: soroshsabz <soorosh_abi@hotmail.com>
|
@jeking3 I applied your suggestion, and answer to your question and pass all tests. I'm grateful to rereview it. thanks |
|
I fixed a bug of thrift lua version. not sure where to put it. wonder if anyone can take a look. link: |
This seems a but out of place. Check to see if there is an open issue in the Apache Jira for thrift. If not, create one. Then submit a pull request to github. Thanks. |
|
@emmenlau could you review? Thanks. |
i do no have privilege to open issue. i'm pretty sure it's a bug and a big one. I don't have much time for this. you guy should take care of it. |
|
@jeking3 any update? I think incremental improvement is a good approach :D thanks a lot :) |
|
@philomely If you submit a pull request from your fork's philomely-patch-1 branch into apache/master then we'll see if the CI build agrees with you. Thanks. |
|
@emmenlau could you review? Thanks. |
|
I agree that its a bit sad that this PR is not cleanly based on my PR. It makes it also a lot harder to review the changes. |
|
@jeking3 The point is that I had not seen anyone else doing anything about this, even though I was searching. so I written this PR from scratch (new) with my point of view, but after I found @emmenlau work, I want to continue those work but because my work does not derived from that work and does not get any help from that, accordingly I think it is very hard to continue from @emmenlau work, because all PR must be change (In fact, it should be completely erased and my changes would be replaced.), So I create new PR. :D |
|
@emmenlau because it is not totaly based on your PR, it is not have any relation between two PRs and codes. I did not have any help from your PR. :) Of course, thank you very much for reviewing my PR and telling my issues. |
|
Thanks for the explanation, that sounds reasonable to me. When two people end up working on the same thing these kinds of collisions can happen. I just want to make sure everyone involved is okay with using THRIFT-4811 as the solution. |
|
@jeking3 So any problem exist? |
4 participants
ITNOA
Add cmake config module, this issue is duplicate THRIFT-4752.