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

dotty.tools.scripting.BashScriptsTests are not running properly, but tests are succeeding #12962

Closed
magnolia-k opened this issue Jun 28, 2021 · 32 comments · Fixed by #12988
Closed

Comments

@magnolia-k
Copy link
Contributor

[info] Test run started
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaArgs started
osname[linux]
bashExe: [/bin/bash]

/bin/bash: /home/vagrant/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc: Permission denied
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalacArgs started
osname[linux]
bashExe: [/bin/bash]

/bin/bash: -script: command not found
[info] Test run finished: 0 failed, 0 ignored, 2 total, 0.061s

BashScriptsTests uses the which function provided by ClasspathTests to get the path of scala and scalac commands, but this function depends on the environment variable PATH, so it cannot get the path of the built binary.

As a result, the which function returns an empty string, and BashScriptsTests interprets the following first argument as a command and tries to execute it.

39   val scalacPath = which("scalac")
40   val scalaPath = which("scala")
@dwijnand
Copy link
Member

This is in CI (as opposed to just when tests are run locally), so I think it merits a priority bump.

@dwijnand
Copy link
Member

@philwalk do you think this is an easy fix?

@philwalk
Copy link
Contributor

philwalk commented Jun 30, 2021

I'll look at today, it sounds like an easy fix.

@griggt griggt linked a pull request Jun 30, 2021 that will close this issue
@magnolia-k
Copy link
Contributor Author

@philwalk @dwijnand

Looking at the results of the CI run, the path seems to be incorrect

osname[linux]
bashExe: [/usr/bin/bash]
[info] Test run started
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaArgs started

/usr/bin/bash: dist/target/pack/bin/scala: No such file or directory
scalacPath[dist/target/pack/bin/scalac]
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalacArgs started
osname[linux]
bashExe: [/usr/bin/bash]

/usr/bin/bash: dist/target/pack/bin/scalac: No such file or directory
[info] Test run finished: 0 failed, 0 ignored, 2 total, 0.029s

@dwijnand dwijnand reopened this Jul 6, 2021
@philwalk
Copy link
Contributor

philwalk commented Jul 6, 2021

I'm not yet familiar with the CI run, is scala already installed somewhere that we can look for at test time? I will look into it when I get a chance. If you can you point me to the section in the logs where the error message occurs, that would help.

The directory dist/target/pack/bin only exists after sbt dist/pack, can that be added in the CI environment?
Otherwise, the fix might be to disable the test if dist/target/pack/bin/scalac is not found.

@magnolia-k
Copy link
Contributor Author

It will not be generated unless dist/pack is executed between compilation(compile) and testing(test).

.github/workflows/ci.yaml

https://github.com/lampepfl/dotty/blob/504b1c2f596a6e3ed21fc6d841322e027b97c1f2/.github/workflows/ci.yaml#L67-L70

@philwalk
Copy link
Contributor

philwalk commented Jul 6, 2021

So should .github/workflows/ci.yaml be modified?
Or should the test be conditional on finding bin/scalac?

@griggt
Copy link
Collaborator

griggt commented Jul 6, 2021

Other tests that require dist/pack as a prerequisite are run by ./project/scripts/bootstrapCmdTests:

https://github.com/lampepfl/dotty/blob/601184787508d8fd655f1dc4e48ed19c418fa710/project/scripts/bootstrapCmdTests#L18-L19

I'm not sure if these tests are suitable (or can be adapted) to be run from there also.

@philwalk
Copy link
Contributor

philwalk commented Jul 6, 2021

The purpose of dotty.tools.scripting.BashScriptsTests is to verify that dist/bin/scala and dist/bin/scalac properly parse command line arguments passed to them. There were some bugs that caused arguments intended for scala scripts to be consumed within dist/bin/scala and/or dist/bin/scalac.

I think the easiest fix here is to have BashScriptsTests verify whether dist/target/pack/bin/scala is present before running the test.

@magnolia-k
Copy link
Contributor Author

magnolia-k commented Jul 7, 2021

If the existence of dist/target/pack/bin/scala determines whether or not tests need to be run, CI will not run BashScriptsTests all the time (since dist/pack is only run in cmdTest after sbt test).

I think that guaranteeing the order in which BashScriptsTests are executed after dist/pack will help stabilize the tests.

@dwijnand
Copy link
Member

dwijnand commented Jul 7, 2021

Moreover I would love to see a change such that the failure is fatal to the test/CI. 🙏🏼

