Runner sets residual args instead of append

[som-snytt]

Fixes #14541

Quick tweak requires proper test.

Backported #14552

[nicolasstucki]

Here is an example of how to call the def main that is generated by the @main in a test.

https://github.com/lampepfl/dotty/pull/13727/files#diff-4c9963dd435c987466fdd17e4c3c0bfa9d5bf45e42f1c505870902e5981438c4R8-R30

[smarter]

Would you mind opening a backport of this fix to the release-3.1.2 branch too?

[som-snytt]

The test doesn't help, and MainGenericRunner isn't amenable to testing. I know there are script-level tests, so I'll track that down. In Scala 2, it figures out a class path programmatically rather than scriptedly.

Meanwhile, I'll learn how to backport to the release branch.

[smarter]

You can add shell script unit tests to https://github.com/lampepfl/dotty/blob/main/project/scripts/cmdTests

[smarter]

Oh but cmdTests uses sbt scalac, I don't know what's the best place to test the output of the actual scalac shell script /cc @BarkingBad

[BarkingBad]

If I got you correctly, you are looking for these tests https://github.com/lampepfl/dotty/tree/main/compiler/test/dotty/tools/scripting
I think @philwalk is the best person to consult with since he applied the most recent changes to these tests

[nicolasstucki]

Adding the following test in project/scripts/cmdTests does the appropriate test. Not sure if that is the place to use bin/scala

echo "testing bin/scala call to main"
clear_out "$OUT"
bin/scalac tests/run-with-compiler/i14541.scala
bin/scala echo hello world > "$tmp"
# cat "$tmp" # for debugging
grep -e "^hello world$" "$tmp"

[som-snytt]

Before

Output from 'tests/run-with-compiler/i14541.scala' did not match check file. Actual output:
echo hello world hello world

After

[success] Total time: 12 s, completed Feb 28, 2022, 3:18:03 AM

[som-snytt]

12 years ago

    /** I have distilled everyone's classpath hopes and dreams into the
     *  question of how to resolve this boolean.  Right at this moment if I
     *  do anything but default to true right here, partest chokes.  I'm
     *  steadily reworking all the places partest gets its hands on the
     *  classpath so eventually I'll be able to remedy that, at which point
     *  my current plan is to have this default to false unless some
     *  property or command line option is supplied.  This is negotiable,
     *  but at this point I can say with confidence that less magic and
     *  less autodetection is more better.
     */
    def useJavaClassPath    = true

The scala 2 script uses env var to set the default for the -usejavacp option

# scala/bug#8358, scala/bug#8368 -- the default should really be false,
# but I don't want to flip the default during 2.11's RC cycle
OVERRIDE_USEJAVACP="-Dscala.usejavacp=true"

At the command line, I'd expect to get a useful class path for free. But I haven't looked to see what assumptions or expectations are built into scala-cli, for contrast.

[nicolasstucki]

This change does not seem to be needed to make tests/run-with-compiler/i14541.scala pass. Why was this added? How could I test this?

[som-snytt]

This is the 2nd commit about whether to do -usejavacp behavior from Scala 2, i.e., use the class loader you were given. I'm doubtful both ways.

[som-snytt]

Oh, it's necessary for the 2nd test where it doesn't supply a class path arg to MainGenericRunner.

[nicolasstucki]

If that is the case, I would remove this change and the second test. The third test already covers the fix. We could have a look at this cp change in a separate PR as we don't know or have time to test unintended consequences of this change. I would prefer to only fix the regression in this PR as this fill be back ported. We also want the backport quickly to be able to release 3.1.2 as soon as possible.

[michelou]

@nicolasstucki
PR #14559 is another backport candidate for release-3.1.2. Can you vote for it please (no one did react till now) ?!

[smarter]

@som-snytt we also have a -usejavacp flag checked in some of our she'll scripts I believe, shouldn't we check it here too?

[som-snytt]

This call works after using "parent class loader" in Runner. Folks with an opinion about how the command line runner should behave might have an opinion.

[som-snytt]

Dropped the speculative commit and squashed.

(Can return to usejavacp usage later, or let scala-cli explore that space.)

Thanks, for future contributions I'll be more disciplined about minimized and easy-to-review deltas.