Bundle scala cli in scala command #20351

bishabosha · 2024-05-07T10:00:06Z

fixes #20098

Proposed changes to zip/targz archive:

in the /bin directory store an extra launcher for Scala CLI (either JAR, or native per platform).
/bin/scala[.bat] is modified to invoke Scala CLI stored in /bin
new /maven2 directory, which stores all the Jars and POM files necessary (in maven repo style) for scala-cli to invoke scala compiler offline (using the -r launcher option).
CHOICE: either replace jar files in /lib by aliases to the corresponding jar in /maven2, OR delete /lib and update references from scripts. (Looks like symlinks are not portable, so probably we should encode the classpath in a file, or adjust slightly how we build the toolchain)
add platform specific suffixes to artefacts:
- e.g. scala-3.5.0-x86_64-pc-linux.tar.gz (for the artefact that bundles the x64 linux launcher)

compiler/test-resources/scripting/showArgs.sc

sjrd

I can only comment on what's jumping at me. The timing is way too short to do a real review.

Even if I did find something it wouldn't get addressed and shipped in time at this point, so, to be blunt: why bother?

bin/scala

compiler/test/dotty/tools/scripting/ClasspathTests.scala

bishabosha · 2024-05-07T18:20:37Z

It appears that on windows a critical class is loaded on boot scala.build.internal.JniGetWinDirs which is compiled by JDK 16, so fails when loading from Java 8. But on Mac OS the jar launcher works fine on java 8 - could it be possible that just this one dependency has a misconfigured build?

Edit: We can not guarantee that the Jar-based launcher works on JDK 17, so this must be do-not-merge until we implement the native launchers

He-Pin · 2024-05-10T04:02:37Z

A question is if a new scala-cli is released, will scala update it automatically, like cs update scala-cli?

bishabosha · 2024-05-10T08:03:52Z

A question is if a new scala-cli is released, will scala update it automatically, like cs update scala-cli?

The way it is currently implemented, the idea would be that each scala version is tied to a specific Scala CLI launcher. and Each scala release would come with whatever is the latest Scala CLI version at that moment

He-Pin · 2024-05-15T21:08:09Z

When do republishing , is there some kind of Sha or md5 checking ?
Does Windows platform been well tested?

hamzaremmal · 2024-05-22T19:32:19Z

README.md needs to be updated too

He-Pin · 2024-05-22T23:30:26Z

project/DottyVersion.scala

+
+  val baseVersion = "3.5.0-RC1"
+
+  val referenceVersion = "3.4.2-RC1"


Is it right?

we need a rebase

bishabosha · 2024-05-30T09:07:41Z

I have manually squashed the commits where it makes sense

bin/scala

compiler/src/dotty/tools/MainGenericRunner.scala

hamzaremmal · 2024-06-05T14:27:48Z

compiler/test/dotty/tools/io/ClasspathTest.scala

@@ -23,7 +25,7 @@ class ClasspathTest {
  @Test def testWildcards(): Unit =
    val outDir = Files.createTempDirectory("classpath-test")
    try
-      val compilerLib = "dist/target/pack/lib"
+      val compilerLib = s"${if isWindows then "dist-win-x86_64" else "dist"}/target/pack/lib"


Why is that change needed ? What is so specific to windows that we don't need to handle for others

because windows runners don't work with the jvm - but it should probably be the native launcher for each platform

We should probably have tests for the jvm launcher and tests for all native launchers

but the jvm tests crash on windows because its JDK 8

What I meant is to run these tests with each native-launcher on each platform and then run the jvm-launcher on linux. But maybe it is a bit overkill to do that at the moment

Do we still care about testing the JVM launcher on Windows?

project/Modes.scala

project/Build.scala

project/scripts/buildScalaBinary

project/scripts/native-integration/winTests.bat

project/scripts/winCmdTests

project/scripts/winCmdTests.bat

Gedochao

LGTM
I just commented on some nitpicks, which can be addressed later in a future PR.

Gedochao · 2024-06-11T09:21:46Z

bin/scala

+scala_args() {
+
+  declare -a CLI_ARGS
+  declare -a SCRIPT_ARGS
+  declare DISABLE_BLOOP=1
+
+  while (( "$#" )); do
+    case "$1" in
+      "--")
+        shift
+        SCRIPT_ARGS+=("--")
+        SCRIPT_ARGS+=("$@")
+        break
+        ;;
+      "clean" | "version" | "--version" | "-version" | "help" | "--help" | "-help")
+        CLI_ARGS+=("$1")
+        DISABLE_BLOOP=0 # clean command should not add --offline --server=false
+        shift
+        ;;
+      *)
+        CLI_ARGS+=("$1")
+        shift
+        ;;
+    esac
+  done
+
+  if [ $DISABLE_BLOOP -eq 1 ]; then
+    CLI_ARGS+=("--offline" "--server=false")
+  fi
+
+  echo "--power ${CLI_ARGS[@]} ${SCRIPT_ARGS[@]}"