magnolia-k added a commit to magnolia-k/dotty that referenced this issue Jul 7, 2021
magnolia-k added a commit to magnolia-k/dotty that referenced this issue Jul 7, 2021
magnolia-k added a commit to magnolia-k/dotty that referenced this issue Jul 7, 2021
@philwalk
Copy link
Contributor

philwalk commented Jul 7, 2021

Moreover I would love to see a change such that the failure is fatal to the test/CI. 🙏🏼

It seems possible that we could just modify .github/workflows/ci.yaml, by replacing this line:

./project/scripts/sbt ";compile ;test"

with this:

./project/scripts/sbt ";dist/pack ;test"

I'm not sure how to run the CI build on my system in order to test this approach. but I'm happy to learn, given some documentation.

@dwijnand
Copy link
Member

dwijnand commented Jul 7, 2021

That would ensure that the test passes. What I meant is the fact that with the previous PR the test was still silently failing.

@magnolia-k
Copy link
Contributor Author

I think the test should fail if the scala command does not exist on the path.

@philwalk
Copy link
Contributor

philwalk commented Jul 7, 2021

That would ensure that the test passes. What I meant is the fact that with the previous PR the test was still silently failing.

Ok, I think I understand now.

I think the test should fail if the scala command does not exist on the path.

That makes sense. I should be able to push something soon.

Here's the proposed fix: #13029

@griggt
Copy link
Collaborator

griggt commented Jul 7, 2021

It seems possible that we could just modify .github/workflows/ci.yaml, by replacing this line:

./project/scripts/sbt ";compile ;test"

with this:

./project/scripts/sbt ";dist/pack ;test"

That line is part of the test_non_bootstrapped job in CI. Modifying it to include dist/pack would force a build of the bootstrapped compiler, which seems undesirable.

@philwalk
Copy link
Contributor

philwalk commented Jul 7, 2021

With the new PR, there is a failure for one of the tests:

Cmd Tests failure

@magnolia-k
Copy link
Contributor Author

@griggt

BashScriptsTests should be changed to run on test instead of test_non_bootstrapped for CI purposes?

@philwalk
Copy link
Contributor

BashScriptsTests should be changed to run on test instead of test_non_bootstrapped for CI purposes?

Just an FYI, the only thing this test does is to run a script that prints all command line arguments to STDOUT, for the purpose of verifying that dist/bin/scala and dist/bin/scalac are not misappropriating any of the arguments. It seems unlikely (but not impossible) that it would fail in some environments and succeed in others, so it might not be worth the effort to change the runtime environment.

@magnolia-k
Copy link
Contributor Author

@philwalk

Executable scala commands are not obtained until dist/pack is run and dist/target/pack/bin/scala is created.
project/Build.scala assumes that the tests for the built Scala compiler will be run in sbt scala3-bootstrapped/test or project/scripts/bootstrapCmdTests

If BashScriptsTests is supposed to test against dist/target/pack/bin/scala, then dist/pack will be executed in project/scripts/bootstrapCmdTests and dist/target/pack/bin/scala will be created, so the tests corresponding to BashScriptsTests should be run after dist/pack in project/scripts/bootstrapCmdTests is executed.

Rather than the test environment, I think the issue is whether or not to run the test in a bootstrapped state.

@philwalk
Copy link
Contributor

@magnolia-k
Thanks for the explanation.

@philwalk
Copy link
Contributor

philwalk commented Jul 14, 2021

@magnolia-k

Rather than the test environment, I think the issue is whether or not to run the test in a bootstrapped state.

How to detect whether the test is running in a bootstrapped state? Is it indicated by the value of property "user.dir", or is there another way?

BarkingBad pushed a commit to BarkingBad/dotty that referenced this issue Jul 23, 2021
@michelou
Copy link
Collaborator

michelou commented Jul 31, 2021

IMO the 3 test files BashScriptTests.scala, ClasspathTests and ScriptingTests.scala in directory scripting/ need some code refactoring (to be done in several steps, preferably). In particular,

  • quite a lot of common code can be shared between all 3 test files.
  • the handling of file paths when calling C:/cygwin64/bin/bash.exe is not correct because the current directory changes when we leave the Windows command prompt.
  • the variable environment JAVA_HOME needs to be set before running the tests, otherwise bash uses the first java.exe executable available from PATH when running the tests (which may differ from the build settings, e.g. I generate snapshots for Java 8, 11 and 17, see OPENJDK.md for more details).

Here is what I see from the output of the CI job test_windows_full mentioned in issue 13176 :

716 [info] Test run started
717 [info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaArgs started
718 osname[windows 10]
719 bashExe: [C:/cygwin64/bin/bash.exe]
720 
721 /usr/bin/bash: dist/target/pack/bin/scala: No such file or directory
722 [info] Test dotty.tools.scripting.BashScriptsTests.verifyScalacArgs started
723 scalacPath[dist/target/pack/bin/scalac]
724 osname[windows 10]
725 bashExe: [C:/cygwin64/bin/bash.exe]
726 
727 /usr/bin/bash: dist/target/pack/bin/scalac: No such file or directory
728 [info] Test run finished: 0 failed, 0 ignored, 2 total, 0.078s

And here what I get in my local build on a Win10 laptop after fixing the file paths (both executables and file arguments) :

[info] Test run started
realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scalac]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scala]
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScriptPathProperty started
realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/scriptPath.sc]
===> verify valid system property script.path is reported by script [scriptPath.sc]
realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scalac]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scala]
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaArgs started

expected: arg  0:[a]
actual  : arg  0:[a]
expected: arg  1:[b]
actual  : arg  1:[b]
expected: arg  2:[c]
actual  : arg  2:[c]
expected: arg  3:[-repl]
actual  : arg  3:[-repl]
expected: arg  4:[-run]
actual  : arg  4:[-run]
expected: arg  5:[-script]
actual  : arg  5:[-script]
expected: arg  6:[-debug]
actual  : arg  6:[-debug]
realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scalac]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scala]
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaOpts started
realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/classpathReport.sc]
===> verify valid system property script.path is reported by script [classpathReport.sc]
stderr [unable to execute script, return value is 126
/bin/bash: /mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/classpathReport.sc: dist/target/pack/bin/scala: bad interpreter: No such file or directory]
realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scalac]
realPath : [/mnt/c/dotty/dist/target/pack/bin/scala]
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalacArgs started
scalacPath[/mnt/c/dotty/dist/target/pack/bin/scalac]

expected: arg  0:[a]
actual  : arg  0:[a]
expected: arg  1:[b]
actual  : arg  1:[b]
expected: arg  2:[c]
actual  : arg  2:[c]
expected: arg  3:[-repl]
actual  : arg  3:[-repl]
expected: arg  4:[-run]
actual  : arg  4:[-run]
expected: arg  5:[-script]
actual  : arg  5:[-script]
expected: arg  6:[-debug]
actual  : arg  6:[-debug]
[info] Test run finished: 0 failed, 0 ignored, 4 total, 12.863s

In short my local solution looks as follows :

  • I define a new function getRealPath(path: String): String which takes into account
    • relative paths vs. absolute paths and
    • virtual drives vs. physical drives (welcome to the Windows subst command !).
  • I write val scalacPath = getRealPath("dist/target/pack/bin/scalac") instead of just val scalacPath = "dist/target/pack/bin/scalac" (and so on).

@philwalk
Copy link
Contributor

  • because the current directory changes when we leave the Windows command prompt.

Can you clarify, or give an example? BTW, your proposal sounds good.

@philwalk
Copy link
Contributor

philwalk commented Aug 1, 2021

@michelou -
How should the test environment communicate where JAVA_HOME and dist/target/pack/bin are?

The current assumption in BashScriptTests.scala, etc. is that the current working directory is set to the parent of dist/target/pack', and that the jdkbin directory is the firstjavain thePATH`.

We could alternatively indicate where they are via environment variables or System properties.

@michelou
Copy link
Collaborator

michelou commented Aug 1, 2021

@philwalk Here is an example of the UnsupportedClassVersionError exception I get when generating the Scala 3 distribution for Java 8 (aka. 8u302), Java 11 (aka. 11.0.12) and Java 17 (aka. 17-ea_33) in the same run (using my batch file snapshot.bat) :

[...]
[info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaArgs started

Exception in thread "main" java.lang.UnsupportedClassVersionError: dotty/tools/dotc/interfaces/ReporterResult has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0
        at java.base/java.lang.ClassLoader.defineClass1(Native Method)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
        at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
        at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800)
        at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:698)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
        at java.base/java.lang.ClassLoader.defineClass1(Native Method)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
        at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
        at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800)
        at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:698)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
        at dotty.tools.scripting.Main$.main(Main.scala:34)
        at dotty.tools.scripting.Main.main(Main.scala)
[...]

NB. Java class file versions : 61 ➞ Java 17, 55 ➞ Java 11, 52 ➞ Java 8.

And here is the version of Java found in PATH when executing bash.exe on my Win10 laptop :


> C:\Windows\System32\bash.exe -c "which java"
/usr/bin/java

