-
Notifications
You must be signed in to change notification settings - Fork 277
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
Add CommandLineParserAdapter for >=2.13.9 #1634
Add CommandLineParserAdapter for >=2.13.9 #1634
Conversation
Fixes //third_party/utils/src/test:test_util for Scala 2.13.9 and later by providing a thin `scala.tools.cmd.CommandLineParser` adapter. Before this change, building under Scala 2.13.14 produced: ```txt $ bazel build --repo_env=SCALA_VERSION=2.13.14 \ //third_party/utils/src/test:test_util ERROR: .../third_party/utils/src/test/BUILD:6:14: scala @@//third_party/utils/src/test:test_util failed: (Exit 1): scalac failed: error executing Scalac command (from target //third_party/utils/src/test:test_util) bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/src/java/io/bazel/rulesscala/scalac/scalac @bazel-out/darwin_arm64-fastbuild/bin/third_party/utils/src/test/test_util.jar-0.params third_party/utils/src/test/io/bazel/rulesscala/utils/TestUtil.scala:10: error: object cmd is not a member of package tools import scala.tools.cmd.CommandLineParser ^ third_party/utils/src/test/io/bazel/rulesscala/utils/TestUtil.scala:119: error: not found: value CommandLineParser val options = CommandLineParser.tokenize(compileOptions) ^ ``` Turns out `CommandLineParser` disappered in Scala 2.13.9, and its `Parser` replacement is private: - scala/scala#10057
@@ -0,0 +1,8 @@ | |||
package scala.tools.cmd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be unlikely but in theory some other compiler plugin might define the same workaround. Have you maybe considered creating a dedicated proxy explicitly for rulesscala to decrease chances of name clash?
I had this issue recently when starting work on dependency analyzer plugin port to Scala 3. I've defined it there as following:
For Scala 2.13.10+
package scala {
package rulesscala {
// proxy to private[scala] compiler API
object Proxy {
def tokenize(cmd: String): List[String] = sys.process.Parser.tokenize(cmd)
}
}
}
package io.bazel.rulesscala.utils {
trait CompilerAPICompat {
def tokenize(cmd: String): List[String] = scala.rulesscala.Proxy.tokenize(cmd)
}
}
For other versions:
package io.bazel.rulesscala.utils
import scala.tools.cmd.CommandLineParser
trait CompilerAPICompat {
def tokenize(cmd: String): List[String] = CommandLineParser.tokenize(cmd)
}
Then I've redefined object TestUtil
to extend this trait and used a proxy method instead of depending on compiler internals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is better. I haven't actually done much Scala in a while, so this approach didn't occur to me, but it's definitely better than monkeypatching scala.tools.cmd
.
I didn't see your original code in the existing code base, so I literally copied and pasted it into a new commit (crediting you, of course).
If this should live somewhere else, or you're about to check it in somewhere else (a quick find | xargs grep
didn't turn it up), let me know.
Copied the `CompilerAPICompat` code verbatim from @WojciechMazur's suggestion on bazelbuild#1634. Definitely seems more robust than injecting `CommandLineParser` directly into `scala.tools.cmd`.
Copied the `CompilerAPICompat` code verbatim from @WojciechMazur's suggestion on bazelbuild#1634. Definitely seems more robust than injecting `CommandLineParser` directly into `scala.tools.cmd`.
import scala.tools.nsc.CompilerCommand | ||
import scala.tools.nsc.Global | ||
import scala.tools.nsc.Settings | ||
import scala.tools.nsc.reporters.StoreReporter | ||
import io.bazel.rulesscala.dependencyanalyzer.DependencyTrackingMethod | ||
|
||
object TestUtil { | ||
object TestUtil extends CompilerAPICompat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to make CompilerAPICompat
an object instead of a trait? It's not a big deal I guess, but on the other hand you are adding tokenize
to the api of TestUtil
and I'm not sure it belongs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I was thinking about doing that, too, as I tend to prefer helper objects to mixins for similar reasons.
I also renamed it to CommandLineParserAdapter
to reflect the very specific reason for its existence.
Requested by @simuons in bazelbuild#1634. Renamed it as well to reflect its very specific function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mbland!
Description
Fixes //third_party/utils/src/test:test_util for Scala 2.13.9 and later by providing a thin
scala.tools.cmd.CommandLineParser
adapter. Part of #1482.Motivation
Before this change, building under Scala 2.13.14 produced:
Turns out
CommandLineParser
disappered in Scala 2.13.9, and itsParser
replacement is private: