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

Move to Scala 2.13.0-M4 #222

Closed
wants to merge 3 commits into from
Closed

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented May 18, 2018

Rebased the newCollectionsRebase branch on current master. This can go in now, no?

import scala.collection.SeqLike
import scala.collection.generic.CanBuildFrom

object ScalaVersionSpecific {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be marked private?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes good point. i did this only in #223, sorry for the confusion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we made these traits private. This means people can't use it as a trait. However, the traits are published, and they are revealed in the type signatures.

scaladoc for scala.xml.NodeSeq

 :type -v <x y="z"/>.attributes
// Type signature
scala.xml.MetaData

// Internal Type structure
TypeRef(
  TypeSymbol(
    abstract class MetaData extends AbstractIterable[scala.xml.MetaData] 
                   with Iterable[scala.xml.MetaData] 
                   with Equality 
                   with ScalaVersionSpecificIterableSerializable[scala.xml.MetaData] 
                   with Serializable

  )
)

Cost of doing business or should we do something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately, dropping such a trait from a future version will be caught by Mima:

> xml/mimaReportBinaryIssues
[info] scala-xml: found 13 potential binary incompatibilities while checking 
[info] against org.scala-lang.modules:scala-xml_2.13.0-M5:1.1.1
...
[error]  * the type hierarchy of class scala.xml.NodeSeq is different in 
[error]  current version. Missing types {scala.xml.ScalaVersionSpecificIterableSerializable}
[error]    filter with: ProblemFilters.exclude[MissingTypesProblem]("scala.xml.NodeSeq")

@ashawley
Copy link
Member

This can go in now, no?

My effort to keep your branch up-to-date was in no way a tacit approval of the changes it contained.

Like most Scala programmers, I am generally confused about the 2.13 changes, so I have a lot of questions. Since this is my first opportunity to review the changes here, I guess the first practical question I have is: "If we merge this, can we still make a 1.1.1 release from master? Can we trust MiMa here?"

@lrytz
Copy link
Member Author

lrytz commented May 19, 2018

Can we trust MiMa here?

Very good question. I'll look in more detail next week.

@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

I looked at the change again in detail with the goal to find binary incmopatible changes. I'm confident that this PR doesn't break binary compatibility.

  • NodeBuffer extends a new trait ScalaVersionSpecificNodeBuffer. This is backwards compatbile (https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.4).
    • The change is not forwards compatbile, i.e., a file compiled against the new version might have a reference to the new interface ScalaVersionSpecificNodeBuffer and therefore not work with the old library. But AFAIK we don't guarantee forwards binary compatibility for modules (unlike the standard library).
    • The stringPrefix method is now inherited from ScalaVersionSpecificNodeBuffer. This is binary compatible. In 2.11, the compiler synthesizes a method in the class NodeBuffer that forwards to the implementaion, so the class has exactly the same method. In 2.12, the compiler in fact does the same, even though it could avoid it (the method is a "default" method in ScalaVersionSpecificNodeBuffer). Either way, both 2.12 cases are binary compatible, existing invocations of the method continue to work.
  • For NodeSeq the situation is the same, there is a new parent ScalaVersionSpecificNodeSeq implementing the method newBuilder.

I did a few tests with scala-xml 1.1.0 and a local version published from this PR, both with 2.11 and 2.12.

@lrytz lrytz force-pushed the newCollectionsBootstrap branch from c749f38 to f2aea77 Compare May 23, 2018 12:15
@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

Rebased this PR on top of #223 (to avoid future conflicts).

@lrytz lrytz force-pushed the newCollectionsBootstrap branch from f2aea77 to 16e53de Compare May 23, 2018 13:44
@lrytz lrytz force-pushed the newCollectionsBootstrap branch from 16e53de to 33e5c85 Compare May 23, 2018 20:09
@ashawley
Copy link
Member

Needs rebase after merge of #223?

@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

Done!

@ashawley
Copy link
Member

ashawley commented May 23, 2018

I'm confident that this PR doesn't break binary compatibility.

Thanks for this analysis.

So presumably a user who was cross-compiling with 2.13.0-M3 and 1.1.0 would still be able to cross-compile with a 1.1.1 release and 2.13.0-M4? Maybe not if the underlying collection calls were changed out from under them?

@ashawley
Copy link
Member

So presumably a user who was cross-compiling with 2.13.0-M3 and 1.1.0

Or... when 2.13.0 finally drops, they technically lose source compatibility with scala-xml 1.1.0.

@lrytz
Copy link
Member Author

lrytz commented May 23, 2018

1.1.1 on 2.12 has the same API as 1.1.0 on 2.12. Changes are only required if a project needs to cross-compile with 2.13.

For most users of scala-xml, it is possible to adapt the code to cross-compile between 2.12 and 2.13. A goal of the collections redesign is that user code can cross-compile.

But yes, NodeSeq is different on 2.13 because Seq[Node] is different. So scala-xml 1.1.1 on 2.12 doesn't have the same API as scala-xml 1.1.1 on 2.13.

See https://github.com/scala/scala/releases/tag/v2.13.0-M4 and https://github.com/scala/collection-strawman/wiki/FAQ for details (end-user documentation is coming for the final release).

@SethTisue
Copy link
Member

@ashawley do you have any remaining concerns about this...?

@ashawley
Copy link
Member

No, I have no remaining concerns. Will be releasing 1.1.1 relatively soon, and will probably work on getting these changes merged after that.

@SethTisue
Copy link
Member

Scala 2.13.0-M5 is now out

@SethTisue
Copy link
Member

@SethTisue SethTisue changed the title Move to M4 Move to Scala 2.13.0-M4/M5 Aug 30, 2018
@ashawley
Copy link
Member

ashawley commented Aug 30, 2018

Thanks, Seth.

Working on updating newCollectionsBootstrap, then we can tag and publish something for M5.

Test build here: https://travis-ci.org/ashawley/scala-xml/jobs/422693328

Only the M5 builds will succeed. Fortunately, the build is separated out, so I think publishing will work when we tag.

@seratch
Copy link

seratch commented Sep 12, 2018

Any updates on this?

@SethTisue
Copy link
Member

@seratch see #256

@ashawley ashawley changed the title Move to Scala 2.13.0-M4/M5 Move to Scala 2.13.0-M4 Sep 14, 2018
@ashawley ashawley mentioned this pull request Sep 14, 2018
2 tasks
@ashawley
Copy link
Member

Changes for 2.13.0-M5 will continue in to #260.

@ashawley ashawley closed this Sep 14, 2018
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.

4 participants