> C:\Windows\System32\bash.exe -c "java -version"
openjdk version "11.0.11" 2021-04-20
OpenJDK Runtime Environment (build 11.0.11+9-Ubuntu-0ubuntu2.18.04)
OpenJDK 64-Bit Server VM (build 11.0.11+9-Ubuntu-0ubuntu2.18.04, mixed mode, sharing)

NB. On MS Windows 10 Java 8 is installed by default in the WSL Ubuntu distribution. In my case I upgraded my installation to Java 11 to get rid of the issue between Java 8 and Java 11 (but now I have the same issue between Java 11 and Java 17) when running the scripting tests.

@philwalk
Copy link
Contributor

philwalk commented Aug 1, 2021

@michelou -
From the look of your realPath log messages, it seems you are wanting to use WSL Ubuntu bash, which is why the current working directory is shown as /mnt/c/dotty:

realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc]

Is that correct?
In addition, It seems you are building against 3 different versions of Windows java, so the path to JAVA_HOME would need to be something like:

/mnt/c/Program Files/Java/jdk-11.0.12

Correct?

FWIW, the following approach might help, although I'm also thinking there needs to be an easy way for tests to query the test environment for the correct settings of JAVA_HOME, SCALA_HOME, cwd, etc.

> C:\Windows\system32>C:\Windows\System32\bash.exe -c "export JAVA_HOME='/mnt/c/opt/jdk'; export PATH=\"/mnt/c/opt/jdk/bin:$PATH\"; which java.exe"

@philwalk
Copy link
Contributor

philwalk commented Aug 2, 2021

@michelou -
I did some cleanup and refactoring of BashScriptTests.scala.

You can now specify the working directory, JAVA_HOME and/or SCALA_HOME for running tests.

TEST_BASH=/mnt/c/Windows/System32/bash.exe
TEST_CWD=/mnt/c/dotty
JAVA_HOME='/mnt/c/Program Files/Java/jdk-11.0.12'
SCALA_HOME='/mnt/c/dotty/dist/target/pack'
PATH="$JAVA_HOME/bin:$SCALA_HOME/bin:$PATH"

If JAVA_HOME is defined, then JAVA_HOME/bin will be added to the PATH.
Likewise with SCALA_HOME, although SCALA_HOME is ignored in favor dist/target/pack if present below the current working directory.

I haven't yet refactored ClasspathTests or ScriptingTests.scala
Let me know if something additional or different would help.

@michelou
Copy link
Collaborator

michelou commented Aug 3, 2021

@michelou -
From the look of your realPath log messages, it seems you are wanting to use WSL Ubuntu bash, which is why the current working directory is shown as /mnt/c/dotty:

realPath : [/mnt/c/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc]

Is that correct?

Right, my function getRealPath do prepend /mnt/ to the paths before calling C:\Windows\System32\bash.exe.

In addition, It seems you are building against 3 different versions of Windows java, so the path to JAVA_HOME would need to be something like:

/mnt/c/Program Files/Java/jdk-11.0.12

Correct?

Right. All my Java implementations are actually installed in directory c:\opt\ (as explained on my page OPENJDK.md).

FWIW, the following approach might help, although I'm also thinking there needs to be an easy way for tests to query the test environment for the correct settings of JAVA_HOME, SCALA_HOME, cwd, etc.

> C:\Windows\system32>C:\Windows\System32\bash.exe -c "export JAVA_HOME='/mnt/c/opt/jdk'; export PATH=\"/mnt/c/opt/jdk/bin:$PATH\"; which java.exe"

@philwalk
Copy link
Contributor

philwalk commented Aug 24, 2021

IMO the 3 test files BashScriptTests.scala, ClasspathTests and ScriptingTests.scala in directory scripting/ need some code refactoring (to be done in several steps, preferably). In particular,

  • quite a lot of common code can be shared between all 3 test files.
  • the handling of file paths when calling C:/cygwin64/bin/bash.exe is not correct because the current directory changes when we leave the Windows command prompt.
  • the variable environment JAVA_HOME needs to be set before running the tests, otherwise bash uses the first java.exe executable available from PATH when running the tests (which may differ from the build settings, e.g. I generate snapshots for Java 8, 11 and 17, see OPENJDK.md for more details).

@michelou - are you able to achieve what you need with the most recent changes to BashScriptsTests.scala in the PR? If so, I can similarly refactor other tests. Otherwise, let me know what other changes you require.

anatoliykmetyuk added a commit that referenced this issue Sep 23, 2021
…s-12962

fail verifyScalacArgs if dist/target/pack/bin/scalac not found - fix for #12962
@philwalk
Copy link
Contributor

I think this issue is resolved. Are there any residual concerns?

@dwijnand
Copy link
Member

Resolved until proved guilty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants