Fix CrossVersion.Disabled compat #280
Conversation
|
Need to add MiMa exclusions for these. I doubt anyone's used them seeing how they're wrong, but also they're inlined constants, so even if they're binary API has changed there are no call sites in bytecode because the values got inlined. |
|
Reported the false positive in MiMa: lightbend-labs/mima#266 |
|
@eed3si9n WDYT? |
There was a problem hiding this comment.
Currently the final vals are pointing to the companion objects, and I think we should encourage ppl to use the lowercase versions like full and binary in sbt 0.13.
The only odd duck is Disabled, which was an object in sbt 0.13, so if you want to make this change for better source compat, I think we should limit it to Disabled.
Also could we target develop branch first?
| final val Constant = sbt.librarymanagement.Constant | ||
| final val Full = sbt.librarymanagement.Full | ||
| final val Patch = sbt.librarymanagement.Patch | ||
| final val Disabled = sbt.librarymanagement.Disabled() |
There was a problem hiding this comment.
The corresponding construct in sbt 0.13 https://github.com/sbt/sbt-zero-thirteen/blob/v0.13.17/ivy/src/main/scala/sbt/CrossVersion.scala#L19:
object Disabled extends CrossVersion { override def toString = "disabled" }so the above change could be considered valid for source compat with sbt 0.13, but in general we probably should recommend migrating to lowercase CrossVersion.disabled.
| final val Full = sbt.librarymanagement.Full | ||
| final val Patch = sbt.librarymanagement.Patch | ||
| final val Disabled = sbt.librarymanagement.Disabled() | ||
| final val Binary = sbt.librarymanagement.Binary() |
There was a problem hiding this comment.
Binary and Full were classes in sbt 0.13, but had lower case value binary and full.
So keeping Binary as the companion object of sbt.librarymanagement.Binary kind of makes sense to me.
dwijnand
changed the title
|
Yep |
Not sure how but sbt#280 has slipped through with Mima failing.
Not sure how but sbt#280 has slipped through with Mima failing.
|
@dwijnand We might need to revert this PR since @cunei found breakage in the community build: |
|
It's a false positive because it's a folded constant. But MiMa isn't aware of those... lightbend-labs/mima#266 |
| @@ -157,6 +157,12 @@ lazy val lmCore = (project in file("core")) | |||
| exclude[DirectMissingMethodProblem]("sbt.librarymanagement.ArtifactExtra.extension"), | |||
| exclude[DirectMissingMethodProblem]("sbt.librarymanagement.ArtifactTypeFilterExtra.types"), | |||
|
|
|||
| // by mistake we aliased the companion object instead of an instance of Disabled | |||
| // but it was aliased as a constant expression, so even if the binary API has changed | |||
| // there are no call sites in bytecode because the value got inlined | |||
There was a problem hiding this comment.
More precisely "constant folded" not "inlined".
|
@cunei Do you have a stacktrace? I am guessing some plugin out there is actually calling |
|
True, it must be. Going to ask the Scala team about constant folding. |
|
I wonder if we can keep the bincompat by adding |
|
Yeah, I wondered that too, but there's no |
|
ok so |
|
Change |
Ref sbt#280 This is to workaround bincompat error detected by sbt community build. ``` [cats] [error] java.lang.NoSuchMethodError: sbt.librarymanagement.CrossVersion$.Disabled()Lsbt/librarymanagement/Disabled$; ```
Ref sbt#280 This is to workaround bincompat error detected by sbt community build. ``` [cats] [error] java.lang.NoSuchMethodError: sbt.librarymanagement.CrossVersion$.Disabled()Lsbt/librarymanagement/Disabled$; ```
Fixes sbt/sbt#4595 Ref sbt#290 Ref sbt#280 This is bit of an odd one. To keep bincompat and also to fix sbt 0.13 compatibility issue we have made `Disabled` companion object extend `Disabled` type. This actually created a subtle deserialization issue: ``` [error] scala.MatchError: Disabled() (of class sbt.librarymanagement.Disabled$) [error] at sjsonnew.FlatUnionFormats$$anon$5.write(FlatUnionFormats.scala:220) [error] at sjsonnew.JsonWriter.addField(JsonFormat.scala:40) [error] at sjsonnew.JsonWriter.addField$(JsonFormat.scala:37) [error] at sjsonnew.FlatUnionFormats$$anon$5.addField(FlatUnionFormats.scala:208) [error] at sjsonnew.Builder.addField(Builder.scala:43) [error] at sbt.librarymanagement.ModuleIDFormats$$anon$1.write(ModuleIDFormats.scala:46) ``` This is because Contraband generates `flatUnionFormat5[CrossVersion, Disabled, ...]` for all of the subtypes of `CrossVersion`, which uses the runtime type information. Now that `Disabled` object is also in the mix, this created JSON that `CrossVersionFormats` cannot deserialize. This brings the code into src/ so we can write this part manually.
Fixes sbt/sbt#4975 This makes `CrossVersion.Disabled` a stable identifier by reverting `final def` back to `final val`. This is to fix Scala.JS build ``` [error] ScalaJSCrossVersion.scala:34:23: stable identifier required, but sbt.`package`.CrossVersion.Disabled found. [error] case CrossVersion.Disabled => [error] ^ [error] one error found [error] (Compile / compileIncremental) Compilation failed ``` ### notes - sbt#121 added `final val Disabled = sbt.librarymanagement.Disabled` but it was just a companion object - sbt#280 actually made it `final val Disabled = sbt.librarymanagement.Disabled()`, but this broke Cat's build that was calling `CrossVersion.Disabled()` - sbt#290 changed to `final def Disabled = sbt.librarymanagement.Disabled` and `object Disabled extends sbt.librarymanagement.Disabled` - This changes back to `final val Disabled = sbt.librarymanagement.Disabled` (but because we changed the companion object in sbt#290 that's ok)
Fixes sbt/sbt#4977