-
Notifications
You must be signed in to change notification settings - Fork 1.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
libthrift version pegging. #133
Comments
I see version 0.5.0 for libthrift in the pom. That seems really old. Is it safe to upgrade? |
#158 wow, messy. So why is finagle-thrift using such an old version of thrift? |
We're stuck on 0.5.0 internally at Twitter, and probably won't be moving any time soon. |
Are there any updates? The last update was almost a year ago. Thanks! |
Hi @jduan, not really much of an update here. We are still on the 0.5.x branch internally. It would be a very large undertaking to migrate everyone internally. Perhaps some day, but we're not actively working on it at the moment. |
@ryanking thank you for the update anyway! |
There's a licensing problem with using com.twitter.libthrift-0.5.0-7. This is a derived work of apache libthrift, thus falls under the ASLv2. Clause 4(d) states that when you distribute the derived work, you must also distribute the NOTICE from the original work. Twitter libthrift does not do this, which put it in violation of the license. What's more, anyone who pulls in finagle and redistributes in binary form with dependencies will also be in violation (which is my problem). Could the twitter libthrift source be made available, or at least the NOTICE added to the jar. |
@ivankelly we're planning on migrating to 0.10.0 shortly, so we won't have to be on our fork anymore. I expect it to be within less than a month. In the mean time, I believe that you can edit the libthrift JAR on your end to add the NOTICE file. Alternatively, you can instead exclude our fork of libthrift, and depend explicitly on vanilla libthrift-0.5.0. Sorry for the oversight, and thanks for pointing it out to us. |
I don't have the notice file though. The modified libthrift contains changes which are presumably copyrighted to twitter, so it's twitter that would have to provide the NOTICE.
Isn't there some concurrency bug though, which was the whole reason for the fork?
No worries. This stuff is very easy to overlook. |
@ivankelly isn't the NOTICE that it needs https://github.com/apache/thrift/blob/master/NOTICE ? When you say
I interpret that as, when you distribute the twitter JAR, you need the NOTICE from apache thrift. If that doesn't work for you, how would you feel about waiting for ~ a month for us to sort out the 0.10.0 stuff? It's almost ready to go but we want to wait to do the cutover until after the holidays. |
@mosesn if one were to put the thrift NOTICE directly into the jar, it would be assigning the copyright of the modifications twitter made to the ASF, which while I'm sure it's fine, is a little weird. Anyhow, this isn't a big deal. Marking libthrift as ASLv2 in my project for now, and as it's a ASF project, it'll be covered by the projects own NOTICE. Anyhow, as I said, no big deal, especially with the upgrade to 0.10. Thanks for the response. |
Sounds good, sorry I don't have a better answer for you :/. We'll make sure to get the 0.10.0 stuff going in a timely manner. |
Closing this since we upgraded to libthrift-0.10.0 as of 61c7a71. |
Hi as I understand since you guys are extending over some of the thrift API's generated by scrooge you are probably pegged to some version of thrift?
The text was updated successfully, but these errors were encountered: