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

Make scripted test run with Scala 2.13 by default #1300

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Friendseeker
Copy link
Member

@Friendseeker Friendseeker commented Dec 3, 2023

Scala 2.13 is significantly more popular than Scala 2.12, hence it would cover more demographics for Zinc Scripted Test to run with 2.13 by default instead of 2.12.

scala/scala has scripted test running at 2.13, but ideally we catch 2.13 specific issue via Zinc CI directly, instead of relying on scala/scala CI.

@Friendseeker
Copy link
Member Author

Friendseeker commented Dec 3, 2023

Two test failures revealed by this PR

  • source-dependencies/compactify
  • source-dependencies/compactify-nested

@dwijnand Would you mind to give it a quick look? Just want to know if these are false positives or not.

@Friendseeker Friendseeker marked this pull request as ready for review December 3, 2023 05:57
@dwijnand
Copy link
Member

dwijnand commented Dec 4, 2023

No, they're true positives. But only because as of 2.13.12 we use in-sources compiler bridges (scala/scala#10472) and the fix that is merged here (#1259) has been merged upstream (scala/scala#10542) but hasn't been released yet.

@lrytz
Copy link
Contributor

lrytz commented Dec 4, 2023

@dwijnand are you sure we are using the binary bridge? Because IIUC that decision is not done by zinc, but by the build tool (https://github.com/sbt/sbt/blob/v1.9.7/zinc-lm-integration/src/main/scala/sbt/internal/inc/ZincLmUtil.scala#L93-L97).

In this case this would mean the scripted testing framework implementation within the zinc repo, no?

The two tests may be failing because -Xmax-classfile-name in 2.12 defaults to 255, but the setting was removed in 2.13 with a new default of 240 (scala/scala#7497).

This might be the reason for compactify-nested failing, @Friendseeker see https://github.com/scala/scala/blob/2.13.x/test/sbt-test/source-dependencies/compactify-nested/test for the 2.13 expected class file names.

Not sure about the compatcify test.

@@ -118,9 +118,12 @@ class IncHandler(directory: Path, cacheDir: Path, scriptedLog: ManagedLogger, co

def initBuildStructure(): Unit = {
val build = initBuild
val defaultScalaVersion = "2.13.12"
Copy link
Member Author

@Friendseeker Friendseeker Dec 5, 2023

Choose a reason for hiding this comment

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

Would it be better for defaultScalaVersion to be stored in sys.props instead?

e.g.

def scalaVersion =
sys.props
.get("zinc.build.compilerbridge.scalaVersion")
.getOrElse(sys.error("zinc.build.compilerbridge.scalaVersion property not found"))

Then we can conveniently setup CI to run scripted tests against both 2.12 & 2.13 in a clean manner. Despite 2.12 is phasing out, it might be still worthy it to keep 2.12 coverage for the slim chance that some issue only surface in 2.12.

Copy link
Member Author

@Friendseeker Friendseeker Dec 5, 2023

Choose a reason for hiding this comment

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

@xuwei-k Since you did lots of work on the CI & Testing side, would you mind to give a quick look on this PR? Your opinion would be valuable.

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.

None yet

4 participants