Compile exercises directly with scalac instead of sbt #31

jcranky · 2022-05-09T21:39:29Z

Improve execution speed by replacing sbt compilation with using scalac directly.

sbt is still used to built the application that converts compilation and test results into exercism's interface format, but it is executed only once, when building the docker image.

sbt, scala and scalatest versions are also updated in some cases, and those should be safe updates.

Finally, one more consequence of this PR is that build.sbt files are not needed for the scala exercises anymore and it should be safe to remove those files, together with their correspondent project folders.

closes #13

ErikSchierboom

Improve execution speed by replacing sbt compilation with using scalac directly.

This is fabulous! Execution time goes from around 8 seconds to 2.5 seconds.

sbt is still used to built the application that converts compilation and test results into exercism's interface format, but it is executed only once, when building the docker image.

Great

sbt, scala and scalatest versions are also updated in some cases, and those should be safe updates.

👍 We should also update the exercises in the Scala repo to match.

Finally, one more consequence of this PR is that build.sbt files are not needed for the scala exercises anymore and it should be safe to remove those files, together with their correspondent project folders.

You mean for the test exercises? If so, I think I would like them to still be there, to better reflect the actual exercise a test runner would run on.

ErikSchierboom · 2022-05-10T13:33:53Z

bin/run-tests.sh

-sbt test
-


Could you restore these two lines? These actually run the tests for the Scala test runner project.

sbt assembly in the Dockerfile already run the tests, so this line causes the tests to be run twice when runnning run-tests-in-docker.sh.

I have a more generic question: are run.sh and run-tests.sh supposed to work outside a docker image? If so, both are broken in this PR and I'll investigate. That being said, those files don't seem to work for me outside docker on the main branch as well.

sbt assembly in the Dockerfile already run the tests, so this line causes the tests to be run twice when runnning run-tests-in-docker.sh.

Ah, I would not have guessed that sbt assembly runs the tests too. Then it is fine to omit it here.

I have a more generic question: are run.sh and run-tests.sh supposed to work outside a docker image? If so, both are broken in this PR and I'll investigate. That being said, those files don't seem to work for me outside docker on the main branch as well.

Ehm, not necessarily. For some tracks, they can. I wouldn't be opposed to a PR that makes them work outside the docker image, but only if it can be done in a somewhat sane way :)

I would say let's leave it as is for now, then 🤐

ErikSchierboom · 2022-05-10T13:34:58Z

bin/run.sh

+# ensure scala binaries are available in the PATH
+export PATH="$PATH:/opt/test-runner/scala-2.13.8/bin"


I think it would make sense to have this be set in the Dockerfile using the ENV command. See https://stackoverflow.com/a/38742545/2071395

Oh, WAY better, thanks. Will change 😄

ErikSchierboom · 2022-05-10T13:51:14Z

bin/run.sh


