Skip to content

fix for #11536 and other terminal and shell environment issues #11539

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

Merged
merged 5 commits into from
Mar 3, 2021
Merged

fix for #11536 and other terminal and shell environment issues #11539

merged 5 commits into from
Mar 3, 2021

Conversation

philwalk
Copy link
Contributor

fix for #11536

Also verified working REPL for various combinations of Terminal and Windows shell.
See https://github.com/lampepfl/dotty/issues/11536#issuecomment-786156696

@philwalk
Copy link
Contributor Author

philwalk commented Feb 26, 2021

NOTE: hopefully this comment isn't confusing, it's an idea for a proposed change, but is not part of this PR.

Currently, calling scripts results in bin/common being sourced twice, first by bin/scala, then by bin/scalac.
We can cut some significant time from script startups, by the following changes:

  1. export variables at the bottom of dist/bin/common
  2. conditionally source bin/common, only if variables are not already initialized

Here's what the change looks like in bin/scalac:

# only need to source common if variables not already initialized
[[ -z "$PSEP" && -z "$DOTTY_COMP" ]] && source "$PROG_HOME/bin/common"

In my environment, it eliminates between 250 and 350 milliseconds from script startups, when a compile is required.

@michelou
Copy link
Contributor

michelou commented Feb 27, 2021

@philwalk Usage of eval and onExit is not consistent across the three bash scripts :

$ git diff dist\bin\scalac dist\bin\scaladoc
diff --git a/dist/bin/scalac b/dist/bin/scalac
index f81c20853b..74bfc4a603 100755
--- a/dist/bin/scalac
+++ b/dist/bin/scalac
@@ -120,7 +120,7 @@ if [ "$PROG_NAME" == "$ScriptingMain" ]; then
   scripting_string="-script $target_script ${scripting_args[@]}"
 fi

-eval exec "\"$JAVACMD\"" \
+eval "\"$JAVACMD\"" \
      ${JAVA_OPTS:-$default_java_opts} \
      "${DEBUG-}" \
      "${java_args[@]}" \
@@ -130,4 +130,5 @@ eval exec "\"$JAVACMD\"" \
      "${scala_args[@]}" \
      "${residual_args[@]}" \
      "${scripting_string-}"
-exit $?
+scala_exit_status=$?
+onExit
diff --git a/dist/bin/scaladoc b/dist/bin/scaladoc
index b536dbf99e..304da8f504 100755
--- a/dist/bin/scaladoc
+++ b/dist/bin/scaladoc
@@ -125,7 +125,7 @@ done

 classpathArgs

-eval exec "\"$JAVACMD\"" \
+eval "\"$JAVACMD\"" \
      ${JAVA_OPTS:-$default_java_opts} \
      "$DEBUG" \
      "${java_args[@]}" \
@@ -134,4 +134,5 @@ eval exec "\"$JAVACMD\"" \
      "${scala_args[@]}" \
      "${residual_args[@]}" \
      "$scripting_string"
-exit $?
+scala_exit_status=$?
+onExit

Do we agree with the following general rules ?!

  • do not write eval exec <something>.
  • a call to eval is always followed by the assignment scala_exit_status=$?.
  • onExit is always the last executed instructon in the script.

@philwalk
Copy link
Contributor Author

philwalk commented Feb 27, 2021

@michelou
The rules sound correct to me, I will make the suggested change to dist\bin\scaladoc and do some manual testing.

@philwalk
Copy link
Contributor Author

@michelou - I'm not able to exhaustively test the change to dist/bin/scaladoc, although it's is pretty simple. Should I push the updated version?

@michelou
Copy link
Contributor

@michelou - I'm not able to exhaustively test the change to dist/bin/scaladoc, although it's is pretty simple. Should I push the updated version?

I'd like to keep scalac and scaladoc in sync. So yes.

@philwalk philwalk changed the title fix for various combinations of terminal and shell environment; fix f… fix for #11536 and various other terminal and shell environment issues Mar 1, 2021
@philwalk philwalk changed the title fix for #11536 and various other terminal and shell environment issues fix for #11536 and other terminal and shell environment issues Mar 1, 2021
@anatoliykmetyuk anatoliykmetyuk merged commit 302b186 into scala:master Mar 3, 2021
@philwalk philwalk deleted the scalac-common-patch branch March 3, 2021 15:27
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.

3 participants