-
Notifications
You must be signed in to change notification settings - Fork 311
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
ScalaPB & sbt Upgrade #818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected some updates to the project/MleapProject.scala sbt file too.
addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.0.0") | ||
addSbtPlugin("com.thesamet" % "sbt-protoc" % "1.0.2") | ||
addSbtPlugin("org.xerial.sbt" % "sbt-sonatype" % "3.9.13") | ||
addSbtPlugin("com.typesafe.sbt" % "sbt-native-packager" % "1.8.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can still be upgraded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can upgrade sbt-protoc to 1.0.6, but according to the repos MLeap looks into, those versions for sbt-sonatype and sbt-native-packager are the latest I can get for scala 2.12/sbt-1.0:
https://scala.jfrog.io/ui/native/sbt-plugin-releases/com.typesafe.sbt/sbt-native-packager/scala_2.12/sbt_1.0/
https://repo1.maven.org/maven2/com/thesamet/sbt-protoc_2.12_1.0/
https://repo1.maven.org/maven2/org/xerial/sbt/sbt-sonatype_2.12_1.0/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should miss something.
Where are "the repos MLeap looks into" defined ?
SNT code repo redirects to this artifact repo for the list of their release.
https://index.scala-lang.org/sbt/sbt-native-packager/artifacts/sbt-native-packager
Anyway, this is not a blocking comment, it is not directly linked to the goal of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where they are defined, but if you specify a version that doesn't exist, the error message tells you which repos its looking into:
...
[error] sbt.librarymanagement.ResolveException: Error downloading com.thesamet:sbt-protoc;sbtVersion=1.0;scalaVersion=2.12:1.0.8
[error] Not found
[error] Not found
[error] not found: https://repo1.maven.org/maven2/com/thesamet/sbt-protoc_2.12_1.0/1.0.8/sbt-protoc-1.0.8.pom
[error] not found: /Users/mtrottiermcdonald/.ivy2/localcom.thesamet/sbt-protoc/scala_2.12/sbt_1.0/1.0.8/ivys/ivy.xml
[error] not found: https://repo.scala-sbt.org/scalasbt/sbt-plugin-releases/com.thesamet/sbt-protoc/scala_2.12/sbt_1.0/1.0.8/ivys/ivy.xml
[error] not found: https://repo.typesafe.com/typesafe/ivy-releases/com.thesamet/sbt-protoc/scala_2.12/sbt_1.0/1.0.8/ivys/ivy.xml
...
@AlexisBRENON There are a number of changes you can see in |
Sorry for the typo. I was thinking about |
@AlexisBRENON I made the requested changes to |
@AlexisBRENON could you also look at the PR for bundle-protobuf? Those changes would need to be merged in first. I also could use some help identifying what's wrong with the python unit tests. There seems to be some problems introduced by the upgrade to java 11. I think I had to introduce it since it's the default java version with sbt 1.0, but I'm not 100% where that came in. I either need to consistently set every back to java 8 or figure out how to make the setup in .travis.yml work with java 11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this all lgtm, though I would love to see green test suite before merging. I think/hope if we merge the protobuf PR first then this could pass tests?
@jsleight @AlexisBRENON I've fixed the python tests. Turns out the SCALA_CLASS_PATH env var wasn't generated right with the sbt upgrade. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the persevering through the changes to travis. It is quite annoying to test the test environment 😢
Solving for this issue: #786
bundle-protobuf PR: combust/bundle-protobuf#3
Upgrading scalapb isn't trivial due to the unusual setup of having the protobuf classes sitting in a separate repository. Scalapb for some reason, also needs to be loaded when sbt itself is being compiled, so an sbt upgrade was required as well to make this work. The sbt config had to be altered and since I'm pretty far from an sbt expert, it should definitely be thoroughly double-checked for correctness. One thing that I can't really check is the changes I had to do w.r.t. sonatype.
I don't expect this PR to compile until the corresponding PR in bundle-protobuf is merged. I suggest getting the changes to both repos locally if you want to verify that the unit tests are green (it works for me at least).