[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-23 Thread RobberPhex 
Github user RobberPhex commented on the issue:

https://github.com/apache/thrift/pull/1391
  
@jeking3 

Currently, I just remove PHP5 support at PHP extension(thrift_protocol), 
PHP library is still support PHP5.
So, I think this PR is good.

---

If PHP5 support for extension is needed, I will add it back.


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-23 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
I posted a message on the developer mailing list about removing php5 
support.  We'll see what comes of it.  php5 is currently still shipping so I 
have some reservations about removing support for it, but I want to hear if 
there are objections.  If there are, can this fix be modified to deal with both?


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-22 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
I already discussed the possibility of retiring the trusty build 
environment to reduce project maintenance.   Maybw now is the time to do it.  
That said, I would really like to get input from someone else who uses php 
regularly.  @Jens-G perhaps... to ensure this change is acceptable (removing 
php5 support).  I haven't seen any objections in the developer email list but 
it hasn't been expressly mentioned.


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-22 Thread RobberPhex 
Github user RobberPhex commented on the issue:

https://github.com/apache/thrift/pull/1391
  
at job `Autotools (Ubuntu Trusty)`, it just run `make check`, so php 
extension cannot be compiled.

at job `Cross Language Tests`, it needn't php extension.

So, I think there already are some CI problems at php part.


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-22 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
Hmm, I'm just curious about this - given support for php5 was dropped, any 
idea why the ubuntu-trusty "make check" job succeeded (because it has PHP 5.5.9 
in it)?  Is the build job not actually running any unit tests or anything that 
would parse (and fail, if it really is incompatible) the code?


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-21 Thread RobberPhex 
Github user RobberPhex commented on the issue:

https://github.com/apache/thrift/pull/1391
  
Hi, about license.

`Copyright (C) 2009 Facebook` at file 
`lib/php/src/ext/thrift_protocol/config.m4` was add by commit 
f82aee5087bd62989482f5c532cbd80f97a39b7f.
But before this commit, the license at 
[`php_thrift_protocol.cpp`](https://github.com/apache/thrift/blob/f82aee5087bd62989482f5c532cbd80f97a39b7f^/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp),
 
[`php_thrift_protocol.h`](https://github.com/apache/thrift/blob/f82aee5087bd62989482f5c532cbd80f97a39b7f^/lib/php/src/ext/thrift_protocol/php_thrift_protocol.h)
 was Apache License.



---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-21 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
@jfarrell I see licensing changes in this pull request, can you review?


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-20 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
I saw cross test pass before, so I think we're good.


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-20 Thread RobberPhex 
Github user RobberPhex commented on the issue:

https://github.com/apache/thrift/pull/1391
  
build job 4 failed due to https://issues.apache.org/jira/browse/THRIFT-2913.

And, appveyor build queued.
but, same commit at my repo CI is success: 
https://ci.appveyor.com/project/RobberPhex/thrift/build/1.0.0-dev.58


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-20 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
UBsan build failed due to https://issues.apache.org/jira/browse/THRIFT-2913.


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-20 Thread RobberPhex 
Github user RobberPhex commented on the issue:

https://github.com/apache/thrift/pull/1391
  
this pr fix 
[`THRIFT-4356`](https://issues.apache.org/jira/browse/THRIFT-4356) and 
[`THRIFT-4353`](https://issues.apache.org/jira/browse/THRIFT-4353).

---

And, this work is besed `董菲 `'s work. so I just 
squashed commits by author(not all-in-one commit).


---

[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension

2017-10-20 Thread jeking3 
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1391
  
Is this for THRIFT-4353?  Can you squash it?  Rebase it on master to fix 
the CI issues.


---