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

Reconsider package names #331

Closed
lrytz opened this issue Apr 30, 2020 · 36 comments
Closed

Reconsider package names #331

lrytz opened this issue Apr 30, 2020 · 36 comments

Comments

@lrytz
Copy link
Member

lrytz commented Apr 30, 2020

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

  • scala.collection.compat.immutable.LazyList, vs
  • scala.util.Using

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
Copy link
Contributor

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
Copy link
Contributor

julienrf commented May 4, 2020

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
Copy link
Member Author

lrytz commented May 4, 2020

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

@SethTisue
Copy link
Member

SethTisue commented May 14, 2020

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.
  • Note that this was the original plan, that this library would never, ever break bincompat. We only did it going from 1.x to 2.x because we caught the motivating problem so early that we thought it was safe that nobody was using 1.x yet. But people are definitely using 2.x now.
  1. make the old library and the new library have no potentially incompatible classes at all in common, so that 2.x and 3.x can be on the same classpath no problem

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:

  • all collections stuff continues to live in scala/collection/compat; bincompat forces this for existing stuff, and if we add more stuff, it would be confusing for everyone to have it in a different location, better to stay consistent
  • everything else can live in its natural location, as we've already done with @nowarn and Using

wdyt?

@ijuma
Copy link
Contributor

ijuma commented Jun 5, 2020

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
Copy link
Member Author

lrytz commented Jun 5, 2020

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

  • break binary compatibility
  • rename back to scala-collection-compat and remain binary compatible forever

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
Copy link
Contributor

julienrf commented Jun 5, 2020

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
Copy link
Contributor

ijuma commented Jun 6, 2020

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
Copy link
Member Author

lrytz commented Jul 8, 2020

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

  • 😄 rename back to scala-collection-compat. Continue adding backports for 2.11 and 2.12, keep 2.13 empty.
    Pro: least breaking change, nobody needs to change the dependency.
    Con: misleading name, the library contains backports outside collections.
  • 🎉 keep scala-library-compat or rename to scala-library-backports. Continue adding backports for 2.11 and 2.12, keep 2.13 empty.
    Pro: better library name.
    Con: users need to find the new artifact name (breaks scala steward), ghost dependency on empty scala-collection-compat:2.1.7.
  • 🚀 move to scala-library-future. Continue adding backports, also add new features for 2.13/2.12/2.11.
    Pro: a single place for standard library extensions.
    Con: same as above.

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
Copy link
Member

dwijnand commented Jul 8, 2020

  • users need to find the new artifact name (breaks scala steward)

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

@NthPortal
Copy link
Contributor

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
Copy link
Contributor

julienrf commented Jul 8, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

julienrf commented Jul 9, 2020

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
Copy link
Contributor

NthPortal commented Jul 9, 2020

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
Copy link
Contributor

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
Copy link
Member Author

lrytz commented Jul 9, 2020

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
Copy link
Contributor

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
Copy link
Contributor

ijuma commented Jul 9, 2020

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
Copy link
Contributor

ijuma commented Jul 9, 2020

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
Copy link
Contributor

julienrf commented Jul 9, 2020

Have we thought about transitive dependency situations?

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

@ijuma
Copy link
Contributor

ijuma commented Jul 9, 2020

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

@NthPortal
Copy link
Contributor

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
Copy link

BardurArantsson commented Jul 12, 2020

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
Copy link
Member Author

lrytz commented Jul 15, 2020

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
Copy link
Contributor

ijuma commented Jul 16, 2020

Good call.

@ekrich
Copy link
Contributor

ekrich commented Jul 16, 2020

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
Copy link
Member

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
Copy link

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

@NthPortal
Copy link
Contributor

should we create a scala-library-future repo?

@som-snytt
Copy link

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

@SethTisue
Copy link
Member

SethTisue commented Sep 3, 2020

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
Copy link
Member

@som-snytt
Copy link

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

@SethTisue
Copy link
Member

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

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

9 participants