search

scala / scala-collection-compat

makes some Scala 2.13 APIs (primarily collections, also some others) available on 2.11 and 2.12, to aid cross-building
Apache License 2.0
203 stars 87 forks source link

Reconsider package names #331

Closed lrytz closed 4 years ago

lrytz commented 4 years ago

Now that we change the artifact name, there's an opportunity to do binary breaking changes. (Though maybe it's a bad idea, and we should only consider it when we release a 3.0.0.)

In particular, I'm thinking of the inconsistency in the packages used for backported library classes, for example

I assume the motivation to put LazyList in a compat sub-package was to avoid split packages, i.e., to ensure that this module doesn't add any classes into packages that exist in the 2.11 / 2.12 standard library? Since it's not done consistently I think we should just use the original package name for backports (like scala.util.Using does).

A small but maybe still realistic issue is that the recommended import scala.collection.compat._ brings an immutable into scope which is not scala.collection.immutable.

The scala.collection.compat package object still makes sense for extension methods (import scala.collection.compat._; xs.to(List)). Though, given that this is now library-compat, should it just be scala.compat._ instead? There might be backports of non-collection extension methods in the future. I'm not sure.

NthPortal commented 4 years ago

just for reference, I put LazyList in that package because ArraySeq was already there. it's possible there were reasons for ArraySeq that don't apply to LazyList, so it shouldn't have been there

julienrf commented 4 years ago

AFAICT, ArraySeq has been moved to package collection.compat.immutable in this PR: #31. The goal was to fix an issue with OSGI (#30). IIRC we have disabled OSGI support? So, we don’t anymore have a good reason to use this package?

lrytz commented 4 years ago

Thanks for digging these out, I didn't remember. My brain seems to be suppressing all memories of OSGi related work.

SethTisue commented 4 years ago

Now that we change the artifact name, there's an opportunity to do binary breaking changes. (Though maybe it's a bad idea, and we should only consider it when we release a 3.0.0.)

Lukas and I discussed this last week.

iirc, we sadly concluded that using a different artifact name doesn't make binary breaking changes harmless, and bumping the version number to 3.0.0 doesn't make them harmless, either. In order not to send any users to dependency hell, we must either:

1) Keep bincompat.

Lukas, is that an accurate summary of our discussion?

Option 2 seems hairy and not-worth-it to me. I'm certainly not eager to tackle it, it will annoy and confuse users, and it's not clear to me that the benefit is so great, anyway. 98% of Scala OSS has already added 2.13 to their crossbuilds, so there is value in just leaving things alone.

So I suggest we go with option 1. That would mean:

wdyt?

ijuma commented 4 years ago

Changing the artifact name will potentially cause issues even with the proposed solution here. Due to transitive dependencies, one may end up with both in the classpath resulting in NoSuchMethodErrors if the newer version was used in compilation while the older version was used at runtime. Do we want to revert the artifact name change perhaps?

lrytz commented 4 years ago

Yeah... Seth said

make the old library and the new library have no potentially incompatible classes at all in common

That prevents classes from evolving. Let's say there's

class A { def f: Int }

in scala-collection-compat, and that it evolves to

class A { def f: Int; def g: Int }

in scala-library-compat. If it was the same module, sbt would pick the newest version. But with a different artifact name, both end up on the classpath, and behavior depends on which classfile is loaded first by scalac (compile-time) and the JVM (run-time).

Also moving a class (LazyList) from scala.collection.compat.immutable to scala.collection.immutable would not work with the two versions on the classpath. Let's say a project uses two libraries that depend on collection-compat

class Lib1(ll: comp.imm.LazyList)   // v1
class Lib2(ll: comp.imm.LazyList) { // v1
  new Lib1(ll)
}

now Lib1 migrates to library-compat

class Lib1(ll: imm.LazyList) // v2

This is not binary compatible comp.imm.LazyList is simply not the same class as imm.LazyList, so Lib2 v1 doesn't work with Lib1 v2.

So I only see two ways forward

Before we decide to go back, I'd like to convince myself that breaking binary compatibility once is really not an option.

The thing is, the compat library has great potential because the Scala 2.13 standard library is locked down by the forwards binary compatibility constraint. Scala 2.13 will have a very very long lifetime and 3.0 is planned to use the 2.13 library too. The compat library allows extending the standard library during that time. It's quite unfortunate that we have to explain to all users "for historical reasons, this library has a weird name and the package structure is inconsistent".

Do we have a chance to find the direct users of scala-collection-compat and somehow make a coordinated effort to move everyone to the new artifact that breaks bin compat?

julienrf commented 4 years ago

Maybe one solution to rename the artifact without ending up with two artifacts in the classpath, would be to keep scala-collection-compat, and to create an empty artifact named scala-library-compat that depends on scala-collection-compat. This way, dependency resolvers would eventually only select one scala-collection-compat artifact, even if your project depends on two libraries that depend on different versions of scala-collection-compat/scala-library-compat.

