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

release for Scala 2.13.0-M5 #540

Closed
raboof opened this issue Jan 31, 2019 · 18 comments
Closed

release for Scala 2.13.0-M5 #540

raboof opened this issue Jan 31, 2019 · 18 comments

Comments

@raboof
Copy link
Contributor

raboof commented Jan 31, 2019

I see there were some releases with Scala 2.13 milestones in the past, but none recently. It would be great to start building those (at least scalapb-runtime), to be prepared for 2.13 and to allow testing projects that depend on scalapb with 2.13 milestones.

Depends on #510

@dwijnand
Copy link

This is increasingly becoming a blocker for our projects.

If any maintainer here could give some guidelines on how to help unblock this, we'd love to be able to help. Thank you!

@thesamet
Copy link
Contributor

thesamet commented Mar 11, 2019

To get this done we need to upgrade to fastparse2 (#510), however fastparse2 dropped support for Scala Native (com-lihaoyi/fastparse#215) - to get that done we are awaiting a 0.3.x release which contains the following fix scala-native/scala-native#1429

Alternatives:

  • Upgrade ScalaPB to fastparse 2 and drop Scala Native support until the above gets fixed :/
  • Release 2.13.0-M5 support for fastparse 1 (I think I have tried that route and hit some more roadblocks)

@dwijnand
Copy link

Thank you for the details, @thesamet! I think @raboof has done that in #541 (though it still says "not for merge" - is that still right, @raboof?). Would you be happy to review and merge that?

@raboof
Copy link
Contributor Author

raboof commented Mar 11, 2019 via email

thesamet added a commit that referenced this issue Mar 16, 2019
@thesamet
Copy link
Contributor

I'm working on the Scala 2.13 support. The breaking changes in the collections library force us to make a choice:
(1) Add a new generator option that generates Scala 2.13 specific code. Users who cross-build would have to call the generator twice from their build.
(2) Generate code for all versions by using https://github.com/scala/scala-collection-compat

Potentially the options are not mutually exclusive. I might discover that the code generated via (1) can be applied to older versions of Scala using this compat library. Still investigating.

@raboof
Copy link
Contributor Author

raboof commented Mar 19, 2019

For Akka-related projects, we have avoided depending on scala-collection-compat but instead added local compat code for those places where it was actually needed for us - of course taking inspiration from scala-collection-compat. That worked out well and we've been able to avoid those compat utilities from leaking into the user-facing API's. That might also work for ScalaPB, depending on the kinds of incompatibilities you are running into?

@thesamet
Copy link
Contributor

The main issue with generated code is usage of breakOut. For this migration, it needs to know the expected collection type so it could write .to(C) - I've got most of this done in a breaking way. What would be a strategy to avoid scala-collection-compat and having a single source? Also, any concerns with ScalaPB runtime library being dependent on scala-collection-compat?

@dwijnand
Copy link

scala-collection-compat's binary API is not stable so generated code would be at risk of breaking at runtime if an newer, incompatible scala-collection-compat were present on the classpath.

If you'd like help and can point me to some partial progress, I'd be happy to send you possible solutions.

@thesamet
Copy link
Contributor

One question in the meantime. Assume that the generated code has something like this:

val c: C = a.map(f)(scala.collectoin.breakOut)

The Scala 2.13 code I'd generate would be:

val c: C = a.iterator.map(f).to(C)

but this won't compile on 2.12. For this specific use case, what would be a good way to generate the same source code for all versions of Scala without relying on scala-collection-compat?

@dwijnand
Copy link

dwijnand commented Mar 19, 2019

I think you first exclude some of the known collections (.toList[A], .toSeq[A]) then I think:

val c: C[B] = {
  val cb = C.newBuilder[B]
  for (a <- as) cb += f(a)
  cb.result()
}

It's not beautiful, but it's the desugared version of what scala-collection-compat builds up.

thesamet added a commit that referenced this issue Mar 24, 2019
@thesamet
Copy link
Contributor

I released 0.9.0-M1 with support for 2.13.0-M5. The code generated uses toSeq and friends, but this triggers deprecation warnings. Unless we discover a better way, we may have a generator option to produce 2.13-only code.

The full changelog note:

  • Experimental support for Scala 2.13.0-M5. Notes:
    • ScalaPB now generates scala.Seq by default for repeated
      fields (previously was scala.collection.Seq). This ensures usage of
      immutable Seqs on Scala 2.13 without breaking compatibility for old code.
    • The generated code compiles for Scala 2.13 and older version, so deprecation warnings
      are expected when compiling for 2.13. In a future release, we may have a
      generator that generates code that compiles cleanly, but only for 2.13.
  • Dropped support for Scala 2.10.
  • Temporarily dropped support for Scala Native (pending on fastparse2 support)
  • Add support for custom map types (It is not allowed to specify the scala Map type when using proto3 maps #410)
  • Upgrade to protobuf 3.7.0
  • Removed deprecated com.trueaccord symbols

@dwijnand
Copy link

Thank you, @thesamet!

adriaanm pushed a commit to scalacommunitybuild/ScalaPB that referenced this issue May 14, 2019
@olafurpg
Copy link
Contributor

@thesamet thank you for adding 2.13 support! We have successfully upgraded Scalameta.

ScalaPB now generates scala.Seq by default for repeated fields (previously was scala.collection.Seq). This ensures usage of immutable Seqs on Scala 2.13 without breaking compatibility for old code.

Would you be open to consider using scala.collection.Seq for repeated fields instead of scala.Seq? This would ensure ScalaPB generated APIs remain compatible between 2.12 and 2.13 and I suspect would ease the 2.13 upgrade for many codebases avoiding the need to customize scalapb.options.

@dwijnand
Copy link

Why deviate from what Scala's varargs does (which is scala.Seq)?

@olafurpg
Copy link
Contributor

Do you have a specific reason for why repeated protobuf fields should use the same type as varargs? I think collection.Seq is a reasonable type to use for repeated fields in protobuf.

Changing from collection.Seq to immutable.Seq is a source breaking change for all clients that use ScalaPB generated classes. Users who want immutable collections can use custom scalapb.options as they have been able to do in 2.12 and 2.11. My understanding is that there was a general agreement on the benefits of keeping collection.Seq for varargs but it was deemed to costly to implement in the compiler.

@dwijnand
Copy link

Changing from collection.Seq to immutable.Seq is a source breaking change for all clients that use ScalaPB generated classes.

The change is from collection.Seq to scala.Seq, which is only source-breaking when migrating your code to Scala 2.13, just like varargs. For existing Scala 2.12 code the type will remain compatible. I think immutable scala.Seq is the more reasonable type for both varargs and repeated protobuf fields, and it's probably best for ScalaPB to mirror varargs.

@olafurpg
Copy link
Contributor

I admit using collection.Seq would be a source breaking change for codebases that use scala.Seq in their own internal APIs so maybe scala.Seq is a better choice after all. I'm fine either way it goes, it wasn't a big change in our end to handle the new immutable.Seq repeated fields.

@thesamet
Copy link
Contributor

I think that scala.Seq is the better choice given the reasons that you both have mentioned (source compatibility with 2.12 and encouraging immutable structures by default in 2.13). See also issue #227 back from 2017.

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

No branches or pull requests

4 participants