I still don't feel this is necessary... passing those arguments explicitly in the tests would make them more readable IMO, rather than embedding them in this script.
it's also easy to get confused where this script is being used.
This is a nitpick, however, we don't need to refactor it in this PR I think.

Gedochao · 2024-06-11T09:23:16Z

bin/test/TestScripts.scala

@@ -57,7 +57,7 @@ class TestScripts {
      s"bin/scalac script did not run properly. Output:$lineSep$dotcOutput"
    )

-    val (retDotr, dotrOutput) = executeScript("./bin/scala HelloWorld")
+    val (retDotr, dotrOutput) = executeScript("./bin/scala -M HelloWorld")


this is slightly confusing, since just passing -M to Scala CLI, without any inputs or -cp will just fail.
I know it doesn't fail because -cp is passed elsewhere, but this line looks like it should fail, technically.

Gedochao · 2024-06-11T09:24:47Z

compiler/src/dotty/tools/MainGenericRunner.scala

+      sys.props.isDefinedAt("coursier.mainJar")
+      || sys.props.get("bootstrap.mainClass").contains("dotty.tools.MainGenericRunner")
+
+    val silenced = sys.props.get("scala.use_legacy_launcher") == Some("true")


We may want to document this prop clearly somewhere later, once we release this.
I'm also thinking an env variable could potentially be useful?
LEGACY_SCALA_RUNNER=true or smth.

Gedochao · 2024-06-11T09:26:19Z

compiler/test-resources/scripting/classpathReport_scalacli.sc

 import java.nio.file.Paths

-def main(args: Array[String]): Unit =
+// def main(args: Array[String]): Unit = // MIGRATION: Scala CLI expects `*.sc` files to be straight-line code


I wonder if it makes sense to keep this comment, or can we just remove it.
it's stated earlier that this is a Scala CLI script.

Suggested change

// def main(args: Array[String]): Unit = // MIGRATION: Scala CLI expects `*.sc` files to be straight-line code

Gedochao · 2024-06-11T09:27:51Z

compiler/test/dotty/tools/io/ClasspathTest.scala

