Use ::: as scalacopts delimiter

[wisechengyi]

Description

As suggested in #1049, using ::: as the delimiter instead of , to separate scalac options, in order to avoid collision with ,.

Motivation

Per scalac doc https://docs.scala-lang.org/overviews/compiler-options/index.html: there could be options like -Ywarn-unused:WARNING1,WARNING2

For example before this change, if

scalaopts = ['-Ywarn-unused:-implicits,-locals,-explicits,-patvars,-privates']

the build would fail with

scalac error: bad option: '-locals'
  scalac -help  gives more information
one error found
java.lang.RuntimeException: Build failed
	at io.bazel.rulesscala.scalac.ScalacProcessor.compileScalaSources(ScalacProcessor.java:256)
	at io.bazel.rulesscala.scalac.ScalacProcessor.processRequest(ScalacProcessor.java:76)
	at io.bazel.rulesscala.worker.GenericWorker.runPersistentWorker(GenericWorker.java:45)
	at io.bazel.rulesscala.worker.GenericWorker.run(GenericWorker.java:111)
	at io.bazel.rulesscala.scalac.ScalaCInvoker.main(ScalaCInvoker.java:41)

[ittaiz]

Thanks!
I’m not sure I understand how the current test guards us from regression.
I understand previously it would have broke rules_scala but I don’t see how the added test demonstrates the support of this attribute.

[wisechengyi]

@ittaiz sorry just saw the message in the slack.

Just added

1. test/scalacopts, so will be covered by bazel build test/...
2. unit test on option parsing: src/java/io/bazel/rulesscala/scalac/CompileOptionsTest.java

Let me know if further adjustment is needed. Thanks!

[ittaiz]

ok, I think the current test indeed guards us but IMHO in a mechanical white box way.
I think a more semantic test could both help regression and demonstrate the feature set more.
For example we could add a target that has `scalacopts = [ "-language:implicitConversions, postfixOps", -language:higherKinds, -Xfatal-warnings]" and I'd have the scala file of that target use these three features. I've added the fatal-warnings since I think (not sure) that you can use them right now without the flags but they will generate a warning so the fatal makes sure that they're actually used.

I'd be happy to push a commit to this PR with the test if the above isn't clear.

Btw,
We can also do an integration test by having a scala file with warnings, use your examples to turn on these warnings and use again fatal-warnings. then the integration test asserts the build fails.
I think the first option is simpler and faster (unit tests are cached) but just wanted to give it for completeness. Both check actual feature.

[wisechengyi]

Thanks! Makes sense. My initial plan was to introduce an unused import in the code then use scalacopts to fail on unused import, which aligns with what you suggested, but ran into backwards incompatibility issue with 2.11.

Let me see if I can somehow only test this with 2.12 via test_versions.sh

[ittaiz]

If you fail on an unused import then this needs to be an external shell integration test (otherwise the build won’t pass).

If you hit a wall with my offer LMK and I’ll help. You can even push a half baked commit.

Re integration tests- There is work in the bazel space to allow such integration tests to be part of the bazel build.
I have an OSS library (bazel integration testing) which does this but it’s not trivial to use and has its costs. There is a chance that whatever will come from bazel will be easier.

[wisechengyi]

here is the relevant part using scalac options

[ittaiz]

Thanks!
I think we’re really close

[ittaiz]

Looks great, thanks!
Will merge once green.