That doesn’t solve the question of breaking binary compatibility or not, though…

ijuma commented 4 years ago

Maybe the library for extending 2.13/3.0 could be scala-library-compat. It could use different package/class names and be a longer term thing. Or maybe the name should not even include compat, if that's the goal.

scala-collection-compat would continue to be used for bridging 2.11, 2.12 and 2.13. For reference, Apache Kafka no longer supports Scala 2.11. If/when we add support for 3.0, we will likely drop support for 2.12. This is just one project, but the overhead of supporting more than two versions of Scala is pretty high and I don't think it's uncommon for projects to stick to 2 versions.

lrytz commented 4 years ago

In the end, I don't think breaking binary compatibility is an option. Browsing through GitHub search, the library is used too widely, and fixing dependency conflicts will be too hard for users (figure out which of their depdendencies is built against an old collection-compat, check if there's a new version, ping maintainers, etc). So the packages have to remain what they are; as Seth said

  • collections stuff continues to live in scala/collection/compat, also new backports
  • everything else can live in its natural location, as we've already done with @nowarn and Using

For changing the artifact name, Julien's idea sounds plausible to me (but we'd have to do some testing). So we could release a new scala-library-compat:1.0.0 that is binary compatible with scala-collection-compat:2.1.6. To avoid having both scala-library-compat:1.0.0 and scala-collection-compat:2.1.6 on the classpath, the new scala-library-compat:1.0.0 would depend on a completely empty scala-collection-compat:2.2.0.

About including new classes for 2.13, we can either do it in this library, or in a completely new one. I'm fine either way. The 2.13 artifacts of scala-collection-compat are currently empty except for two package objects (scala.collection.compat and scala.collection.compat.immutable) that contain type and val aliases, which is a clean enough starting point. If we do that, I think we should rename the library to scala-library-future.

To summarize, let's vote on these options (please comment for feedback / other suggestions):

If we pick 😄 or 🎉, we can at any point start a new library scala-library-future that contains extensions for 2.13 (and backports of them for 2.12 / 2.11).

dwijnand commented 4 years ago
  • users need to find the new artifact name (breaks scala steward)

Feels viable to teach Scala Steward this, at worst non-generically.

NthPortal commented 4 years ago

IMO, for maximum ease, we should keep the new scala-library-compat at the same version as scala-collection-compat (i.e. start it at 2.2.0).

To avoid having both scala-library-compat:1.0.0 and scala-collection-compat:2.1.6 on the classpath, the new scala-library-compat:1.0.0 would depend on a completely empty scala-collection-compat:2.2.0.

something about that doesn't sound right to me. the "outer" dependency (i.e. the one that depends on the other) needs to be the one that's empty. so either collection-compat is empty, and depends on library-compat, or library-compat is empty, and depends on collection-compat. as described, if someone upgrades to collection-compat:2.2.0, they will end up with a broken dependency lacking contents (and it would thus, technically speaking, not be binary-compatible).

julienrf commented 4 years ago

IMO, for maximum ease, we should keep the new scala-library-compat at the same version as scala-collection-compat (i.e. start it at 2.2.0).

Why would that make things easier? Dependency resolution systems (e.g., coursier) will consider anyway that the two artifacts, scala-library-compat et scala-collection-compat are independent from each other so they won’t even compare their revision numbers.

as described, if someone upgrades to collection-compat:2.2.0, they will end up with a broken dependency lacking contents (and it would thus, technically speaking, not be binary-compatible).

Right, good catch! My initial proposal was to keep the content of scala-collection-compat as it is (so that it remains backward binary compatible in the next versions, and to put new stuff (unrelated to the collections) into a new artifact, scala-library-compat, that depends on scala-collection-compat (or, we could put everything into collection-compat and have library-compat an empty artifact that depends on it). Does that sound correct?

NthPortal commented 4 years ago

Why would that make things easier?

my thought was that it makes it easier for the end user to switch from the "deprecated" collection-compat to the preferred library-compat whenever they want to, since they will have the same version

NthPortal commented 4 years ago

My initial proposal was to keep the content of scala-collection-compat as it is (so that it remains backward binary compatible in the next versions, and to put new stuff (unrelated to the collections) into a new artifact, scala-library-compat, that depends on scala-collection-compat

I thought some of the new stuff had already been released in scala-collection-compat. if not, I'm fine with it being in scala-library-compat, but if so, I feel like it might be weird for some of it to be in one artifact and some in another? idk. it probably doesn't make a huge difference either way

julienrf commented 4 years ago

Looking at the changelog and Git history, it seems that the content not related to collections has not been released yet, so we still have a chance to move it to a separate artifact.

NthPortal commented 4 years ago

regarding scala-library-future - I think it should be a separate library, and I think we should version the packages (e.g. scala.future.v1.collection.NewThing), so that we can evolve things cleanly without breaking bincompat (e.g. we determined that the initial API for NewThing wasn't great, so now we have scala.future.v2.collection.NewThing). older versions (e.g. s.f.v1) could be deprecated in newer releases (in favour of s.f.v2), thus allowing smooth evolution without ever breaking bincompat. and for things that haven't changed/don't need deprecation, we could either leave as is in the old package, or alias the new package to the old package.

NthPortal commented 4 years ago

Looking at the changelog and Git history, it seems that the content not related to collections has not been released yet, so we still have a chance to move it to a separate artifact.

except for @nowarn, but that's a small enough thing that I don't think we should ruin the package separation over it

lrytz commented 4 years ago

keep the content of scala-collection-compat as it is and put new stuff into a new scala-library-compat artifact that depends on scala-collection-compat

That might make things more confusing, I feel that it's better to keep all classes in the same artifact. The standard library also doesn't have a split between collections and the rest.

NthPortal commented 4 years ago

That might make things more confusing, I feel that it's better to keep all classes in the same artifact.

In that case, I think we should make library-compat the primary artifact, and collection-compat the empty artifact.

ijuma commented 4 years ago

Have we thought about transitive dependency situations? Say I depend on collection-compat and a separate library depends on library-compat. Depending on how the build tool resolves conflicting dependencies (say the explicit dependency is the current released version, before the introduction of library-compat), this can result in some weird situations. Is that worth it just for a cosmetic module name issue?

ijuma commented 4 years ago

It seems like Maven has something for changing group ids, not sure if it also works with artifact ids: http://maven.apache.org/guides/mini/guide-relocation.html

The discussion linked from that page seems to indicate that it's under-tested though.

julienrf commented 4 years ago

Have we thought about transitive dependency situations?

sbt would emit an eviction warning if there is a transitive dependencies conflict.

ijuma commented 4 years ago

That's good for SBT users. Thoughts on Maven and Gradle?

NthPortal commented 4 years ago

Thoughts on Maven and Gradle?

I don't really use them so I'm not an expert, but dependency conflicts are not specific to this artifact, so I'm sure a solution exists. maybe the enforcer plugin for Maven? not sure

BardurArantsson commented 4 years ago

Have we thought about transitive dependency situations?

sbt would emit an eviction warning if there is a transitive dependencies conflict.

It does, but it's routinely ignored because there are so many totally inconsequential ones in projects that aren't extremely close to the upstreamiest of upstreams. IME this only really surfaces once you get a NoSuchMethodError (or similar) at runtime.

lrytz commented 4 years ago

The vote is 2-2-2 so far.

But it's 4:2 against using this library for new 2.13 additions. So if it continues to be a backports-only library, I'm really in favor of keeping the old name. Sure, it's a bit confusing, but we can explain. And it avoids messing with empty or split artifacts, introducing a faux dependency, wondering what will happen on various build tools, explaining why there are two artifacts. The usage of this library will fade out over time (when people migrate off 2.12).

ijuma commented 4 years ago

Good call.

ekrich commented 4 years ago

I don't understand all the in and outs of this discussion but would like to add a few points.

  1. To me having this library was a godsend as I was easily able to support 2.13 along with a bunch of changes as things are getting more strict.
  2. Getting my library to compile from 2.11 - Dotty was possible with just tightening things up a bit more.
  3. Adding stuff to 2.12/2.11 from 2.13 means I can use them and they work just from cross compiling.
  4. I can see this library continuing as 2.11, 2.12 are dropped when 3.1, 3.2 are added.
  5. Having standard library and collection compatibility is a good thing.

Overall, this is so good I can't imagine that it can't continue until the point it is not need via Tasty or whatever.

SethTisue commented 4 years ago

I reallllly wanted the new name, as evidenced by my premature renaming of the repository, but after reading through all this discussion, I must regretfully conclude that it isn't worth the weirdness and trouble, so we should go back to scala-collection-compat :-(

And we have consensus that scala-library-future will be separate.

som-snytt commented 4 years ago

I would like to just depend on scala-compat-compat.

NthPortal commented 4 years ago

should we create a scala-library-future repo?

som-snytt commented 4 years ago

Also make clear that scala-library-future is not just for Future fixes. Maybe we need scala-future-future.

SethTisue commented 4 years ago

I renamed the repo back to scala-collection-compat, and I'm working on merging open PRs and publishing.

I will make the scala-library-future repo soon. (We don't need to decide on the final name until it's time to publishing. The new repo will include a ticket to talk about the name. I'm thinking maybe scala-library-next would be less ambiguous.)

SethTisue commented 4 years ago

https://github.com/scala/scala-library-next

som-snytt commented 4 years ago

:100: We definitely need a separate module just for Iterators. They are so hard to get right. Calling it next is quite elegant.

SethTisue commented 4 years ago

scala-library-next(), though, or I'll get a warning