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

Update ScalaPB to 0.10.6 and switch to sandboxed classloader. #202

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

thesamet
Copy link
Contributor

This change uses a new functionality of protocbridge which enables code generators
to be loaded via a sandboxed classloader. This eliminates the possibility of binary
compatibility problems with SBT, protobuf-java or ScalaPB itself. This
would also enable moving forward to protobuf-java 3.12.x as it is
incompatible with the version of protobuf-java presently shipped with
SBT.

@rossabaker
Copy link
Member

Thanks.

I don't do protobuf anymore. I'd like to know what @ahjohannessen thinks, but this seems reasonable to me on the surface.

This change uses a new functionality of protocbridge which enables code generators
to be loaded via a sandboxed classloader. This eliminates the possibility of binary
compatibility problems with SBT, protobuf-java or ScalaPB itself. This
would also enable moving forward to protobuf-java 3.12.x as it is
incompatible with the version of protobuf-java presently shipped with
SBT.
@ahjohannessen
Copy link
Collaborator

@thesamet Looks good to me, but somehow I get this when publishLocal and trying it out in a project that only depends on addSbtPlugin("org.lyranthe.fs2-grpc" % "sbt-java-gen" % "0.7.2-7-gdea4202-SNAPSHOT") in plugins.sbt in project:

[error] stack trace is suppressed; run last core / Compile / protocGenerate for the full output
[error] (core / Compile / protocGenerate) error occurred while compiling protobuf files: Error downloading com.thesamet:sbt-protoc;sbtVersion=1.0;scalaVersion=2.12:0.99.34
[error]   Not found
[error]   not found: /Users/ahjohannessen/.ivy2/local/com.thesamet/sbt-protoc/scala_2.12/sbt_1.0/0.99.34/ivys/ivy.xml
[error]   not found: https://repo1.maven.org/maven2/com/thesamet/sbt-protoc_2.12_1.0/0.99.34/sbt-protoc-0.99.34.pom
[error] Total time: 1 s, completed Jun 22, 2020 11:22:30 AM

However, if I add addSbtPlugin("com.thesamet" % "sbt-protoc" % "0.99.34") before then things work as expected. Not sure why that is so.

@thesamet
Copy link
Contributor Author

thesamet commented Jun 22, 2020

@ahjohannessen It looks like this problem happens in a multi-project setup. The resolver I added here doesn't take effect. I think that a better direction than touching the project's resolvers, would be to split the code generator from the sbt-plugin into a separate library. This way default resolution would work. I'll update this PR.

@thesamet
Copy link
Contributor Author

Latest commit restructures the project to have the code generator as a separate plugin. This set up also paves the path to using the code generator outside SBT (can be compiled to native, used with maven, etc)

@ahjohannessen
Copy link
Collaborator

@thesamet I'll try it out

@ahjohannessen
Copy link
Collaborator

@thesamet Now it works :) Great stuff 👍

@ahjohannessen
Copy link
Collaborator

@rossabaker LGTM - would be great to get a 0.7.3 version published when this is merged :)

@rossabaker
Copy link
Member

Does this change anything for consumers in a way that would justify a bump to 0.8.0?

@thesamet
Copy link
Contributor Author

I don't think a bump to 0.8.0 is needed as end users should be unaffected, and the Java runtime is the same (so it's binary compatible with earlier versions). However, the SBT plugin (sbt-java-gen) is not binary compatible with earlier versions since I moved things to a new artifact. The only scenario where something might break is if there is some another SBT plugin that depends on sbt-java-gen (which I believe is unlikely)

@rossabaker rossabaker merged commit 00615e1 into typelevel:master Jun 22, 2020
@ahjohannessen
Copy link
Collaborator

Awesome :) Thanks a lot @thesamet

@rossabaker
Copy link
Member

Released as 0.7.3.

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.

3 participants