-mv -f "${original_tests_file}" "${tests_file}"
+# compile source and tests
+scalac -classpath "${test_runner_jar}" -d "${workdir_target}" "${workdir}"/src/main/scala/* "${workdir}"/src/test/scala/* &> "${build_log_file}"


Would this work for exercises that use one or more libraries? E.g. lens-person https://github.com/exercism/scala/blob/main/exercises/practice/lens-person/build.sbt#L8-L9

If it doesn't work, we should do a quick check to see which exercises use libraries and then we can decide what to do (nothing blocking).

Those will only work if those libraries are added to the scala-test-runner dependencies, so that they are added to the fat jar built in the Dockerfile with sbt assembly. In other words, we can make it work 😄

Does that work with the current code? Given the fact that the docker images are run with network disabled, I assume not, but maybe I missed some piece of the puzzle.

Those will only work if those libraries are added to the scala-test-runner dependencies, so that they are added to the fat jar built in the Dockerfile with sbt assembly. In other words, we can make it work 😄

Great! I've just ran a little bash command to try and see which libraries are being used, and this is what I found:

"com.github.julien-truffaut" %% "monocle-core" % "2.0.0",
"com.github.julien-truffaut" %% "monocle-macro" % "2.0.0"
"org.scalaz" %% "scalaz-core" % "7.2.28"
"org.scala-lang.modules" %% "scala-parser-combinators" % "2.1.0"
"org.scalacheck" %% "scalacheck" % "1.14.1" % "test"
"org.scalatestplus" %% "scalacheck-1-15" % "3.2.9.0" % "test"

If we could ensure that all these libraries work in the test runner, we should be good for now. Would you be interested in doing a follow-up PR to add this? If so, I'd suggest adding new test cases to the tests dir to verify that they can use these libraries successfully.

Yes, I can do that. Can you point me to the specific tests we need to support? I would like to have a look at the code and also understand why those libraries are needed in the first place 😄

Sure.

monocle-core, monocle-macro and scalaz-core are used in lens-person: https://github.com/exercism/scala/blob/main/exercises/practice/lens-person/build.sbt

scala-parser-combinators is used in alphametics, wordy, forth, matching-brackets and sgf-parsing

scalacheck is used in simple-linked-list

ErikSchierboom · 2022-05-10T14:50:14Z

tests/example-all-fail/build.sbt

-scalaVersion := "2.12.8"
-
-libraryDependencies += "org.scalatest" %% "scalatest" % "3.0.1" % "test"


I'd argue that this file should still be there (albeit updated to version 2.13.6 that exercises in the Scala repo use), as that better reflects the expected files of an exercise.

You mean leaving them there as sort of a piece of documentation? Because other than that, they wouldn't be used.

It is not really documentation, but more like: this is an actual exercise's contents, which matches what the test runner will receive when we run it in production. So it is more about asserting that the test runner works with the actual exercise files.

All right, adding those files back 😄

Actually... I see the build.sbt file in there, but not project/build.properties. Did I miss something?

ErikSchierboom · 2022-05-10T14:50:20Z

tests/example-all-fail/project/build.properties

@@ -1 +0,0 @@
-sbt.version=1.4.5


See above comment

ErikSchierboom · 2022-05-10T14:50:45Z

tests/example-empty-file/expected_results.json

Maybe there could be a post-processing step to change /tmp/exercise to /solution?

Being a bit lazy, would directly creating a /solution folder and working from there inside the docker image also be acceptable? 😸

No, sorry, as the /solution folder is automatically being mapped.

Ah, I missed that. Will add the post-processing.

ErikSchierboom

This is fantastic. I'm gonna merge this and then enable the test runner. This will make the Scala track so much better.
Thank you so much for doing this.

ErikSchierboom · 2022-05-12T07:38:07Z

bin/run.sh


-mv -f "${original_tests_file}" "${tests_file}"
+# compile source and tests
+scalac -classpath "${test_runner_jar}" -d "${workdir_target}" "${workdir}"/src/main/scala/* "${workdir}"/src/test/scala/* &> "${build_log_file}"


Sure.

monocle-core, monocle-macro and scalaz-core are used in lens-person: https://github.com/exercism/scala/blob/main/exercises/practice/lens-person/build.sbt

scala-parser-combinators is used in alphametics, wordy, forth, matching-brackets and sgf-parsing

scalacheck is used in simple-linked-list

ErikSchierboom · 2022-05-12T09:29:34Z

@jcranky Ah, the tests are not failing. Could you look into that?

ErikSchierboom · 2022-05-12T11:16:00Z

@jcranky I've fixed the issue. Some of the tests were outdated.

I'd like to get this merged. There are two open issues:

Support the libraries mentioned above
I don't much care for the test name grade-school-error. What is the actual error that is being tested by this case?

For both, follow-up PRs would be great.

ErikSchierboom · 2022-05-12T11:17:31Z

Merged! Thanks. I'll do a follow-up PR to enable the test runner.

ErikSchierboom · 2022-05-12T11:44:00Z

@jcranky I've just enabled the test runner, and seems to be working!

jcranky · 2022-05-12T16:58:17Z

@jcranky I've fixed the issue. Some of the tests were outdated.

I'd like to get this merged. There are two open issues:
1. Support the libraries mentioned above

2. I don't much care for the test name `grade-school-error`. What is the actual error that is being tested by this case?
For both, follow-up PRs would be great.

I'll work on that 😄

jcranky · 2022-05-12T17:44:18Z

@ErikSchierboom I'm actually not sure it is working 😢
At least for the exercises I submitted previously to Exercism, they seem to stay stuck on this:

jcranky · 2022-05-14T16:47:49Z

Looking a bit more into the issue I'm having, and it might not be related to this PR. Seems like I'm having some CORS issue:

Should I report this somewhere else?

iHiD · 2022-05-16T10:56:53Z

Those CORS issues are when your browser is trying to connect to bugnsag to log a bug with us. So I think there's some issue before that. If you could report whatever the issue you're having is at at exercism/exercism, we'll take a look.

ErikSchierboom · 2022-05-17T06:21:08Z

@jcranky Did you update your exercise to the latest version?

jcranky · 2022-05-18T20:35:42Z

@iHiD I have no idea what the issue is, I just don't see the code get compile, nor tests run, and get stuck at that screen I shared above.

@ErikSchierboom What does that mean? I didn't change anything, just tried to open the "Tests" tab.

fwiw, this is what I'm doing, step by step:

In Firefox (I tried Chrome, with a similar issue), I open https://exercism.org/tracks/scala/
I click on the Space Age exercise (the problem is actually the same for all exercises)
I already had a solution submitted in the past, so I click on "Your Iterations"
On the right side of the screen, I click on "Tests"
And this is where I get the "We're testing your code to check it works" message, which stays there forever

ErikSchierboom · 2022-05-19T06:31:50Z

Do you see something like this when you go to https://exercism.org/tracks/scala/exercises/space-age? If so, could you click on it and update the exercise first?

jcranky · 2022-05-19T19:23:16Z

Nope:

ErikSchierboom · 2022-05-20T07:32:28Z

Hmmm, weird. I'll look into it

ErikSchierboom · 2022-05-20T09:27:49Z

@jcranky Could you try to click on the "Run tests" in the Online Editor? You need to add a space somewhere to get it enabled.

jcranky · 2022-05-20T16:04:55Z

@ErikSchierboom Just tried doing that for a couple of different exercises and I can confirm that running tests from within the online editor works.

ErikSchierboom · 2022-05-24T11:53:51Z

@jcranky Ah right, then I know what the issue is. The problem is that the website now thinks that there is a test runner, but there wasn't at the time the solution was submitted. We're not automatically scheduling old solutions. We have a fix in mind, but have not yet gotten around to implementing it.

exercism/exercism#6064 is used to track this.

jcranky · 2022-05-25T23:52:48Z

I see, thanks :)
I just submitted a solution to an exercise I didn't complete before, and it worked quite well 😄

jcranky added 2 commits May 9, 2022 23:22

Compile exercises directly with scalac instead of sbt

37a058b

Remove unecessary build.sbt and project/

b7baadf

jcranky requested a review from a team as a code owner May 9, 2022 21:39

ErikSchierboom reviewed May 10, 2022

View reviewed changes

jcranky added 4 commits May 10, 2022 20:45

Add scala binaries to the path using docker ENV

e74b5fe

Change workdir to input_dir in the final results file

8d0eb8f

Restore tests's build.sbt files

73c8ec5

Align dependency versions with the ones used by the actual exercises

5d32812

ErikSchierboom approved these changes May 12, 2022

View reviewed changes

Fix failing tests

81ca0a2

ErikSchierboom added the x:rep/large Large amount of reputation label May 12, 2022

ErikSchierboom merged commit 06428d8 into exercism:main May 12, 2022

jcranky deleted the compile-exercises-without-sbt branch May 12, 2022 16:58

This was referenced Jul 14, 2022

Document reputation label exercism/docs#347

Merged

Consider correct reputation amounts for different labels exercism/exercism#6440

Open

Consider correct reputation amounts for different labels exercism/exercism#6441

Closed

jcranky mentioned this pull request Sep 2, 2022

remove grade-school example #34

Merged

		# ensure scala binaries are available in the PATH
		export PATH="$PATH:/opt/test-runner/scala-2.13.8/bin"

		scalaVersion := "2.12.8"

		libraryDependencies += "org.scalatest" %% "scalatest" % "3.0.1" % "test"

		@@ -1 +0,0 @@
		sbt.version=1.4.5

Compile exercises directly with scalac instead of sbt #31

Compile exercises directly with scalac instead of sbt #31

Conversation

jcranky commented May 9, 2022 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcranky May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcranky May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcranky May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSchierboom commented May 12, 2022

ErikSchierboom commented May 12, 2022

ErikSchierboom commented May 12, 2022

ErikSchierboom commented May 12, 2022

jcranky commented May 12, 2022

jcranky commented May 12, 2022

jcranky commented May 14, 2022

iHiD commented May 16, 2022

ErikSchierboom commented May 17, 2022

jcranky commented May 18, 2022

ErikSchierboom commented May 19, 2022

jcranky commented May 19, 2022

ErikSchierboom commented May 20, 2022

ErikSchierboom commented May 20, 2022

jcranky commented May 20, 2022

ErikSchierboom commented May 24, 2022

jcranky commented May 25, 2022

jcranky commented May 9, 2022 •

edited

Loading

jcranky May 10, 2022 •

edited

Loading

jcranky May 11, 2022 •

edited

Loading

jcranky May 10, 2022 •

edited

Loading