Skip to content
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

Merged
merged 21 commits into from
Jul 6, 2022
Merged

Conversation

emitc2h
Copy link
Contributor

@emitc2h emitc2h commented Jun 9, 2022

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).

@emitc2h emitc2h changed the title ScalaPB Upgrade ScalaPB & sbt Upgrade Jun 9, 2022
@emitc2h emitc2h mentioned this pull request Jun 9, 2022
Copy link

@AlexisBRENON AlexisBRENON left a 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")

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

Copy link
Contributor Author

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/

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.

Copy link
Contributor Author

@emitc2h emitc2h Jun 13, 2022

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
...

@emitc2h
Copy link
Contributor Author

emitc2h commented Jun 10, 2022

@AlexisBRENON There are a number of changes you can see in project/MLeapProject.scala, which had to do with the changes in how to specify dependencies in sbt 1.+. Were you expecting anything else?

@AlexisBRENON
Copy link

AlexisBRENON commented Jun 13, 2022

@AlexisBRENON There are a number of changes you can see in project/MLeapProject.scala, which had to do with the changes in how to specify dependencies in sbt 1.+. Were you expecting anything else?

Sorry for the typo. I was thinking about project/Protobuf.scala (which is related to scalapb) where there is a lot of <task> in <config> to convert to <config> / <task> .

@emitc2h
Copy link
Contributor Author

emitc2h commented Jun 13, 2022

@AlexisBRENON I made the requested changes to Protobuf.scala.

@emitc2h
Copy link
Contributor Author

emitc2h commented Jun 15, 2022

@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.

Copy link
Contributor

@jsleight jsleight left a 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?

@emitc2h
Copy link
Contributor Author

emitc2h commented Jul 5, 2022

@jsleight @AlexisBRENON I've fixed the python tests. Turns out the SCALA_CLASS_PATH env var wasn't generated right with the sbt upgrade.

Copy link
Contributor

@jsleight jsleight left a 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 😢

@jsleight jsleight merged commit 9f265d1 into combust:master Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants