[GitHub] thrift issue #1410: Common Lisp support

2017-11-04 Thread uint
Github user uint commented on the issue:

https://github.com/apache/thrift/pull/1410
  
So about removing the bundled dependencies (`lib/cl/externals`).

There is a nifty package manager for CL (Quicklisp) that works a bit like, 
say, Cargo for Rust. It's commonly used, but the problem is it isn't available 
in `PATH` or anything - it's installed in the user's dir normally. There's no 
good way to detect its presence while building Thrift.

What if we included the Quicklisp installer (literally a single `.lisp` 
file), installed it locally during the build, and then used it to pull in other 
dependencies? Would that be fine?


---


[GitHub] thrift issue #1410: Common Lisp support

2017-11-03 Thread dkochmanski
Github user dkochmanski commented on the issue:

https://github.com/apache/thrift/pull/1410
  
Thank you for taking your time for the review


---


[GitHub] thrift issue #1410: Common Lisp support

2017-11-03 Thread jeking3
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1410
  
@jfarrell for some reason there is no recorded Travis CI build for this 
pull request.  When the author squashes and resubmits hopefully this will kick 
off a proper build?


---


[GitHub] thrift issue #1410: Common Lisp support

2017-11-03 Thread jeking3
Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1410
  
Some initial thoughts, before I review all of the files individually:

1. Please squash.
2. Please add [THRIFT-82] at the beginning of the commit description and 
the pull request description.
3. Please review https://thrift.apache.org/docs/HowToContribute.

I resurrected https://issues.apache.org/jira/browse/THRIFT-82 upon seeing 
this.  I'll start going through the files.


---