sbt / sbt-launcher-package

Packaging for sbt so you can run it.
http://scala-sbt.org/download.html
Apache License 2.0
89 stars 92 forks source link

Fix launcher option handling #332

Closed avdv closed 4 years ago

avdv commented 4 years ago

See sbt/sbt#5885

eed3si9n commented 4 years ago

@avdv Thanks for the contribution.

Could you add a test here please? - https://github.com/sbt/sbt-launcher-package/blob/master/integration-test/src/test/scala/RunnerTest.scala

avdv commented 4 years ago

Could you add a test here please? - https://github.com/sbt/sbt-launcher-package/blob/master/integration-test/src/test/scala/RunnerTest.scala

Sure, I am trying....

But, when running sbt -Dsbt.build.version=1.3.10 universal:packageBin universal:stage integrationTest/test I get the following:

[addJava] arg = '-XX:ReservedCodeCacheSize=128m'
[copyRt] java9_rt = '/home/claudio/.sbt/0.13/java9-rt-ext-adoptopenjdk_11_0_6/rt.jar'
[addJava] arg = '-Dscala.ext.dirs=/home/claudio/.sbt/0.13/java9-rt-ext-adoptopenjdk_11_0_6'
Error: Invalid or corrupt jarfile /home/claudio/github/sbt-launcher-package/target/universal/stage/bin/sbt-launch.jar
- quoted * should not glob expand to local files *** FAILED ***
  RuntimeException: Nonzero exit value: 1
    scala.sys.package$.error(package.scala:30)
    sbt.internal.AbstractProcessBuilder.getString(ProcessImpl.scala:131)
    sbt.internal.AbstractProcessBuilder.$bang$bang(ProcessImpl.scala:133)
...

It seems the sbt-launch.jar is just an empty file:

$ fd -u sbt-launch.jar -x file
target/sbt-launch.jar: empty
target/universal/stage/bin/sbt-launch.jar: empty
avdv commented 4 years ago

I added tests to both commits. Caveat: clean room implementation since I cannot run them... :wink:

avdv commented 4 years ago

I tried to download the sbt-launch.jar manually but also ran into problems...

Isn't it a bit crazy to start a JVM during the integration tests, only (mostly) to examine the arguments that are passed to the process?

I wrote a quick mock instead:

#!/usr/bin/env python

import sys

if '--version' in sys.argv or '-version' in sys.argv:
    print 'openjdk version "1.8.0_212"'
else:
    for arg in sys.argv[1:]:
        print(repr(arg))

Then, in SbtRunnerTest.scala:

  private val javaBinDir = new File(".", "integration-test" + File.separator + "bin").getAbsolutePath

  def sbtProcess(args: String*) = sbtProcessWithOpts(args: _*)("", "")
  def sbtProcessWithOpts(args: String*)(javaOpts: String, sbtOpts: String) = {
    val path = sys.env("PATH")
    sbt.internal.Process(Seq(sbtScript.getAbsolutePath) ++ args, new File("citest"),
      "JAVA_OPTS" -> javaOpts,
      "SBT_OPTS" -> sbtOpts,
      "PATH" -> (javaBinDir + File.pathSeparator + path)
    )
  }

That works for most of the tests in there, only 5 of them really expect to call the real sbt-launch.jar. Needless to say this is real quick too.

eed3si9n commented 4 years ago

Isn't it a bit crazy to start a JVM during the integration tests, only (mostly) to examine the arguments that are passed to the process?

Previously most of these bash/batch options didn't have any test at all and they kept failing on Mac, Linux, and Windows.. so the test is not ideal but it's far better than nothing. If you or anyone wants to improve the status quo, I'd be happy to accept changes.

avdv commented 4 years ago

Previously most of these bash/batch options didn't have any test at all and they kept failing on Mac, Linux, and Windows.. so the test is not ideal but it's far better than nothing. If you or anyone wants to improve the status quo, I'd be happy to accept changes.

I've separated the tests into different files depending on whether they really need to run the sbt-launch.jar or not. I could also split out those changes if you want.

avdv commented 4 years ago

@eed3si9n I think this should be ready to review. Note that I also removed the piccolo.link as those constantly returned an error:

$ curl https://piccolo.link/sbt-$SBT_VER.zip -L --output /tmp/sbt.zip
$ unzip /tmp/sbt.zip -d $HOME/sbt
Archive:  /tmp/sbt.zip
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.

Note, runtime on Travis is down from ~12 mins to ~9 mins.