Scrooge3upgrade #22

Merged
merged 6 commits into from Aug 24, 2012

Conversation

Projects
None yet
4 participants

jowens commented Aug 21, 2012

I believe these changes work properly with scrooge3. They upgrade a significant number of library versions, though, and I'm also not well-versed in either a) who else uses this library and might face the ramifications or b) scrooge3 and what else might break beyond the simple searchbird example that I've used. So actually looking at my changes would be appreciated.

Franklin Hu, on my changes: "Yeah that looks pretty reasonable."

@stevegury stevegury commented on the diff Aug 22, 2012

lib/template/project/plugins.sbt
-addSbtPlugin("com.twitter" %% "sbt11-scrooge" % "1.0.0")
+addSbtPlugin("com.twitter" %% "sbt11-scrooge" % "3.0.0")
+
+addSbtPlugin("com.twitter" % "sbt-thrift2" % "0.0.1")
@stevegury

stevegury Aug 22, 2012

Because of some previous ABI incompatibility, the scala convention is to happend the version of scala to the name of the jar. Sbt push this concept a little bit further by adding the version of sbt.
The people that developed those plugins have stopped maintaining them (maybe because there were tired of releasing a new version of the plugin for each new version of sbt). For those plugins the latest build available is for sbt 0.11.2, if you use another version of sbt this code won't compile.

A pragmatic way of avoiding this version number nightmare would be to provide your own sbt script like in finagle (See https://github.com/twitter/finagle/blob/master/sbt)

@jowens

jowens Aug 22, 2012

@stevegury: OK, I'll need some elaboration on this. :) Adding my own sbt script would a) do what in particular and b) obviate the need for which plugins? Does this seem to you to be the right thing to do?

I'm trying to do a balance between "do this right" and "not want to spend 3 weeks writing shell scripts".

@stevegury

stevegury Aug 22, 2012

Here you don't specify the sbt version the user has to use, so depending on which version of sbt do you have this code will potentially fail.
On my machine I have sbt 0.11.3-2, if I load this project, it will fail because it can't find sbt-package-dist_2.9.1_0.11.3 version 1.0.5

If you provide a script (just copy paste the one in finagle) that run a specific version of sbt (0.11.2), it will succeed because sbt-package-dist_2.9.1_0.11.2 version 1.0.5 is available.

@jowens

jowens Aug 22, 2012

OK. I added this sbt script and changed the other files accordingly. Great suggestion.

@stevegury stevegury and 3 others commented on an outdated diff Aug 22, 2012

lib/template/build.sbt.erb
@@ -17,16 +17,17 @@ version := "1.0.0-SNAPSHOT"
libraryDependencies ++= Seq(
"org.scala-lang" % "jline" % "2.9.1",
- "com.twitter" %% "scrooge-runtime" % "1.1.3",
- "com.twitter" %% "finagle-core" % "3.0.0",
- "com.twitter" %% "finagle-thrift" % "3.0.0",
- "com.twitter" %% "finagle-ostrich4" % "3.0.0",
- "org.scalatest" %% "scalatest" % "1.7.1" % "test",
- "com.twitter" %% "scalatest-mixins" % "1.0.3" % "test"
+ "com.twitter" % "scrooge" % "3.0.1" intransitive(),
+ "com.twitter" % "scrooge-runtime_2.9.2" % "3.0.1" intransitive(),
@stevegury

stevegury Aug 22, 2012

Why did you add "intransitive"?

@jowens

jowens Aug 22, 2012

Franklin Hu: "You shouldn't have to make changes in the generated files, we just marked scrooge as intransitive() to prevent it from pulling in thrift 0.8.0." and later "You just have to make scrooge not pull in newer version of thrift by marking them intransitive since there have been API changes between 0.5.0 and 0.8.0." (That's what they're doing in the Zipkin build.)

@chunyan

chunyan Aug 23, 2012

Is it intentional to use THRIFT 0.5.0, not THRIFT 0.8.0? Scrooge 3 implements THIRFT 0.8.0, I believe.

@jowens

jowens Aug 23, 2012

I don't know, but I'll ask Franklin to comment on this.

@franklinhu

franklinhu Aug 23, 2012

Just revisited and you may be okay without marking it intransitive.
We had to pin the dependency in Zipkin since cassie depends on finagle-thrift, which uses thrift 0.5.0. Cassie has a FakeCassandra that starts up a thrift TThreadPoolServer whose constructor signature has changed between 0.5.0 and 0.8.0.
Sorry for the confusion.

@jowens

jowens Aug 23, 2012

OK, so delete the two "intransitive()" and everything else is the same?

franklinhu was assigned Aug 22, 2012

Owner

jowens commented on 2dea7f8 Aug 22, 2012

Oops! "Finagle". Doh!

chunyan was assigned Aug 23, 2012

Owner

jowens commented on cde884d Aug 23, 2012

Only change is removal of intransitive(), everything else is whitespace

@chunyan chunyan commented on the diff Aug 23, 2012

lib/template/build.sbt.erb
@@ -16,17 +16,18 @@ name := "birdname"
version := "1.0.0-SNAPSHOT"
libraryDependencies ++= Seq(
- "org.scala-lang" % "jline" % "2.9.1",
- "com.twitter" %% "scrooge-runtime" % "1.1.3",
- "com.twitter" %% "finagle-core" % "3.0.0",
- "com.twitter" %% "finagle-thrift" % "3.0.0",
- "com.twitter" %% "finagle-ostrich4" % "3.0.0",
- "org.scalatest" %% "scalatest" % "1.7.1" % "test",
- "com.twitter" %% "scalatest-mixins" % "1.0.3" % "test"
+ "org.scala-lang" % "jline" % "2.9.1",
+ "com.twitter" % "scrooge" % "3.0.1",
+ "com.twitter" % "scrooge-runtime_2.9.2" % "3.0.1",
+ "com.twitter" % "finagle-core" % "5.3.6",
@chunyan

chunyan Aug 23, 2012

Scrooge 3.0.1 depends on the following versions of util and finagle
finagle-core: 5.0.0
finagle-thirft: 5.0.0
finagle-ostrich4: 5.0.0
util-core: 5.0.3
util-codec: 5.0.3

Does it work if you put newer versions here? (it might, but I just want to make sure)

@jowens

jowens Aug 23, 2012

It appears to work for the purposes of initializing searchbird. I just picked the latest versions of the three finagle libraries (and did not specify the util-* libraries). But of course searchbird is only one use instance.

@chunyan chunyan added a commit that referenced this pull request Aug 24, 2012

@chunyan chunyan Merge pull request #22 from jdowens/scrooge3upgrade
Scrooge3upgrade
e38ae6c

@chunyan chunyan merged commit e38ae6c into twitter-archive:master Aug 24, 2012

chunyan was unassigned by grimreaper Mar 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment