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 dependency is outdated #546

Closed
olafurpg opened this issue May 29, 2018 · 26 comments · Fixed by #713
Closed

ScalaPB dependency is outdated #546

olafurpg opened this issue May 29, 2018 · 26 comments · Fixed by #713

Comments

@olafurpg
Copy link
Member

olafurpg commented May 29, 2018

libraryDependencies += "com.trueaccord.scalapb" %% "compilerplugin" % "0.6.0"

The latest scalapb version is 0.7.4 https://scalapb.github.io/

addSbtPlugin("com.thesamet" % "sbt-protoc" % "0.99.18")

libraryDependencies += "com.thesamet.scalapb" %% "compilerplugin" % "0.7.4"

This causes binary conflicts for sbt plugins that depend on fastparse 1.0.0. Reported in scalameta/scalafmt#1200

@dwijnand
Copy link
Member

This will have to wait for zinc 2 (and sbt 2) because bumping this in an sbt 1 release would break existing sbt 1 plugins.

It's the same situation as jawn's binary-breaking change: eed3si9n/sjson-new#85

@dwijnand dwijnand added this to the 2.0.0 milestone May 30, 2018
@olafurpg
Copy link
Member Author

What is the timeline for zinc 2? Would it be a breaking change to exclude the fastparse dependency? ScalaPB uses fastparse for protobuf textformat, which I believe zinc does not use.

@olafurpg
Copy link
Member Author

Scratch the timeline question, just saw "(and sbt 2)". Guess that's a while from now. Seems best to change our usage of fastparse APIs in scalameta to not use default values.

@olafurpg
Copy link
Member Author

Is it possible to pin the sbt fastparse dependency to use v1.0.0 instead of v0.4.2? For example with dependencyOverrides. This would provide users with a workaround.

@dwijnand
Copy link
Member

Could you explain what the (various) dependency trees look like?

@olafurpg
Copy link
Member Author

$ coursier fetch --tree com.trueaccord.scalapb:scalapb-runtime_2.12:0.6.0 org.scalameta:common_2.12:4.0.0-M1
  Result:
├─ com.trueaccord.scalapb:scalapb-runtime_2.12:0.6.0
│  ├─ com.google.protobuf:protobuf-java:3.3.1 -> 3.5.0 (possible incompatibility)
│  ├─ com.lihaoyi:fastparse_2.12:0.4.2 -> 1.0.0 (possible incompatibility)
│  │  ├─ com.lihaoyi:fastparse-utils_2.12:1.0.0
│  │  │  ├─ com.lihaoyi:sourcecode_2.12:0.1.4
│  │  │  │  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
│  │  │  └─ org.scala-lang:scala-library:2.12.3 -> 2.12.4
│  │  ├─ com.lihaoyi:sourcecode_2.12:0.1.4
│  │  │  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
│  │  └─ org.scala-lang:scala-library:2.12.3 -> 2.12.4
│  ├─ com.trueaccord.lenses:lenses_2.12:0.4.12
│  │  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
│  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
└─ org.scalameta:common_2.12:4.0.0-M1
   ├─ com.lihaoyi:sourcecode_2.12:0.1.4
   │  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
   ├─ org.scala-lang:scala-library:2.12.4
   └─ org.scalameta:semanticdb3_2.12:4.0.0-M1
      ├─ com.thesamet.scalapb:scalapb-runtime_2.12:0.7.1
      │  ├─ com.google.protobuf:protobuf-java:3.5.0
      │  ├─ com.lihaoyi:fastparse_2.12:1.0.0
      │  │  ├─ com.lihaoyi:fastparse-utils_2.12:1.0.0
      │  │  │  ├─ com.lihaoyi:sourcecode_2.12:0.1.4
      │  │  │  │  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
      │  │  │  └─ org.scala-lang:scala-library:2.12.3 -> 2.12.4
      │  │  ├─ com.lihaoyi:sourcecode_2.12:0.1.4
      │  │  │  └─ org.scala-lang:scala-library:2.12.2 -> 2.12.4
      │  │  └─ org.scala-lang:scala-library:2.12.3 -> 2.12.4
      │  ├─ com.thesamet.scalapb:lenses_2.12:0.7.0-test2
      │  │  └─ org.scala-lang:scala-library:2.12.4
      │  └─ org.scala-lang:scala-library:2.12.4
      └─ org.scala-lang:scala-library:2.12.4

@dwijnand
Copy link
Member

Some notes:

  • fastparse 1.0.0 is out and it's binary incompatible with 0.4.2.
  • scalapb-runtime 0.6.0 uses fastparse 0.4.2, to parse text format protobuf messages which zinc doesn't use (correct?).
  • sbt depends on org.scala-sbt/sbt/1.1.5/fastparse-utils_2.12-0.4.2.jar with classes in the fastparse package.. so half-shaded.. 🙄

@dwijnand
Copy link
Member

See also @fommil and @eed3si9n's chat in sbt/sbt: https://gitter.im/sbt/sbt?at=5b0d7a16e26c847ac89e6a26

@olafurpg
Copy link
Member Author

to parse text format protobuf messages which zinc doesn't use (correct?).

I think so, protobuf has a text format that scalapb supports on Scala.js with a custom parser using fastparse.

sbt depends on org.scala-sbt/sbt/1.1.5/fastparse-utils_2.12-0.4.2.jar with classes in the fastparse package.. so half-shaded..

Can someone elaborate on what this package does and why it can't depend on fastparse directly?

@olafurpg
Copy link
Member Author

olafurpg commented May 30, 2018

I find this situation quite scary for the long-term health of sbt. I waited ~2 years to start using scalameta in sbt plugins without classloading hacks. It's only been a few months since sbt-scalafmt was released for sbt 1.0 (thanks to a generous sponsorship from the ENSIME funds) and now we're already back to the same situation as on 0.13.

sbt 1.0 has many dependencies

$ coursier fetch org.scala-sbt:sbt:1.0.2 | wc -l
71
$ coursier fetch org.scala-sbt:sbt:1.0.2 | xargs du -hc
...
36M total

Would it be possible to shade them all under some package like sbt.internal.sbt1dependencies?

Additionally, what is the policy for cases where sbt dependencies fix critical bugs? For example if okhttp3 has a bug on Java 10 that gets fixed in a new incompatible release, will that still mean sbt 1 can't upgrade?

@olafurpg
Copy link
Member Author

If the answer remains "no, it's not possible to shade or upgrade sbt dependencies" then I don't see another option than to keep on using classloading tricks from 0.13 or stop publishing sbt plugins altogehter (in favor of command-line tools). The effort required to write sbt plugins without bincompat issues is too high.

@fommil
Copy link

fommil commented May 30, 2018

if we could limit scalameta's dependencies to the versions used by sbt, we could at least limit the damage. It is very annoying that a scala dependency has creeped into the sbt hardcoded boot dependency list.

@olafurpg
Copy link
Member Author

Downgrading scalameta dependencies is an equally breaking change for our users. We were in the same situation for sbt-scalafmt/sbt-scalafix in 0.13, the best solution I know is classloading or stop publishing sbt plugins. I'm eager to find a better solution.

@dwijnand
Copy link
Member

This is, indeed, a can of worms. One that has always existed in sbt, and wasn't solved in sbt 1. Welcome to jar hell in Scala on the JVM.

I'll just respond to the questions, to avoid going even further off topic..

Would it be possible to shade them all under some package like sbt.internal.sbt1dependencies?

Sure, provided we don't expose any of their classes in public API.

Additionally, what is the policy for cases where sbt dependencies fix critical bugs? For example if okhttp3 has a bug on Java 10 that gets fixed in a new incompatible release, will that still mean sbt 1 can't upgrade?

If need be fork, backport and publish..

@fommil
Copy link

fommil commented May 31, 2018

I guess semanticdb could shade the fastparse deps too

@olafurpg
Copy link
Member Author

How hard do you estimate it would be to implement classloader isolation for sbt plugins? That would allow sbt-scalafmt to live in it's own classloader without conflicting with sbt.

I don't see a way around the current situation but to stop publishing sbt-scalafmt. I was planning to rewrite sbt-scalafix for sbt 1 to avoid classloading overhead but seems we'll hit on the same issue again. Any sbt plugin that uses fastparse 1.0 will encounter the same problem.

@dwijnand
Copy link
Member

How hard do you estimate it would be to implement classloader isolation for sbt plugins? That would allow sbt-scalafmt to live in it's own classloader without conflicting with sbt.

Not sure, but the problem with classloader isolation is you can't share types across them, so it's not generally applicable to sbt plugins.

Any sbt plugin that uses fastparse 1.0 will encounter the same problem.

Any sbt plugin that uses jawn-parser 0.11.0 will hit the same issue. It's why we pushed for a binary stable JSON datatypes (and ended up forking it because it never delivered).

@olafurpg
Copy link
Member Author

@fommil scalafmt does not use semanticdb, it even has an exclusion rule for it.

The responsibility should not be shifted onto sbt users and plugin authors to come up with workarounds for this problem. The problem is not that people use dependencies. The problem is the current plugin architecture, you can't release an sbt plugin with confidence that it works correctly for end-users. It's impossible to test all potential classpath combinations in end-user builds.

We can wait for sbt 2 to upgrade dependencies but we'll hit on same problem again a few months later.

@fommil
Copy link

fommil commented May 31, 2018

this is easily worked around by just shading fastparse / sourcecode with the plugin, no? I wouldn't even be against source forking to avoid making my build more complex or messing with binaries.

We do this in both ensime-sbt and ensime with problematic things.

@olafurpg
Copy link
Member Author

olafurpg commented Jun 5, 2018

@dwijnand I have a proposal to solve this problem https://github.com/olafurpg/sbt-big
This proposal enables sbt to

  • guarantee more stable binary compatibility by not forcing sbt dependencies onto sbt plugins
  • faster sbt startup time by reducing the resolution done by the sbt launcher

This would effectively remove all sbt dependencies, which I believe is a binary compatible change and is safe to release in sbt 1.2 or 1.3.

@lihaoyi
Copy link
Contributor

lihaoyi commented Jun 5, 2018

Mill standardizes on classloader isolating “plugins” by default. Zinc goes in it’s own classloader, as does Scala.js, meaning you can trivially have multiple versions running and classpath conflicts do not occur.

You cannot share types across classloaders, but presumably things like protobuf parsing isn’t part of your plugin’s public API anyway.

@lihaoyi
Copy link
Contributor

lihaoyi commented Jun 5, 2018

Given one of SBT’s core competencies is preparing classpaths for execution in classloaders/subprocesses, I can’t imagine doing so in a plugin would be that difficult. e.g. https://github.com/sjrd/sbt-dynscalajs uses classloads to isolate plugin classpaths and avoid conflicts in SBT

@ngbinh
Copy link

ngbinh commented Jun 5, 2018

This is a problem we hit again and again on our mono repo. Hope there is a real solution

@Atry
Copy link

Atry commented Jun 5, 2018

Scalameta dependency is outdated, too.

Some of my sbt plugins have to depend on scalameta 1.7, because Sbt 1.x depends on scalameta 1.7 indirectly via scalafmt.

https://github.com/ThoughtWorksInc/sbt-example/blob/4f831c3/build.sbt#L9

@olafurpg
Copy link
Member Author

For the record, sbt-scalafmt works around this problem by depending on a module scalafmt-big which is a fatjar with its dependencies shaded. If someone wants to do the same, here is an example build configuration https://github.com/scalameta/scalafmt/blob/4b198a1865f62c73e8930522274ecc323d203135/build.sbt#L76-L85

@jvican
Copy link
Member

jvican commented Aug 27, 2018

The ideal solution here would be to upgrade Zinc and make sbt use a shaded version of it. There's no reason why sbt's classpath handling issues should be propagated to Zinc's development.

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

Successfully merging a pull request may close this issue.

8 participants