search

scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

Consider bumping scala-compiler's `scala-xml` dependency to 2.x in Scala 2.12 #12632

Closed ckipp01 closed 2 years ago

ckipp01 commented 2 years ago

Problem outline

I know this was a conversation a long time ago in https://github.com/scala/scala/pull/9743 but recently this conversation has surfaced again. Recently in sbt-scoverage I had some reports of conflicts since I was still on 1.x, but some other libraries like scalatest bumped to 2.x making the problem hit people a bit harder. Due to that, I went ahead and updated to 2.x. Due to me updating, we got even more reports of conflicts with sbt-native-packager, which caused them to update to 2.x as well. Now we're in a position where people are getting hit with stuff like this:

[error]         * org.scala-lang.modules:scala-xml_2.12:2.1.0 (early-semver) is selected over {1.3.0, 1.0.6, 1.2.0}
[error]             +- org.scoverage:scalac-scoverage-reporter_2.12:2.0.2 (depends on 2.1.0)
[error]             +- com.github.sbt:sbt-native-packager:1.9.10 (sbtVersion=1.0, scalaVersion=2.12) (depends on 2.1.0)
[error]             +- org.scala-sbt:testing_2.12:1.7.1                   (depends on 1.3.0)
[error]             +- org.scala-sbt:sbinary_2.12:0.5.1                   (depends on 1.0.6)
[error]             +- org.scala-sbt:main_2.12:1.7.1                      (depends on 1.3.0)
[error]             +- org.scala-sbt:librarymanagement-core_2.12:1.7.0    (depends on 1.2.0)
[error]             +- org.scala-lang:scala-compiler:2.12.16              (depends on 1.0.6)
[error]             +- io.get-coursier:lm-coursier-shaded_2.12:2.0.10     (depends on 1.3.0)

Where common sbt plugins are conflicting with sbt and Scala 2.12 itself. Looking back at past conversations I understand the breaking changes between 1.x and 2.x of scala-xml are extremely small so doing the following is pretty safe:

in your project/plugins.sbt

ThisBuild / libraryDependencySchemes ++= Seq(
  "org.scala-lang.modules" %% "scala-xml" % VersionScheme.Always
)

But this isn't ideal to have this all over the place, and will cause confusions for users. Is it time to update scala-xml in 2.12? Is there something holding this back? We're in a weird position now where have the ecosystem is update for 2.12 and half isn't, causing issues.

guizmaii commented 2 years ago

FYI, we're having this issue right now

julienrf commented 2 years ago

Bumping to 2.x will also create issues in projects that depend on 1.x in some way. However, I agree that we should initiate the migration at some point.

ckipp01 commented 2 years ago

Bumping to 2.x will also create issues in projects that depend on 1.x in some way.

For sure, but we have the ecosystem sort of split right now. So there already are problems. Also, from my understanding an update from scala-xml 1.x to 2.x is quite trivial. So if we're going to cause issues for 2.x users or 1.x users, then I think it's wise to lean in on everyone updating, especially since the train has left the station.

I'm happy to pr this change, and can even help out around the ecosystem with pr'ing bumps to 2.x in projects that are lagging behind.

eed3si9n commented 2 years ago

Agreed on scala-xml 2.x. I am happy to update sbt dependency in the next feature release 1.8.x.

persistent versioning?

Related thought. In the age of scala-streward where people bump up versions the day a library gets released, we should be more mindful of any major version bumps for any org.scala-lang.modules that were formerly standard library, because effectively they remain more or less to be standard library. One way to workaround the conundrum of progress vs stability is making the library persistent. https://eed3si9n.com/persistent-versioning/. A persistent library embeds the major version number in the organization name and package name, like org.scala-lang.modules.xml2 and scala.xml2. This will cause no version conflict since they will look like different libraries from library management perspective.

_2.13 vs _3

Related thought 2. Even if we convince all library authors to migrate to scala-xml 2.x, an interesting issue remains for Scala 2.13-3.x dependency, such as CrossVersion.for3Use2_13. I recently wanted to use 2.13 library from Scala 3, and again because scala-xml etc are effectively standard library, they popped up as:

[info] Fetched artifacts of
[error] Modules were resolved with conflicting cross-version suffixes in ProjectRef(...):
[error]    org.scala-lang.modules:scala-xml _2.13, _3

This got me thinking that maybe we should all consider not using the _3 variant for things like scala-xml, scala-collection-compat, etc. Or put another way, the price of using _3 variant for standard library is that CrossVersion.for3Use2_13 becomes somewhat useless. If we as ecosystem want to maximize the freedom for application developers, it might be worth standardizing standard modules to _2.13?

SethTisue commented 2 years ago

I support including this upgrade in Scala 2.12.x (.17 or .18).

We might have to rethink if new information comes to light. For example, we'll want to see if scala/scala CI results contain any surprises, and the same for the Scala 2.12 community build. But based on what I know today, I'm in favor.

Just now I reviewed the various scala-xml 2.x release notes at https://github.com/scala/scala-xml/releases for potential compatibility concerns. The following PRs removed API. That bincompat breakage, even if largely rare/theoretical, was the forcing factor for the major version bump:

but it's all obscure deprecated stuff that's unlikely to have been in use by almost anybody. Well, scala.xml.pull is probably a bit likelier to have been in use, but I'm confident it was never in wide use.

A potential compatibility concern is:

This seems to me like the other most significant change (besides the scala.xml.pull removal) that some people might need to be aware of (most will not). We could link to it from the release notes.

We can't be sure a few people won't be bitten by other changes — there are other behavioral changes between 1.x and 2.x and any behavioral change might trip up somebody. Regardless, I think we should go forward with the upgrade.

If we as ecosystem want to maximize the freedom for application developers, it might be worth standardizing standard modules to _2.13?

That discussion, and the one about “persistent versioning”, is out of scope on this ticket, I think. Here, let's stick to the narrower question of what Scala 2.12.x's dependencies should be.

fyi @lrytz @ashawley

ashawley commented 2 years ago

Releasing 2.0 last year was known to be both a binary compatibility change and a behavior change but overall it was intended to be as conservative a major release as possible. The version bump was necessary, but the deprecated removals were of "vestigial code that is either unused, of poor quality or both" and that no user should really notice their loss as I had documented in the release notes:

https://github.com/scala/scala-xml/blob/8008173/CHANGELOG.md

SethTisue commented 2 years ago

Tentatively milestoned for 2.12.17, pending the outcome of this discussion. (2.12.18 is a possibility, too.)

SethTisue commented 2 years ago

The (draft-for-now) upgrade in scala/scala went smoothly:

• https://github.com/scala/scala/pull/10108

though it did require one code change, since we were using a now-removed Elem constructor. It's difficult to guess whether others might hit the same issue.

And in the 2.12 community build:

• https://github.com/scala/community-build/pull/1588

very smooth sailing — one repo had unused test-only code using scala.xml.pull, another repo had a test case that had an illegal XML comment that the new code rejects (correctly, afaict)

I've edited my long comment above so it now mentions the scala.xml.pull removal.

SethTisue commented 2 years ago

it did require one code change, since we were using a now-removed Elem constructor. It's difficult to guess whether others might hit the same issue

The worst-case scenario I can imagine is that we discover after release that some popular library or popular sbt plugin (or a library used in a popular plugin) was using some removed bit of API (perhaps the Elem constructor, or scala.xml.pull), resulting in NoSuchMethodErrors when they get the upgrade because sbt 1.8.x gets the upgrade. If that library or plugin is still actively maintained, then it's not a big problem; the maintainers can put out a new version with the needed adjustments. If the library or plugin is abandoned, then doing something about it is harder, but not impossible (e.g. by forking).

Since sbt 1.x still has a long life ahead of it, I still think we should go ahead with this upgrade — I just want to lay out the nature of the risk we're taking.

lrytz commented 2 years ago

I can't think of other concerns, but I'm also not very confident, it's difficult to predict the possible consequences. But reading through the notes here, I agree with the conclusion to go ahead.

SethTisue commented 2 years ago

I did one further test, as follows, using the PR validation snapshot for the linked PR (commit e1a218e).

As expected, the POM at https://scala-ci.typesafe.com/native/scala-pr-validation-snapshots/org/scala-lang/scala-compiler/2.12.17-bin-e1a218e-SNAPSHOT/scala-compiler-2.12.17-bin-e1a218e-20220810.200117-1.pom has the scala-xml 2.1.0 dependency.

I grabbed the scala-compiler, scala-reflect, and scala-library JARs for e1a218e from scala-pr-validation-snapshots, and threw in Iterator.scala from the 2.12 standard library sources.

Then as a sanity check I verified that java -classpath scala-compiler-2.12.17-bin-e1a218e-20220810.200117-1.jar:scala-library-2.12.17-bin-e1a218e-20220810.200117-1.jar:scala-reflect-2.12.17-bin-e1a218e-20220810.200117-1.jar scala.tools.nsc.ScalaDoc -usejavacp Iterator.scala fails with java.lang.NoClassDefFoundError: scala/xml/MetaData, since we have no version of scala-xml at all on the classpath.

Then I added /Users/tisue/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/2.1.0/scala-xml_2.12-2.1.0.jar to the classpath and the same thing succeeded.

So then finally the real test is, suppose we put the scala-xml 1.0.6 JAR on the classpath, does Scaladoc generation still succeed?

And the answer is: yes, It does.

So that shows that we have an additional level of safety here: even if someone somehow ends up with the 2.12.17 scala-compiler.jar on their classpath along with scala-xml 1.0.6, they should be fine, even if they want to generate Scaladoc. (Recall that Scaladoc generation is the only part of the 2.12 compiler where scala-xml is even used.)

Yes, in theory, there could be some problematic code path in Scaladoc generation that this test didn't happen to exercise, so the test isn't completely ironclad. Regardless, it's good additional confidence to have.

SethTisue commented 2 years ago

I asked the community about this on the 2.12.17 planning thread at https://contributors.scala-lang.org/t/scala-2-12-17-release-planning/5805/3

If nothing new comes to light from that this week, then I think we can go ahead.

ghost commented 1 year ago

I would like to upgrade from Scala 2.12.15 to 2.12.17 but we do unfortunately have a dependency on scala.xml.pull so we are stuck. Can the scala.xml.pull classes be taken from and older scala-xml version or are there incompatibilities that will prevent this from working?

SethTisue commented 1 year ago

Can the scala.xml.pull classes be taken from and older scala-xml version or are there incompatibilities that will prevent this from working?

Good question. Let's discuss at https://github.com/scala/scala-xml/discussions/638 rather than here.