Fix windows CI

[BarkingBad]

[test_windows_full]

[philwalk]

This change to relpath in ClasspathTests.scala is not quite right, because it produces a path with backslashes, which cannot be passed as-is to bash:

   val relpath = some(testscript.topath.relpath.norm).map {
      case s if cygwin => s"$$(cygpath -aw $s)"
      case s => s
    }.get

The backslash is avoided by passing -am to cygwin instead:

   val relpath = some(testscript.topath.relpath.norm).map {
      case s if cygwin => s"$$(cygpath -am $s)"
      case s => s
    }.get

If the test is run in a cygwin environment, it fails like this:

$ bash -c compiler/target/scala-3.1.1-RC1/test-classes/scripting/classpathReport.sc
env: unknown option -- S
Try '/usr/bin/env --help' for more information.

It passes in my msys64 shell (after switching from cygpath -aw to cygpath -am).

I just realized, we don't need to use cygpath when we're passing a file to bash (it already does the conversion internally).

[philwalk]

the problem is with path construction. Apparently, we were going up in the file system tree unnecessarily while using the relativize method.

What is an example of the path construction problem? In this case, where the path is consumed by cygwin bash, the only example I can't think of where cygpath adjustments to the path would be useful is if the path has backslashes, or if it has embedded spaces. For example:

cygpath -dm "C:\Program Files"

converts to an equivalent path without spaces:

C:/PROGRA~1

[BarkingBad]

Sorry, I deleted my comment as I realized I was wrong, however, normalization at least helped, now it is failing indeed with the unrecognized -S option, not with file not found error

[philwalk]

Ok, good to know.
BTW, the reason for preferring a relative path is to avoid lots of corner cases where Windows drive letters complicate things.

[BarkingBad]

So I see that the current problem is that we should fix somehow passing arguments.
The question is, whether your proposition will work?

    -cp*|-classpath*) # partial fix for #10761
      # hashbang can combine args, e.g. "-classpath 'lib/*'"
      CLASS_PATH="${1#* *}${PSEP}"
      let class_path_count+=1
      shift
      ;;

If I am correct we will get arguments as solid string, so I feel like we should try to split the string into multiple args by white spaces rather than to add this additional case, because we will never enter that

[philwalk]

If I am correct we will get arguments as solid string

This strips -classpath from the argument -classpath 'lib/*', resulting in CLASS_PATH='lib/*'.

I'm not sure it is sufficient as-is, however, so you're probably right.
We should fix it in MainGenericRunner by recognizing and spliting these combined strings.

[BarkingBad]

If I am correct we will get arguments as solid string

This strips -classpath from the argument -classpath 'lib/*', resulting in CLASS_PATH='lib/*'.

I'm not sure it is sufficient as-is, however, so you're probably right. We should fix it in MainGenericRunner by recognizing and spliting these combined strings.

But then we won't have env variables set up. We should split the $@ by spaces in dist/bin/scala though I don't know how to do that properly. Then we could iterate over them to pattern match env variables and pass all other settings like classpath etc.

[philwalk]

Ok, this should work (I will start testing it to verify):

    -cp*|-classpath*) # partial fix for #10761
      # separate hashbang-combined args "-classpath 'lib/*'"
      addScala "-classpath"
      addScala "${1#* *}${PSEP}"
      shift
      ;;

In addition, we can't use /usr/bin/env in the shebang line. This should work, I';; verify

#!dist/target/pack/bin/scala -classpath 'lib/*'

[BarkingBad]

It works partially, but for script like this:

#!bin/scala -classpath 'dist/target/pack/lib/*' -Dkey=value

def main(args: Array[String]): Unit =
  println("Hello " + util.Properties.propOrNull("key"))

It won't process the -Dkey=value part because it will never hit it. We have to somehow split all the args by white space from $@ and then we will have general solution for as many arguments as we want

[philwalk]

split all the args by white space

That would prevent scala from passing arguments (e.g., filenames) with embedded whitespace.
For now, we can address only the case where an argument contains -classpath plus whitespace.
The reason for the importance of that use-case is because otherwise, it's impossible to create scala scripts with non-standard imports.

So I'm proposing this: if an argument matches "-cp|-classpath*)", then we split the entire argument on whitespace and replace $1 with its substrings in "$@".

The version handles -Dkey=value:

    -classpath*)
      if [ "$1" != "${1##* }" ]; then
        # hashbang-combined args "-classpath 'lib/*'"
        A=$1 ; shift # consume $1 before adding its substrings back
        set -- $A "$@" # split $1 on whitespace and put it back
      else
        addScala "$1"
        shift
      fi
      ;;

[BarkingBad]

People cheat this with wrapper workaround, in our case it would be:

#!/bin/bash
exec bin/scala -classpath 'dist/target/pack/lib/*' -Dkey=value path/to/script

[philwalk]

People cheat this with wrapper workaround, in our case it would be:

#!/bin/bash
exec bin/scala -classpath 'dist/target/pack/lib/*' -Dkey=value path/to/script

This works too

[philwalk]

wow, I accidentally hit [CLOSE WITH COMMENT] and there's no chance to cancel !
I cancelled the unintended new test run to conserver resources.

[philwalk]

It won't process the -Dkey=value part because it will never hit it. We have to somehow split all the args by white space from $@ and then we will have general solution for as many arguments as we want