@@ -23,7 +25,7 @@ class ClasspathTest {
  @Test def testWildcards(): Unit =
    val outDir = Files.createTempDirectory("classpath-test")
    try
-      val compilerLib = "dist/target/pack/lib"
+      val compilerLib = s"${if isWindows then "dist-win-x86_64" else "dist"}/target/pack/lib"


Do we still care about testing the JVM launcher on Windows?

hamzaremmal · 2024-06-11T09:41:19Z

Do we still care about testing the JVM launcher on Windows?

(Not sure why I can't reply directly to your comment) We can't run JVM launchers on Windows due to the restriction on the Java version. We will keep releasing the JVM launcher anyways for other uses so it is good to test them too.

Adapts the workflow to the changes in #20351

fixes scala#20098 Proposed changes to zip/targz archive: - in the `/bin` directory store an extra launcher for Scala CLI (either JAR, or native per platform). - `/bin/scala[.bat]` is modified to invoke Scala CLI stored in `/bin` - new `/maven2` directory, which stores all the Jars and POM files necessary (in maven repo style) for scala-cli to invoke scala compiler offline (using the `-r` launcher option). - CHOICE: either replace jar files in `/lib` by aliases to the corresponding jar in `/maven2`, OR delete `/lib` and update references from scripts. (Looks like symlinks are not portable, so probably we should encode the classpath in a file, or adjust slightly how we build the toolchain) - add platform specific suffixes to artefacts: - e.g. `scala-3.5.0-x86_64-pc-linux.tar.gz` (for the artefact that bundles the x64 linux launcher) --------- Co-authored-by: Hamza REMMAL <[email protected]>

Backports #20351 to 3.5.0-RC2

This PR is based on #20351. The idea is to use `sbt-native-packager` instead of `sbt-pack` since it has the ability to generate `.msi` files. [test_msi]

bishabosha force-pushed the bundle-scala-cli branch from 1e4a234 to acbd467 Compare May 7, 2024 12:12

Gedochao requested review from Kordyjan and hamzaremmal May 7, 2024 12:29

Gedochao assigned hamzaremmal May 7, 2024

Gedochao requested a review from sjrd May 7, 2024 12:31

Gedochao assigned bishabosha May 7, 2024

Gedochao added the needs-minor-release This PR cannot be merged until the next minor release label May 7, 2024

bishabosha added the release-notes Should be mentioned in the release notes label May 7, 2024

hamzaremmal reviewed May 7, 2024

View reviewed changes

compiler/test-resources/scripting/showArgs.sc Show resolved Hide resolved

sjrd reviewed May 7, 2024

View reviewed changes

bin/scala Outdated Show resolved Hide resolved

compiler/test/dotty/tools/scripting/ClasspathTests.scala Outdated Show resolved Hide resolved

hamzaremmal approved these changes May 7, 2024

View reviewed changes

bishabosha unassigned hamzaremmal May 7, 2024

hamzaremmal added the stat:do not merge label May 7, 2024

Kordyjan added this to the 3.5.0 milestone May 10, 2024

This was referenced May 14, 2024

Hotfix to release 3.5.0-RC1 #20407

Merged

binary distribution: package information about jars required for scalac classpath in a separate file #20413

Closed

bishabosha force-pushed the bundle-scala-cli branch 7 times, most recently from cbcd575 to 46696d5 Compare May 21, 2024 14:55

He-Pin reviewed May 22, 2024

View reviewed changes

hamzaremmal force-pushed the bundle-scala-cli branch from 8ee4e6d to 863b28b Compare May 23, 2024 08:09

hamzaremmal requested changes Jun 6, 2024

View reviewed changes

scala deleted a comment from bishabosha Jun 8, 2024

Address the comments in the review

aeafc08

hamzaremmal force-pushed the bundle-scala-cli branch from 5f55261 to aeafc08 Compare June 8, 2024 11:09

hamzaremmal mentioned this pull request Jun 8, 2024

Adapt the workflow to release on SDKMAN! #20535

Merged

hamzaremmal requested review from Gedochao June 10, 2024 12:29

hamzaremmal assigned Gedochao and unassigned bishabosha Jun 10, 2024

hamzaremmal mentioned this pull request Jun 10, 2024

Use sbt-native-packager instead of sbt-pack #20547

Merged

Gedochao approved these changes Jun 11, 2024

View reviewed changes

Gedochao assigned hamzaremmal Jun 11, 2024

hamzaremmal removed the stat:do not merge label Jun 11, 2024

hamzaremmal merged commit f7ab683 into scala:main Jun 11, 2024
24 checks passed

hamzaremmal deleted the bundle-scala-cli branch June 11, 2024 09:42

hamzaremmal mentioned this pull request Jun 13, 2024

Adapt the release workflow to SIP-46 #20565

Merged

hamzaremmal added a commit that referenced this pull request Jun 15, 2024

Adapt the workflow to release on SDKMAN! (#20535)

48a823f

Adapts the workflow to the changes in #20351

WojciechMazur added a commit to WojciechMazur/dotty that referenced this pull request Jun 17, 2024

Adapts the workflow to the changes in scala#20351

03d8b87

WojciechMazur added a commit that referenced this pull request Jun 17, 2024

Adapts the workflow to the changes in #20351

edbb7c4

WojciechMazur added the backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" label Jun 20, 2024

WojciechMazur mentioned this pull request Jun 20, 2024

Backport "Bundle scala cli in scala command" to 3.5.0 #20706

Merged

WojciechMazur added a commit that referenced this pull request Jun 21, 2024

Backport "Bundle scala cli in scala command" to 3.5.0 (#20706)

6fd3950

Backports #20351 to 3.5.0-RC2

WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jun 21, 2024

hamzaremmal added a commit that referenced this pull request Jul 15, 2024

Use sbt-native-packager instead of sbt-pack (#20547)

a212ff0

This PR is based on #20351. The idea is to use `sbt-native-packager` instead of `sbt-pack` since it has the ability to generate `.msi` files. [test_msi]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle scala cli in scala command #20351

Bundle scala cli in scala command #20351

bishabosha commented May 7, 2024 •

edited

Loading

sjrd left a comment

bishabosha commented May 7, 2024 •

edited

Loading

He-Pin commented May 10, 2024

bishabosha commented May 10, 2024 •

edited

Loading

He-Pin commented May 15, 2024

hamzaremmal commented May 22, 2024

He-Pin May 22, 2024

bishabosha May 25, 2024

bishabosha commented May 30, 2024

hamzaremmal Jun 5, 2024

bishabosha Jun 7, 2024

hamzaremmal Jun 8, 2024

bishabosha Jun 8, 2024

hamzaremmal Jun 8, 2024 •

edited

Loading

Gedochao Jun 11, 2024

Gedochao left a comment

Gedochao Jun 11, 2024

Gedochao Jun 11, 2024

Gedochao Jun 11, 2024

Gedochao Jun 11, 2024

Gedochao Jun 11, 2024

hamzaremmal commented Jun 11, 2024


		val baseVersion = "3.5.0-RC1"

		val referenceVersion = "3.4.2-RC1"

Bundle scala cli in scala command #20351

Bundle scala cli in scala command #20351

Conversation

bishabosha commented May 7, 2024 • edited Loading

sjrd left a comment

Choose a reason for hiding this comment

bishabosha commented May 7, 2024 • edited Loading

He-Pin commented May 10, 2024

bishabosha commented May 10, 2024 • edited Loading

He-Pin commented May 15, 2024

hamzaremmal commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishabosha commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamzaremmal Jun 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gedochao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamzaremmal commented Jun 11, 2024

bishabosha commented May 7, 2024 •

edited

Loading

bishabosha commented May 7, 2024 •

edited

Loading

bishabosha commented May 10, 2024 •

edited

Loading

hamzaremmal Jun 8, 2024 •

edited

Loading