Details

    • Type: New Feature
    • Status: In Progress
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 0.12.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      Common Lisp support is attached

      1. thrift-cl.patch
        39 kB
        Patrick Collison

        Issue Links

          Activity

          Hide
          dreiss David Reiss added a comment -

          What is the point of temporary_var?
          This should inherit directly from t_generator, not from the poorly-named t_oop_generator, which is for c-style syntax.
          generated_package is unused, and I don't think it works because of the hyphen.
          In the README, why do you pass the client to the server?
          Let's see how this turns out: http://wiki.apache.org/thrift/NamingVote

          Show
          dreiss David Reiss added a comment - What is the point of temporary_var? This should inherit directly from t_generator, not from the poorly-named t_oop_generator, which is for c-style syntax. generated_package is unused, and I don't think it works because of the hyphen. In the README, why do you pass the client to the server? Let's see how this turns out: http://wiki.apache.org/thrift/NamingVote
          Hide
          roger.meier Roger Meier added a comment -

          issue is too old, please reopen or create a new issue and patch if you need this.
          see http://thrift.apache.org/docs/HowToContribute/

          Show
          roger.meier Roger Meier added a comment - issue is too old, please reopen or create a new issue and patch if you need this. see http://thrift.apache.org/docs/HowToContribute/
          Hide
          jking3 James E. King, III added a comment -

          A pull request was submitted today implementing Common Lisp support so I am reopening this.
          https://github.com/apache/thrift/pull/1410

          Show
          jking3 James E. King, III added a comment - A pull request was submitted today implementing Common Lisp support so I am reopening this. https://github.com/apache/thrift/pull/1410
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          jking3 James E. King, III added a comment -

          Jake Farrell consider adding "Common Lisp - Compiler" and "Common Lisp - Library" to the components list when finished.

          Show
          jking3 James E. King, III added a comment - Jake Farrell consider adding "Common Lisp - Compiler" and "Common Lisp - Library" to the components list when finished.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1410

          I think it would be better to make a shell script that downloads quicklisp and runs it to install the dependencies, and make the local build in lib/cl depend on successful execution of the shell script. This makes it similar to the nodejs build which runs npm to do something similar, however the only difference is that npm is installed as part of the docker build.
          .

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1410 I think it would be better to make a shell script that downloads quicklisp and runs it to install the dependencies, and make the local build in lib/cl depend on successful execution of the shell script. This makes it similar to the nodejs build which runs npm to do something similar, however the only difference is that npm is installed as part of the docker build. .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1410

          Hey, removed all externals and added `ensure-externals.sh` script. Also squashed all commits.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1410 Hey, removed all externals and added `ensure-externals.sh` script. Also squashed all commits.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149446453

          — Diff: lib/cl/server.lisp —
          @@ -0,0 +1,230 @@
          +(in-package #:org.apache.thrift.implementation)
          — End diff –

          Does one need the ".implementation" in the namespace?

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149446453 — Diff: lib/cl/server.lisp — @@ -0,0 +1,230 @@ +(in-package #:org.apache.thrift.implementation) — End diff – Does one need the ".implementation" in the namespace?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149446200

          — Diff: lib/cl/ensure-externals.sh —
          @@ -0,0 +1,12 @@
          +#!/bin/sh
          +
          +set -e
          +
          +if [ ! -d "externals/" ] ; then
          — End diff –

          What happens if externals are partially populated due to a crash? Can one run the same quicklisp command idempotently? If so, I would remove this if statement and just always run quicklisp to make sure externals are correct. You could perhaps make the curl command smarter to only update the file if it has changed - I suspect curl has an option for that, but I don't know...

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149446200 — Diff: lib/cl/ensure-externals.sh — @@ -0,0 +1,12 @@ +#!/bin/sh + +set -e + +if [ ! -d "externals/" ] ; then — End diff – What happens if externals are partially populated due to a crash? Can one run the same quicklisp command idempotently? If so, I would remove this if statement and just always run quicklisp to make sure externals are correct. You could perhaps make the curl command smarter to only update the file if it has changed - I suspect curl has an option for that, but I don't know...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1410

          Without a linux build, won't be able to do any merges...

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1410 Without a linux build, won't be able to do any merges...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jfarrell commented on the issue:

          https://github.com/apache/thrift/pull/1410

          Travis requests show an error with "Abuse detected". I've emailed Travis support asking them to look into the issue

          Show
          githubbot ASF GitHub Bot added a comment - Github user jfarrell commented on the issue: https://github.com/apache/thrift/pull/1410 Travis requests show an error with "Abuse detected". I've emailed Travis support asking them to look into the issue
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jens-G commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149475953

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          > Our work on Thrift is sponsored by Rigetti, so there is no need for that.

          That sounds ... strange.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jens-G commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149475953 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – > Our work on Thrift is sponsored by Rigetti, so there is no need for that. That sounds ... strange.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jens-G commented on the issue:

          https://github.com/apache/thrift/pull/1410

          @jfarrell: What's your opinion re the (c) things above?

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jens-G commented on the issue: https://github.com/apache/thrift/pull/1410 @jfarrell: What's your opinion re the (c) things above?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149489004

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          Rigetti hired us to implement (and possibly merge) CL support to Thrift. All software under this delivery has to have license required by the project we are contributing to (that is Thrift). Not sure what is strange about that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149489004 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – Rigetti hired us to implement (and possibly merge) CL support to Thrift. All software under this delivery has to have license required by the project we are contributing to (that is Thrift). Not sure what is strange about that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149489770

          — Diff: lib/cl/server.lisp —
          @@ -0,0 +1,230 @@
          +(in-package #:org.apache.thrift.implementation)
          — End diff –

          it is a common practice in Common Lisp to have a package dedicated for usage by a programmer (which exports the protocol symbols - library API that is) and having no other symbols of its own and an implementation package which may have other internal symbols and interfaces. This allows avoiding for instance symbol name conflicts (for instance list vs list).

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149489770 — Diff: lib/cl/server.lisp — @@ -0,0 +1,230 @@ +(in-package #:org.apache.thrift.implementation) — End diff – it is a common practice in Common Lisp to have a package dedicated for usage by a programmer (which exports the protocol symbols - library API that is) and having no other symbols of its own and an implementation package which may have other internal symbols and interfaces. This allows avoiding for instance symbol name conflicts (for instance list vs list).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149491753

          — Diff: lib/cl/ensure-externals.sh —
          @@ -0,0 +1,12 @@
          +#!/bin/sh
          +
          +set -e
          +
          +if [ ! -d "externals/" ] ; then
          — End diff –

          it is possible. Fixed in the pushed commit (some extra eval forms are explained in the commit message.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149491753 — Diff: lib/cl/ensure-externals.sh — @@ -0,0 +1,12 @@ +#!/bin/sh + +set -e + +if [ ! -d "externals/" ] ; then — End diff – it is possible. Fixed in the pushed commit (some extra eval forms are explained in the commit message.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jens-G commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149504639

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          Well, the term "sponsored" is a bit broad to derive that particular conclusion from it. So I asked for further clarification.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jens-G commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149504639 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – Well, the term "sponsored" is a bit broad to derive that particular conclusion from it. So I asked for further clarification.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jfarrell commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149508723

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          @dkochmanski thank you for working on this and contributing it back the the Apache Thrift. The Apache license header on this should not contain a copyright to Rigetti Computing, details available at https://www.apache.org/legal/src-headers.html#headers part 2

          Show
          githubbot ASF GitHub Bot added a comment - Github user jfarrell commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149508723 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – @dkochmanski thank you for working on this and contributing it back the the Apache Thrift. The Apache license header on this should not contain a copyright to Rigetti Computing, details available at https://www.apache.org/legal/src-headers.html#headers part 2
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149592614

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          Right, thank you for pointing me to the document. Last commit removes all copyrights added by us. Note though, that files in lib/cl/ are based on work by James Anderson (and have copyrights as well) and I don't think I can remove them. Should I contact him and ask whenever we can remove his copyright stings too?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149592614 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – Right, thank you for pointing me to the document. Last commit removes all copyrights added by us. Note though, that files in lib/cl/ are based on work by James Anderson (and have copyrights as well) and I don't think I can remove them. Should I contact him and ask whenever we can remove his copyright stings too?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jfarrell commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149655532

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          That would be great. is it possible fetch his work as a third party downloadable dependency or was it used as the basis for work you did on top of it?

          Show
          githubbot ASF GitHub Bot added a comment - Github user jfarrell commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149655532 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – That would be great. is it possible fetch his work as a third party downloadable dependency or was it used as the basis for work you did on top of it?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1410#discussion_r149660082

          — Diff: lib/cl/framed-transport.lisp —
          @@ -0,0 +1,136 @@
          +(in-package #:org.apache.thrift.implementation)
          +
          +;;;; Copyright 2017 Rigetti Computing <rigetti.com>
          — End diff –

          I've send an email to Mr. Anderson – will let you know when / if I receive a reply. No, it is not possible, our work takes his repository[1] as base for further adjustments to meet the contribution requirements (missing protocols etc).

          [1] https://github.com/lisp/de.setf.thrift

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on a diff in the pull request: https://github.com/apache/thrift/pull/1410#discussion_r149660082 — Diff: lib/cl/framed-transport.lisp — @@ -0,0 +1,136 @@ +(in-package #:org.apache.thrift.implementation) + +;;;; Copyright 2017 Rigetti Computing <rigetti.com> — End diff – I've send an email to Mr. Anderson – will let you know when / if I receive a reply. No, it is not possible, our work takes his repository [1] as base for further adjustments to meet the contribution requirements (missing protocols etc). [1] https://github.com/lisp/de.setf.thrift
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jfarrell commented on the issue:

          https://github.com/apache/thrift/pull/1410

          @dkochmanski Travis support responded saying that no one from TurtleWarePL has logged into https://travis-ci.org/ ever and this is the reason this PR is not getting run within travis.

          From Travis support:

          > do you think one of their members could try to log into Travis CI at least once at https://travis-ci.org/ and try closing and reopen one of the rejected Pull Requests?

          Show
          githubbot ASF GitHub Bot added a comment - Github user jfarrell commented on the issue: https://github.com/apache/thrift/pull/1410 @dkochmanski Travis support responded saying that no one from TurtleWarePL has logged into https://travis-ci.org/ ever and this is the reason this PR is not getting run within travis. From Travis support: > do you think one of their members could try to log into Travis CI at least once at https://travis-ci.org/ and try closing and reopen one of the rejected Pull Requests?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1410

          Why don't we close this PR and open a new one free of any copyright issues or inclusion of third party code. That should resolve the issue?

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1410 Why don't we close this PR and open a new one free of any copyright issues or inclusion of third party code. That should resolve the issue?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1410

          @jfarrell logged into travis a moment ago for the first time.

          @jeking3 I'm waiting for a response from @lisp if we can remove his copyright strings (we've removed ours). Should I close the PR and issue a new one before I receive a reply?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1410 @jfarrell logged into travis a moment ago for the first time. @jeking3 I'm waiting for a response from @lisp if we can remove his copyright strings (we've removed ours). Should I close the PR and issue a new one before I receive a reply?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1410

          Let's follow @jfarrell 's lead. Whatever he says, he's managing the travis interaction.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1410 Let's follow @jfarrell 's lead. Whatever he says, he's managing the travis interaction.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1410

          closing, will reopen with squashed commits.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1410 closing, will reopen with squashed commits.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uint closed the pull request at:

          https://github.com/apache/thrift/pull/1410

          Show
          githubbot ASF GitHub Bot added a comment - Github user uint closed the pull request at: https://github.com/apache/thrift/pull/1410
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user dkochmanski opened a pull request:

          https://github.com/apache/thrift/pull/1412

          THRIFT-82 Add Common Lisp support

          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.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/TurtleWarePL/thrift develop

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/thrift/pull/1412.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 #1412


          commit c5f8973b32bcf251fa2cc9a83fedd01000148ef8
          Author: Tomek Kurcz <tomsandbox@gmail.com>
          Date: 2017-09-19T07:16:43Z

          THRIFT-82 Add Common Lisp support

          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.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user dkochmanski opened a pull request: https://github.com/apache/thrift/pull/1412 THRIFT-82 Add Common Lisp support 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. You can merge this pull request into a Git repository by running: $ git pull https://github.com/TurtleWarePL/thrift develop Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1412.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 #1412 commit c5f8973b32bcf251fa2cc9a83fedd01000148ef8 Author: Tomek Kurcz <tomsandbox@gmail.com> Date: 2017-09-19T07:16:43Z THRIFT-82 Add Common Lisp support 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1412

          squashed and rebased on top of the thrift master head.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1412 squashed and rebased on top of the thrift master head.
          Hide
          githubbot ASF GitHub Bot added a comment -
          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1410 see https://github.com/apache/thrift/pull/1412
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lisp commented on the issue:

          https://github.com/apache/thrift/pull/1410

          good evening; this popped up in my mail due to mr kochmanski's reference.
          what are you waiting for from me?

          Show
          githubbot ASF GitHub Bot added a comment - Github user lisp commented on the issue: https://github.com/apache/thrift/pull/1410 good evening; this popped up in my mail due to mr kochmanski's reference. what are you waiting for from me?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1410

          Hello, I've send you an email a few hours ago with a question, if we can remove your "copyright" headers from the code found in de.setf.thrift, because that is what is required by Thrift team (I have included details in the email). Plese see: https://github.com/apache/thrift/pull/1410#discussion_r149655532 .

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1410 Hello, I've send you an email a few hours ago with a question, if we can remove your "copyright" headers from the code found in de.setf.thrift, because that is what is required by Thrift team (I have included details in the email). Plese see: https://github.com/apache/thrift/pull/1410#discussion_r149655532 .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1412

          The bottom line is this: under the law of where mr. Anderson lives he can't yield the IP rights nor he sees a reason to do so, so we can't remove the copyrights put by him there. Code is licensed under the same license as the Thrift code though.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1412 The bottom line is this: under the law of where mr. Anderson lives he can't yield the IP rights nor he sees a reason to do so, so we can't remove the copyrights put by him there. Code is licensed under the same license as the Thrift code though.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1412

          My take on the Apache licensing rules is that we cannot accept this if it has other copyrights in it:
          ```
          If the source file is submitted with a copyright notice included in it, the copyright owner (or owner's agent) must either:

          • remove such notices, or
          • move them to the NOTICE file associated with each applicable project release, or
          • provide written permission for the ASF to make such removal or relocation of the notices.
            ```
            If Mr. Anderson would provide the ASF with written permission to move the copyright statements into a NOTICE.md file within lib/cl that would be sufficient for inclusion in the project.
          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1412 My take on the Apache licensing rules is that we cannot accept this if it has other copyrights in it: ``` If the source file is submitted with a copyright notice included in it, the copyright owner (or owner's agent) must either: remove such notices, or move them to the NOTICE file associated with each applicable project release, or provide written permission for the ASF to make such removal or relocation of the notices. ``` If Mr. Anderson would provide the ASF with written permission to move the copyright statements into a NOTICE.md file within lib/cl that would be sufficient for inclusion in the project.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1412

          After email exchange with Mr. Anderson I've came to conclusion that we should first put thrift CL library in Quicklisp (we already download dependencies from there) and pull the library from there. In this scenario lib/cl will have only Makefile and some tests. That way there is no problem with copyrights. As of tutorial part and cross tests, they are written by us, so there shouldn't be any issue with that.

          Tutorial code is written by us, same goes for cross tests. My only concern is about t_cl_generator.cc file from compiler/ module, which has yet another copyright owner, original PR THRIFT-82 issuer (https://issues.apache.org/jira/browse/THRIFT-82) Mr. Patrick Collison: https://issues.apache.org/jira/secure/attachment/12386027/thrift-cl.patch . Should I write to him? His GH handle is @pc, but he doesn't seem to be very active here lately.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1412 After email exchange with Mr. Anderson I've came to conclusion that we should first put thrift CL library in Quicklisp (we already download dependencies from there) and pull the library from there. In this scenario lib/cl will have only Makefile and some tests. That way there is no problem with copyrights. As of tutorial part and cross tests, they are written by us, so there shouldn't be any issue with that. Tutorial code is written by us, same goes for cross tests. My only concern is about t_cl_generator.cc file from compiler/ module, which has yet another copyright owner, original PR THRIFT-82 issuer ( https://issues.apache.org/jira/browse/THRIFT-82 ) Mr. Patrick Collison: https://issues.apache.org/jira/secure/attachment/12386027/thrift-cl.patch . Should I write to him? His GH handle is @pc, but he doesn't seem to be very active here lately.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1412

          Good news, I have consent from @pc to change the header of the generator to match contributor guidelines. So next steps are:

          • merge changes to de.setf.thrift repository (so missing protocols are implemented)
          • publish de.setf.thrift on Quicklisp
          • adjust this pull request for Common Lisp inclusion

          should I close this pull request for now, or leave it as is until then?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1412 Good news, I have consent from @pc to change the header of the generator to match contributor guidelines. So next steps are: merge changes to de.setf.thrift repository (so missing protocols are implemented) publish de.setf.thrift on Quicklisp adjust this pull request for Common Lisp inclusion should I close this pull request for now, or leave it as is until then?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user adxpx commented on the issue:

          https://github.com/apache/thrift/pull/1412

          Just confirming that I'm 👍 here and excited to see this make progress. Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user adxpx commented on the issue: https://github.com/apache/thrift/pull/1412 Just confirming that I'm 👍 here and excited to see this make progress. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1412

          @jfarrell was going to take action either on contacting individuals or making decisions on this, so I'm waiting to see what the result of those efforts is. I would much prefer the cl code for thrift be in the thrift project itself and not hosted somewhere else.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1412 @jfarrell was going to take action either on contacting individuals or making decisions on this, so I'm waiting to see what the result of those efforts is. I would much prefer the cl code for thrift be in the thrift project itself and not hosted somewhere else.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkochmanski commented on the issue:

          https://github.com/apache/thrift/pull/1412

          any update on this? I'm fine with both options (given all parties agree on them) and have time to work on code forward.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkochmanski commented on the issue: https://github.com/apache/thrift/pull/1412 any update on this? I'm fine with both options (given all parties agree on them) and have time to work on code forward.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1412

          @dkochmanski still waiting for @jfarrell to chime in since he was going to take some actions. I provided him with an email that could be used to contact the author(s) involved, but I haven't heard anything since that. My preference would be to get permission to build the cl implementation for thrift into the thrift project rather than have it sitting external.

          If the cl part of the project is going to sit external then we'll likely need to consider it a "contrib/" type of submission where it is maintained external to the project. That doesn't sound like a good long term solution for adding a language to the collection.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1412 @dkochmanski still waiting for @jfarrell to chime in since he was going to take some actions. I provided him with an email that could be used to contact the author(s) involved, but I haven't heard anything since that. My preference would be to get permission to build the cl implementation for thrift into the thrift project rather than have it sitting external. If the cl part of the project is going to sit external then we'll likely need to consider it a "contrib/" type of submission where it is maintained external to the project. That doesn't sound like a good long term solution for adding a language to the collection.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1412

          I have sent an email to the two participants for which permission is needed to move copyright statements in order to comply with the Apache licensing requirements. I asked them to each post either acceptance or rejection here for the record.

          Once we have their answers we can proceed to whatever the next step will be. Without permission the bulk of the code may have to live in some other repository, untouched, and downloaded as part of the build process.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1412 I have sent an email to the two participants for which permission is needed to move copyright statements in order to comply with the Apache licensing requirements. I asked them to each post either acceptance or rejection here for the record. Once we have their answers we can proceed to whatever the next step will be. Without permission the bulk of the code may have to live in some other repository, untouched, and downloaded as part of the build process.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uint commented on the issue:

          https://github.com/apache/thrift/pull/1412

          • Rebased and resolved merge conflicts
          • Fixed some minor issues with `make clean` and dependencies
          • Added (newly) failing tests to `known_failures_Linux.json`
          Show
          githubbot ASF GitHub Bot added a comment - Github user uint commented on the issue: https://github.com/apache/thrift/pull/1412 Rebased and resolved merge conflicts Fixed some minor issues with `make clean` and dependencies Added (newly) failing tests to `known_failures_Linux.json`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uint commented on the issue:

          https://github.com/apache/thrift/pull/1412

          @jeking3 I'm really sorry, I only noticed there were suggested changes attached to your comment after I did a force push. I can't see them anymore. Do you remember if there was anything important there?

          I removed the CL Thrift library and made it so that it's downloaded during the building process. I think all the code that is left is either written by us (cross-tests, tutorial, build integration, etc.) or isn't an issue (code generator).

          Currently we download the library from our fork of Anderson's work by downloading (curl) the zip file from github and unzipping it. The long-term goal, though, is to merge our fork with upstream and get it added to quicklisp. After that we can just download the library in a similar way we download all the dependencies.

          I used that suggested bash snippet from the closed PR. It worked nicely.

          What do you think? Is this PR fine in its current shape?

          Show
          githubbot ASF GitHub Bot added a comment - Github user uint commented on the issue: https://github.com/apache/thrift/pull/1412 @jeking3 I'm really sorry, I only noticed there were suggested changes attached to your comment after I did a force push. I can't see them anymore. Do you remember if there was anything important there? I removed the CL Thrift library and made it so that it's downloaded during the building process. I think all the code that is left is either written by us (cross-tests, tutorial, build integration, etc.) or isn't an issue (code generator). Currently we download the library from our fork of Anderson's work by downloading (curl) the zip file from github and unzipping it. The long-term goal, though, is to merge our fork with upstream and get it added to quicklisp. After that we can just download the library in a similar way we download all the dependencies. I used that suggested bash snippet from the closed PR. It worked nicely. What do you think? Is this PR fine in its current shape?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1412

          I will take another look. Can you squash this into a single commit to prepare it for inclusion if it looks good?

          Show
          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1412 I will take another look. Can you squash this into a single commit to prepare it for inclusion if it looks good?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uint commented on the issue:

          https://github.com/apache/thrift/pull/1412

          Of course. Squashed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uint commented on the issue: https://github.com/apache/thrift/pull/1412 Of course. Squashed.
          Hide
          jking3 James E. King, III added a comment -

          The 0.11.0 release cycle started before this was ready; I would love to see this get into 0.11.0 but I think it might be too late, so I am marking it for 0.12.0. We're going to go more frequent releases than we have in the past few years however, given we've added one more release manager.

          Jens Geyer if there's a way to pull this into 0.11.0, please consider it. I've marked it as 0.12.0 planned for now.

          Show
          jking3 James E. King, III added a comment - The 0.11.0 release cycle started before this was ready; I would love to see this get into 0.11.0 but I think it might be too late, so I am marking it for 0.12.0. We're going to go more frequent releases than we have in the past few years however, given we've added one more release manager. Jens Geyer if there's a way to pull this into 0.11.0, please consider it. I've marked it as 0.12.0 planned for now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uint commented on the issue:

          https://github.com/apache/thrift/pull/1412

          I won't lie, this feels good! Thanks a lot for the extensive support.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uint commented on the issue: https://github.com/apache/thrift/pull/1412 I won't lie, this feels good! Thanks a lot for the extensive support.
          Hide
          jensg Jens Geyer added a comment -

          Bad timing, but there will be a 0.12.0. I'm happy enough we have this one this far.

          Show
          jensg Jens Geyer added a comment - Bad timing, but there will be a 0.12.0. I'm happy enough we have this one this far.

            People

            • Assignee:
              jking3 James E. King, III
              Reporter:
              patrickc Patrick Collison
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development