Here's a proposal: in dist/bin/scala, if an argument contains whitespace, and in addition, at least one of the sub-arguments (when splitting it on whitespace) begins with a hyphen, then we replace the argument with the split sub-arguments in "$@".

[BarkingBad]

It won't process the -Dkey=value part because it will never hit it. We have to somehow split all the args by white space from $@ and then we will have general solution for as many arguments as we want

Here's a proposal: in dist/bin/scala, if an argument contains whitespace, and in addition, at least one of the sub-arguments (when splitting it on whitespace) begins with a hyphen, then we replace the argument with the split sub-arguments in "$@".

This what I was thinking about too. The only question is whether $# will be replaced too. Currently I am experimenting with your proposal and observed two interesting things:

• $# is still set to count = 1 so we don't process further arguemtns after splitting
• the * is being unglobbed. When I echo args I pass in bash script I get raw dist/target/pack/lib/* but in `MainGenericRunner when I print out args I get list of all matching jars.

[BarkingBad]

If you have an idea how to fix the shebang issue with splitting the args please commit to this branch as here we have windows CI test set up. I will try to do that too but I am not very proficient with bash

[BarkingBad]

EDIT: I haven't seen your proposal. I think that if it works it is close enough to be accepted, cause it is hard to propose something more clever in this situation. I will check if CI passes

[philwalk]

This version handles -Dkey=value and all other args, as long as it begins with -classpath:

    -classpath*)
      if [ "$1" != "${1##* }" ]; then
        # hashbang-combined args "-classpath 'lib/*'"
        A=$1 ; shift # consume $1 before adding its substrings back
        set -- $A "$@" # split $1 on whitespace and put it back
      else
        addScala "$1"
        shift
      fi
      ;;

However, I'm now grappling with another issue (although it might be unrelated).
With these changes (including changes the shebang line (#!bin/scala -classpath ...), we end up with the following java command line being generated by dist/bin/scala:

java [...] -classpath $jvm_cp_args \
dotty.tools.MainGenericRunner  \
-classpath $jvm_cp_args \
-classpath dist/target/pack/lib/* \
 compiler/target/scala-3.1.1-RC1/test-classes/scripting/classpathReport.sc

The problem is that the script only sees $jvm_cp_args, so the test fails:

[info] Test run finished: 1 failed, 0 ignored, 2 total, 5.332s
[error] Failed: Total 2, Failed 1, Errors 0, Passed 1
[error] Failed tests:
[error]         dotty.tools.scripting.ClasspathTests
[error] (scala3-compiler / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 7 s, completed Dec 14, 2021, 8:25:29 AM
sbt:scala3>

I assume the problem is in MainGenericRunner, or possibly further down the pipe. All classpaths after the first might be discarded (at one time, there was a warning, but it might still be (silently) discarding things). The proper fix would be to append the additional classpath each time. Otherwise, a script is never able to affect its classpath.

which part is confusing? The bash script?

[philwalk]

question is whether $# will be replaced too

This example code will insert several arguments at the head of $@ and as a side-effect, it will modify $#.

set -- newarg1 "$@"
set -- newarg2 "$@"
set -- newarg3 newarg4 $@

If $# started out zero, the new value is 4, and $@ is "newarg3" "newarg4" "newarg2" "newarg1"

So these two lines:

        A=$1 ; shift # consume $1 before adding its substrings back
        set -- $A "$@" # split $1 on whitespace and put it back

have the following effect:

1. remove "$1" from $@
2. split $A (because it's not quoted)
3. insert the subtstrings of $A back into $@, updating $# in the process

Afterwards, the split arguments will be parsed in order, as intended.

[BarkingBad]

Since test and test_windows_full passed I think we should merge it, even though it is a workaround, and fix it in some future PR. The second problem, with test_non_bootstrapped should be fixed by your @philwalk second PR with refactoring. I still think that after merging this PR we should close your PR and open a new one with:

[test_windows_full]
[test_non_bootstrapped]

to make sure that everything will work OK. Make sure to include the change to cwd property with normalize function I made in this PR (I see that you refactored these things) since it is required for Windows platforms (and is more concise with the logic of paths). Is that OK?

[philwalk]

Since test and test_windows_full passed I think we should merge it, even though it is a workaround, and fix it in some future PR. The second problem, with test_non_bootstrapped should be fixed by your @philwalk second PR with refactoring. I still think that after merging this PR we should close your PR and open a new one with:

[test_windows_full]
[test_non_bootstrapped]

to make sure that everything will work OK. Make sure to include the change to cwd property with normalize function I made in this PR (I see that you refactored these things) since it is required for Windows platforms (and is more concise with the logic of paths). Is that OK?

I'm investigating. It sounds good, although this problem with ClasspathTests is not yet fixed.
In order to push a PR anytime soon, I will disable the failing test so we can check the other fixes in.

[BarkingBad]

To disable tests one cpuld remove dist/pack from the CI, both test_windows_full and test check. Then all tests will silently fail as they were doing for a long time, but you can add @Ignore annotation as well

[philwalk]

To disable tests one cpuld remove dist/pack from the CI, both test_windows_full and test check. Then all tests will silently
fail as they were doing for a long time, but you can add @Ignore annotation as well

I am creating a new PR, so I'll be able to trigger the additional tests.
I'll include both of these:

[test_windows_full]
[test_non_bootstrapped]

The new PR is #14113

[BarkingBad]

Ok, I'm closing this one then