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

Cross publish native #477

Merged
merged 5 commits into from
Apr 10, 2022
Merged

Conversation

kpodsiad
Copy link
Member

@kpodsiad kpodsiad commented Jan 5, 2022

An attempt to cross publish munit-native & rest to Scala 3.

Needs Scala 3.1.1 to be released, otherwise macro mixing in MacroCompat2 won't work. It fails with the Experimental erased may only be used with a nightly or snapshot version of the compiler - see scala/scala3#13795 for more details.

build.sbt Outdated Show resolved Hide resolved
build.sbt Show resolved Hide resolved
addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.6.0")

addSbtPlugin("org.portable-scala" % "sbt-scala-native-crossproject" % "1.1.0")
addSbtPlugin("org.scala-native" % "sbt-scala-native" % "0.4.3-RC2")

Choose a reason for hiding this comment

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

We can drop the -RC2 now 😃

@kpodsiad
Copy link
Member Author

@sideeffffect @gabro I'll answer to #488 here. There's a problem with scalacheck - there is no scalacheck published for Scala 3 & Native 0.4.x, see maven. I tried various tricks with CrossVersion.for3Use2_13 and etc. but without any results.
When running ++3.1.1 testsNative/test I always ended with munit depending on 0.4_3 and munitScalacheck depending on the 0.4_2.13 which unfortunately doesn't work.

@sideeffffect if you want you can play with it to get ++3.1.1 testsNative/test passing. I'm not a multiversion multiplatform expert, maybe you can get it done ;)

There is a PR to support scala-native Scala 3 in scalacheck repo typelevel/scalacheck#868. Unfortunately, it's waiting for maintainers to look at it.

@kpodsiad
Copy link
Member Author

I see 3 options:

@gabro Are you in favor of any option?

@gabro
Copy link
Member

gabro commented Jan 25, 2022

Well if we have a viable workaround, sure, but it really depends on whether you or someone else can find it.

Reaching out to Typelevel about the status of Scalacheck and getting that PR merged upstream would be the preferable option for sure, while I'm not really keen on the idea of releasing MUnit without full Scalacheck support.

@sideeffffect
Copy link

I tried various tricks with CrossVersion.for3Use2_13 and etc. but without any results.
When running ++3.1.1 testsNative/test I always ended with munit depending on 0.4_3 and munitScalacheck depending on the 0.4_2.13 which unfortunately doesn't work.

I think I've also hit this issue

[error] Modules were resolved with conflicting cross-version suffixes in ProjectRef(uri("file:/home/ondra/Projects/sbt-locales/"), "apiNative"):
[error]    org.scala-native:windowslib_native0.4 _3, _2.13
...

Please read more here:
portable-scala/portable-scala-reflect#23 (comment)

@avdv
Copy link

avdv commented Jan 25, 2022

Note, there is also a problem with scala-native itself you would need to workaround: scala-native/scala-native#2543

@sideeffffect
Copy link

sideeffffect commented Jan 25, 2022

For a workaround for the

[error] Modules were resolved with conflicting cross-version suffixes in ProjectRef(uri("file:/home/ondra/Projects/sbt-locales/"), "apiNative"):
[error]    org.scala-native:windowslib_native0.4 _3, _2.13

issue, see cquiroz/sbt-locales#190

@avdv
Copy link

avdv commented Jan 26, 2022

Using an inline conditional to work around issue scala-native/scala-native#2543 would be possible:

diff --git a/munit/native/src/main/scala/munit/internal/PlatformCompat.scala b/munit/native/src/main/scala/munit/internal/PlatformCompat.scala
index c08636e..e625d88 100644
--- a/munit/native/src/main/scala/munit/internal/PlatformCompat.scala
+++ b/munit/native/src/main/scala/munit/internal/PlatformCompat.scala
@@ -38,7 +38,7 @@ object PlatformCompat {
 
   def isJVM: Boolean = false
   def isJS: Boolean = false
-  def isNative: Boolean = true
+  inline val isNative = true // Scala 3 syntax
 
   def newRunner(
       taskDef: TaskDef,
diff --git a/munit/shared/src/main/scala-3/munit/internal/Compat.scala b/munit/shared/src/main/scala-3/munit/internal/Compat.scala
index 7e489e2..92b507d 100644
--- a/munit/shared/src/main/scala-3/munit/internal/Compat.scala
+++ b/munit/shared/src/main/scala-3/munit/internal/Compat.scala
@@ -7,7 +7,10 @@ object Compat {
   val LazyList = scala.LazyList
   def productElementNames(p: Product): Iterator[String] =
     p.productElementNames
-  def collectionClassName(i: Iterable[_]): String = {
-    i.asInstanceOf[{ def collectionClassName: String }].collectionClassName
+  inline def collectionClassName(i: Iterable[_]): String = {
+    if PlatformCompat.isNative then
+        i.getClass.getName // or just "Iterable" ?
+    else
+        i.asInstanceOf[{ def collectionClassName: String }].collectionClassName
   }
 }

@kpodsiad
Copy link
Member Author

I spoke with Wojciech and there should be the Scala Native release at the end of this week. That release will contain fixes for problems spotted here, so I'll wait for it.

@armanbilge
Copy link
Contributor

That release will contain fixes for problems spotted here, so I'll wait for it.

Do you mean for CrossVersion.for3Use2_13? I must strongly recommend against using this technique.

When a widely-used library (such as scalacheck) is depended on with CrossVersion.for3Use2_13 by another widely-used library (such as munit) this creates a binary-breaking division in the ecosystem: even after scalacheck is properly published for Native/Scala 3, this artifact is incompatible with the Native/2.13 artifact that munit is using, and thus the ecosystem will be split on this dependency.

For an example of how this plays out, I strongly recommend you read this thread.

tl;dr scala-js-dom and all its downstream users (very nearly the entire Scala.js ecosystem) had to do a breaking version jump.

Further reading:

@kpodsiad
Copy link
Member Author

@armanbilge thanks for this information. Just to confirm, the preferable solution is waiting until scalacheck is published properly for scala-native/scala3? In every other approach, munit ends up depending on the _2.13 artifact.

@armanbilge
Copy link
Contributor

Just to confirm, the preferable solution is waiting until scalacheck is published properly for scala-native/scala3?

Yes, exactly. Preferable, as in doesn't break the ecosystem 😉

@SethTisue
Copy link
Contributor

I just merged some PRs in the ScalaCheck repo that may help get things moving again.

@kpodsiad
Copy link
Member Author

@SethTisue this PR still needs typelevel/scalacheck#868 (or equivalent) to be merged & new version released

@armanbilge
Copy link
Contributor

armanbilge commented Mar 15, 2022

@kpodsiad how would munit feel about scalacheck bumping to 3.1 on all platforms? See discussion in typelevel/scalacheck#868 (comment).

@kpodsiad
Copy link
Member Author

@armanbilge I'm afraid that I'm not the right person to ask.

@olafurpg @gabro could you share your opinion as creators and maintainers?

@gabro
Copy link
Member

gabro commented Mar 15, 2022

IIUC this would only affect MUnit + Scalacheck on Scala Native.

If that's the case then I think it's fine (I'm assuming we can keep using the current version of Scalacheck for jvm/js targets in order to preserve compatibility with 3.0)

The only drawback is that downstream libraries targeting e.g. jvm and native would need to do the same (either depend on different versions of MUnit or break compatibility with 3.0)

I'll let @olafurpg call the shots, but it wouldn't be a terrible thing in my opinion

@olafurpg
Copy link
Member

No objections from me to drop 3.0 support

@SethTisue
Copy link
Contributor

SethTisue commented Mar 15, 2022

IIUC this would only affect MUnit + Scalacheck on Scala Native
I'm assuming we can keep using the current version of Scalacheck for jvm/js targets in order to preserve compatibility with 3.0

one would assume so (I did too), but the assumption was called into question by @armanbilge at typelevel/scalacheck#868 (comment) , where he wrote:

these sorts of hacks seem to cause problems see e.g. portable-scala/sbt-crossproject#130

how strong an objection that is, I'm not sure — if we run into trouble, would it be minor trouble we could work around, or really serious trouble?

anyway, I hope that clarifies why Arman chose this phrasing above:

how would munit feel about scalacheck bumping to 3.1 on all platforms?

note the "on all platforms".

@armanbilge
Copy link
Contributor

No one's talked me down, so let's escalate 😉

@armanbilge
Copy link
Contributor

Scalacheck has now published a snapshot with Scala 3 / native support! I expect a stable release is imminent.

https://s01.oss.sonatype.org/content/repositories/snapshots/org/scalacheck/scalacheck_native0.4_3/1.16-0ac8005-SNAPSHOT/

@armanbilge
Copy link
Contributor

Your move! 😁

https://repo1.maven.org/maven2/org/scalacheck/scalacheck_native0.4_3/

Kamil Podsiadlo added 2 commits April 8, 2022 10:01
Drop 3.0.x and pretend it never existed, use 3.1.x for JVM, JS and Native platforms
Scala 3 doesn't allow to call method with parentheses without them. Normally class can extends trait/interface and overwrite their def's using val/override val no matter if def was declared with or without parentheses. However for Scala 3 this is no llonger true. This PR uses a workaround with underscores.
@kpodsiad
Copy link
Member Author

kpodsiad commented Apr 8, 2022

@armanbilge Could you help me with JS tests? They suddenly started failing after I bumped scala-js to the 1.9.0. Although there is a clue what to do, I don't why it's needed at all and that's why I would appreciate help from someone with any scala.js experience ;)

@sjrd
Copy link

sjrd commented Apr 8, 2022

You're running into the fixes with compatibility concerns about regular expressions in Scala.js 1.7.0. There are suggested strategies on how to avoid MULTILINE in the regular expressions doc page of Scala.js.

This is not my repo, but I would humbly suggest upgrading Scala.js in a separate PR to be able to address these changes independently of the larger changes in this PR.

@sjrd
Copy link

sjrd commented Apr 8, 2022

It seems that the problematic regular expressions is at

val elapsedTimePattern =
Pattern.compile(" \\d+\\.\\d+s ?", Pattern.MULTILINE)

This regexp doesn't even use ^ nor $, and is only used with replaceAll (so not any anchored situation). The MULTILINE flag has no effect in that case, so it can safely be removed without any other change.

@armanbilge
Copy link
Contributor

@kpodsiad does Seb's suggestion fix it (thank you!)? Otherwise I'll try and looking into it a bit later, I would really like to unblock this :)

@kpodsiad
Copy link
Member Author

kpodsiad commented Apr 8, 2022

@armanbilge I'll try to look at this tomorrow or, more probably, next week.

Because of with Scala.js 1.8+ in
Scalacheck 1.16.0 is binary compatible with the 1.15.x and 1.14.x series. It is published for Scala 2.12, 2.13, and 3.1+ with Scala.js 1.8+ and Scala Native 0.4. This release is the first to support Scala 3 on the Native platform.

it seems like I need to either:

  • leave the old version of scalacheck for JS and use the newly published 1.16.0 for Native and JVM.
  • just add the linkerConfig for tests (scalaJSLinkerConfig ~= { _.withESFeatures(_.withESVersion(ESVersion.ES2018)) }) because it's needed only in tests and it won't affect library users I think.

@armanbilge
Copy link
Contributor

Hum, if I understand #477 (comment) correctly you just need to remove that MULTILINE and it should work.

Ok, I'll give it a try.

@armanbilge
Copy link
Contributor

Woops, I also see that this PR is targeting main instead of a series/0.7 branch. If these changes are released as a 1.0 milestone, then most of the ecosystem won't be able to upgrade.

@armanbilge
Copy link
Contributor

Ok, put up a PR at:

@armanbilge
Copy link
Contributor

armanbilge commented Apr 8, 2022

Hum, the multiline flag is needed. The suggestion to bump the ES version for tests-only would work without affecting downstreams, but it would prevent your tests from being able to catch uses of ES2018 features in the published code. Which is not so great.

scalaJSLinkerConfig ~= { _.withESFeatures(_.withESVersion(ESVersion.ES2018)) }

Update: my bad, the regex was different in 0.7.29 which is why MULTILINE was required. Seb is right, as always 😉

@kpodsiad kpodsiad marked this pull request as ready for review April 9, 2022 13:21
@kpodsiad
Copy link
Member Author

kpodsiad commented Apr 9, 2022

@olafurpg approval will be gladly welcomed to fulfill formalities ;)

@armanbilge
Copy link
Contributor

Woops, fyi we've got conflicts now.

@valencik
Copy link
Collaborator

Sorry, merge conflicts are my fault. I'll resolve those and then merge once CI passes.

Copy link
Collaborator

@valencik valencik left a comment

Choose a reason for hiding this comment

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

After some discussion in Discord yesterday we're going to go ahead with this targeting 1.0 and soon cut a stable v1.0

These changes look reasonable to me, and CI is a nice shade of green :)

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.

9 participants