Fix dotr -tasty #3436

nicolasstucki · 2017-11-03T16:30:01Z

No description provided.

smarter · 2017-11-03T22:53:23Z

project/Build.scala


-        if (java == "")
+        if (args.isEmpty) {
+          println("Couldn't run `dotr` without args. Use `repl` to run the repl or add args to run the dotty application")


Why not call the repl task at this point instead?

I tried with repl.evaluated but then dotr only ran repl ignoring the nonempty args. Then I just decided to show this message as we don't really need more in the sbt project.

sbt task evaluation is tricky, you probably need to use a dynamic task to get that to work.

That is what I thought, hence settled for the message. My main concerns were the factorisation of code and checking the it was executing correctly.

I don't think we should special case for empty args. We need a better solution:
If I run dotr -Xprint:typer, args is not empty, but I still want to run the REPL

you can at least run repl -Xprint:typer.

allanrenucci · 2017-11-04T18:19:21Z

project/Build.scala

      }
  )

+  def dotDynTask(main: String) = Def.inputTaskDyn {


I think runCompilerMain is a better name

allanrenucci · 2017-11-04T18:22:54Z

project/Build.scala

          println("Couldn't find scala-library on classpath, please run using script in bin dir instead")
        } else {
          val dottyLib = packageAll.value("dotty-library")
          s"""$java -classpath .:$dottyLib:$scalaLib ${args.mkString(" ")}""".!


Since you're refactoring. Triple quotes is not needed here

allanrenucci · 2017-11-04T18:29:06Z

.drone.yml

    commands:
      - cp -R . /tmp/1/ && cd /tmp/1/
-      - ./project/scripts/sbt ";compile ;testAll ;dotty-bench/jmh:run 1 1 tests/pos/alias.scala"
+      - ./project/scripts/sbt ";compile ;testAll ;dotty-bench/jmh:run 1 1 tests/pos/alias.scala; ;dotc tests/run/arraycopy.scala ;dotr Test"


tests/pos/alias.scala; ;dotc double semicolon?

Also not sure it is the best place to test an sbt task. @smarter Do you think it should be a scripted test?

Scripted tests are always good in my opinion :)

allanrenucci · 2017-11-04T18:33:30Z

project/Build.scala


-        if (java == "")
+        if (args.isEmpty) {
+          println("Couldn't run `dotr` without args. Use `repl` to run the repl or add args to run the dotty application")


I don't think we should special case for empty args. We need a better solution:
If I run dotr -Xprint:typer, args is not empty, but I still want to run the REPL

nicolasstucki · 2017-11-13T17:17:55Z

~~Next step (in another PR) will be to remove the dotty.tools.dot.FromTasty driver and integrate it to dotty.tools.dot.Main. This will make -tasty an actual ScalaSetings flag.~~
Done :)

smarter · 2017-11-14T22:46:36Z

dist/bin/dotc

    -Oshort) addJava "-XX:+TieredCompilation -XX:TieredStopAtLevel=1" && shift ;;
        -repl) PROG_NAME="$ReplMain" && shift ;;
-       -tasty) addScala "-Yretain-trees" && PROG_NAME="$FromTasty" && shift ;;
+       -tasty) addScala "-Yretain-trees" && PROG_NAME="$CompilerMain" && shift ;;


I would just make -Yretain-trees default to true when -tasty is used.

I mean, in the compiler itself, instead of doing it in various scripts.

smarter

Otherwise LGTM.

nicolasstucki · 2017-11-15T12:48:31Z

Last commit needs review

OlivierBlanvillain · 2017-11-15T13:44:27Z

project/scripts/sbtBootstrappedTests

+# check that `dotc` compiles and `dotr` runs it
+echo "testing sbt dotc and dotr"
+mkdir out/scriptedtest0
+./dist-bootstrapped/target/pack/bin/dotc tests/pos/sbtDotrTest.scala -d out/scriptedtest0


Any reason to use dist-bootstrapped/**/dotc instead of bin/dotc?

I wanted to make sure I was testing that version of the script. We could also add some tests for bin/dotc in the future.

bin/dotc just forwards to dist-bootstrapped/target/pack/bin/dotc and calls sbt dist-bootstrapped/pack if needed :)

Ok, I'll change it.

Fix #3496: Insert classpath before other arguments

nicolasstucki requested a review from allanrenucci November 3, 2017 16:30

smarter reviewed Nov 3, 2017

View reviewed changes

allanrenucci reviewed Nov 4, 2017

View reviewed changes

nicolasstucki force-pushed the abstract-dotc-repl branch 3 times, most recently from 6cf735e to be81325 Compare November 8, 2017 09:58

nicolasstucki added the stat:needs review label Nov 9, 2017

nicolasstucki requested a review from allanrenucci November 9, 2017 10:13

nicolasstucki assigned allanrenucci Nov 11, 2017

nicolasstucki force-pushed the abstract-dotc-repl branch from be81325 to 53e24a5 Compare November 12, 2017 13:46

nicolasstucki changed the title ~~Abstract dotc repl~~ Fix dotr -tasty Nov 12, 2017

nicolasstucki requested review from smarter and removed request for allanrenucci November 12, 2017 14:23

nicolasstucki assigned smarter and unassigned allanrenucci Nov 12, 2017

nicolasstucki force-pushed the abstract-dotc-repl branch 2 times, most recently from 069c36d to 7c4d1d7 Compare November 12, 2017 16:12

smarter reviewed Nov 14, 2017

View reviewed changes

smarter approved these changes Nov 14, 2017

View reviewed changes

nicolasstucki force-pushed the abstract-dotc-repl branch from d287c70 to 7b9c17f Compare November 15, 2017 07:00

nicolasstucki added 9 commits November 15, 2017 11:28

Add sbt dotr instuctions for repl

b47c487

Factor sbt dotr/repl common init code

ec084ec

Add simple regression tests for sbt dotc/dotr

5aaed79

Improve implementation of sbt dotc

660cb40

Rename sbt dotc/repl abstraction to runCompilerMain

47ae1cf

Move sbt tests to scripted tests

20c72f4

Check sbt dotr output

e133b9e

Fix dotr -tasty

26cdb57

Fix sbt dotr with -classpath

106b15d

nicolasstucki added 3 commits November 15, 2017 11:28

Add regression test for dotc -tasty and dotr -classpath

6889d44

Extract inner classes from FromTasty

178402e

Make -tasty a ScalaSettings flag and remove FromTasty

fdbab48

nicolasstucki force-pushed the abstract-dotc-repl branch from 7b9c17f to a809cfd Compare November 15, 2017 10:54

Do not insert -Yretain-trees when compiling with -tasty

6706b9e

nicolasstucki force-pushed the abstract-dotc-repl branch from a809cfd to 6706b9e Compare November 15, 2017 12:27

OlivierBlanvillain reviewed Nov 15, 2017

View reviewed changes

Change dist-bootstrapped/target/pack/bin to bin

faedad7

nicolasstucki mentioned this pull request Nov 16, 2017

Create dotc -decompiler #3485

Merged

OlivierBlanvillain approved these changes Nov 16, 2017

View reviewed changes

nicolasstucki merged commit fe24786 into scala:master Nov 16, 2017

smarter mentioned this pull request Nov 17, 2017

"sbt dotr" does not automatically put scala-library on the classpath anymore #3496

Closed

nicolasstucki referenced this pull request Nov 17, 2017

Merge pull request #3498 from dotty-staging/fix-#3436

0155804

Fix #3496: Insert classpath before other arguments

allanrenucci deleted the abstract-dotc-repl branch December 14, 2017 15:01

Uh oh!

Fix dotr -tasty #3436

Fix dotr -tasty #3436

Uh oh!

Conversation

nicolasstucki commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlivierBlanvillain Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicolasstucki commented Nov 13, 2017 •

edited

Loading

OlivierBlanvillain Nov 15, 2017 •

edited

Loading