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

Scala sh rewrite #13081

Merged
merged 10 commits into from
Aug 2, 2021
Merged

Scala sh rewrite #13081

merged 10 commits into from
Aug 2, 2021

Conversation

BarkingBad
Copy link
Contributor

No description provided.

@BarkingBad BarkingBad force-pushed the scala-sh-rewrite branch 2 times, most recently from 46dbb4f to 86095f7 Compare July 15, 2021 16:32
@BarkingBad
Copy link
Contributor Author

Does anyone know why we couldn't create val of type Regex then reuse it and we had to inline the regex in the if condition? My last commit changes this and then the CI passes. Previously it was failing with for CompilationTests.tastyBootstrap:

[info] Test dotty.tools.dotc.CompilationTests.tastyBootstrap started
possible data race involving globally reachable variable root in class Pattern: java.util.regex.Pattern#Node
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable matchRoot in class Pattern: java.util.regex.Pattern#Node
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable buffer in class Pattern: Array[Int]
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable predicate in class Pattern: java.util.regex.Pattern#CharPredicate
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable namedGroups in class Pattern: java.util.Map[String, Integer]
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable groupNodes in class Pattern: Array[java.util.regex.Pattern#GroupHead]
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable topClosureNodes in class Pattern: java.util.List[java.util.regex.Pattern#Node]
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable localTCNCount in class Pattern: Int
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable hasGroupRef in class Pattern: Boolean
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable capturingGroupCount in class Pattern: Int
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
possible data race involving globally reachable variable localCount in class Pattern: Int
  use -Ylog:checkReentrant+ to find out more about why the variable is reachable.
Compilation failed for: 'dotty1 from tastyBootstrap/dotty1'                     
Error:  Test dotty.tools.dotc.CompilationTests.tastyBootstrap failed: java.lang.AssertionError: Expected no errors when compiling, failed for the following reason(s):
Error:  
Error:    - encountered 1 test failures(s), took 34.712 sec
Error:      at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkCompile(ParallelTesting.scala:897)
Error:      at dotty.tools.dotc.CompilationTests.$anonfun$2(CompilationTests.scala:288)
Error:      at scala.collection.immutable.List.map(List.scala:250)
Error:      at dotty.tools.dotc.CompilationTests.tastyBootstrap(CompilationTests.scala:288)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
Error:      at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Error:      at java.lang.reflect.Method.invoke(Method.java:567)
Error:      ...

Also, for coursier tests I need artifacts to be published. Is it OK @smarter or I should redesign it, for example move it to the separate CI action?

@BarkingBad BarkingBad marked this pull request as ready for review July 16, 2021 11:00
@smarter
Copy link
Member

smarter commented Jul 16, 2021

Does anyone know why we couldn't create val of type Regex

The analysis done by CheckReentrant is imprecise and incorrectly thinks that java regexps are not thread-safe, but it's actually fine to use them, so we just turn off the check using @sharable: https://github.com/lampepfl/dotty/blob/839ef3203e654a38b431558eea52821c974a1734/compiler/src/dotty/tools/repl/ParseResult.scala#L114 (though we could also special-case regexps in CheckReentrant)

Also, for coursier tests I need artifacts to be published. Is it OK @smarter or I should redesign it

What's important is that if I run test locally, all tests should succeed, so we can't have tests enabled by default that require some extra step.

def process(args: List[String], settings: Settings): Settings = args match
case Nil =>
settings
case "-repl" :: tail =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which situation is it necessary to explicitly pass -repl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, never. But dist/bin script provides such option, so we wanted to recreate this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's never used let's just remove it then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which situation is it necessary to explicitly pass -repl?

In dist/bin/scalac, it's needed, but doesn't seem to be needed in dist/bin/scala.

@smarter
Copy link
Member

smarter commented Jul 16, 2021

The last commit committed .class and .tasty files that shouldn't be in git.

@BarkingBad
Copy link
Contributor Author

The last commit committed .class and .tasty files that shouldn't be in git.

I did it on purpose for testing -run but I can recompile the .scala file indeed.

@lrytz
Copy link
Member

lrytz commented Jul 19, 2021

This PR addresses coursier/coursier#2058, right?

@BarkingBad
Copy link
Contributor Author

@lrytz I was not aware about this issue. However I am afraid it will take some time till we get it released to maven central, so the coursier descriptor can be created for scala3 as it will be release with 3.0.3

@smarter
Copy link
Member

smarter commented Jul 19, 2021

if this is deemed important enough, we could backport it to the 3.0.2 branch.

@BarkingBad BarkingBad force-pushed the scala-sh-rewrite branch 3 times, most recently from 1fae969 to 04af902 Compare July 19, 2021 19:00
@philwalk
Copy link
Contributor

philwalk commented Jul 20, 2021

@BarkingBad - I like the idea of migrating functionality from sh script to scala code, although I'm not entirely clear as to the scope of this PR. Will there be support for jline console functionality?

@BarkingBad
Copy link
Contributor Author

What do you mean @philwalk? If you mean that coursier tests cannot initialize jline I think it's the internal problem from calling in the virtual terminal. Calling REPL works just fine for both shell script and when we eventually turn it into coursier executable
obraz

@philwalk
Copy link
Contributor

philwalk commented Jul 20, 2021

What do you mean @philwalk?

I'm not referring specifically to the coursier use-case, but to whether and how a Windows cygwin or msys environment (for example) might be affected by this PR. Possibly I just need to understand some context.

When I launch a REPL session from a cygwin bash terminal, here's what I see with these changes:

# philwalk@d5 ~/workspace/dotty-sh-rewrite
$ bin/scala
Starting scala3 REPL...
←[?1h←=←[90m~←[0m
←[?2004h←[34mscala> ←[0m:q
:q
←[?1l←>←[?1000l←[?2004l

By contrast, here's what happens with the current scala release:

$ /opt/scala3/bin/scala
scala> :q

Also, there is some other lost functionality compared to the current release (e.g., SCALA_OPTS is no longer supported).

BTW, I developed a scala implementation of dist/bin/common that I can contribute, if such a thing makes sense here.

@philwalk
Copy link
Contributor

A fix for the cygwin console problem is to copy the following line from dist/bin/scalac line 72 to dist/bin/scala just before calling java (just before the eval $JAVACMD, near line 32)

[ -z "${ConEmuPID-}" -o -n "${cygwin-}" ] && export MSYSTEM= PWD= # workaround for #12405

@BarkingBad
Copy link
Contributor Author

Thank you for your response @philwalk. I will try to setup VM Windows to test it as well when I settle up with other issues

@philwalk
Copy link
Contributor

philwalk commented Jul 22, 2021

@BarkingBad
We're actually closer than I thought. Here are some remaining gaps in the implementation, AFAICT:

The -Dscript.path=<script-path> property needs to be set in dist/bin/scala, and the script compiler/test-resources/scripting/scriptPathh.sc should have caused the build to fail. An updated version of BashScriptsTests has a new test to verify this property #13132.

The decision as to whether an argument is a <script-path> should be generous and accept an argument even if the specified file does not exist; otherwise, when an interactive user misspells a script path, all of the arguments intended for the script will be sent to the compiler, resulting in incomprehensible error messages, or worse.

The release version of dist/bin/scala discards scala_args when launching REPL mode, since they are intended as the args that go to scala when running in script mode. It would be nice if they could be handled properly in REPL mode as well, although that hasn't been implemented yet.

@BarkingBad
Copy link
Contributor Author

BarkingBad commented Jul 27, 2021

Hi @philwalk

I implemented your cygwin fix.

Also referring to your comment #13081 (comment) the stacktrace is from the Run branch since I didn't implement @ arguments, therefore he tried to run the classpath as a compiled file. (As you can see it does not enter dotty.tools.scriptin.Main.main in the stacktrace)

The second comment #13081 (comment) about script.path is in my opinion not valid in terms of what we are trying to do, which is not to process args in bash script but in scala entry Main. I added setting up the value of script.path in the scripting.Main and also test it in CoursierTests

I rebased onto your branch with tests (since they are not merged yet) and ran these tests locally - all passed 😄

@BarkingBad BarkingBad force-pushed the scala-sh-rewrite branch 2 times, most recently from 2275440 to ca5b9a7 Compare July 27, 2021 12:40
@philwalk
Copy link
Contributor

philwalk commented Jul 27, 2021

@BarkingBad

I added setting up the value of script.path in the scripting.Main

Yes, that's better. Everthing looks good to me!

@BarkingBad
Copy link
Contributor Author

Could you take a look @philwalk one more time and if everything is OK give approval? I need it to merge the changes

Copy link
Contributor

@philwalk philwalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BarkingBad - everything looks good, I only have one suggestion:

  1. in CoursierScalaTests.scala, the scriptTest only verifies that the property is defined, it could also verify that the value endsWith scriptPath.sc.

@BarkingBad BarkingBad merged commit d0c0eff into scala:master Aug 2, 2021
@bishabosha
Copy link
Member

can we backport this?

@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Aug 2, 2021
@BarkingBad
Copy link
Contributor Author

I think we could, it would be beneficial for all coursier users I guess

@dwijnand
Copy link
Member

dwijnand commented Aug 2, 2021

I'd say no: it's ~500 lines of fresh code, let's leave the backports for the critical bugfixes.

@SethTisue
Copy link
Member

SethTisue commented Aug 11, 2021

Hey, this is great! 👏

I was able to figure out how to use this from Coursier today, with a little extra effort; see coursier/coursier#2058 (comment) for details

@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 26, 2022
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants