diff --git a/scala/private/macros/scala_repositories.bzl b/scala/private/macros/scala_repositories.bzl index f7b8fef7a..861569ded 100644 --- a/scala/private/macros/scala_repositories.bzl +++ b/scala/private/macros/scala_repositories.bzl @@ -83,8 +83,16 @@ def scala_repositories( "io_bazel_rules_scala_scalactic", "io_bazel_rules_scala_scala_xml", "io_bazel_rules_scala_scala_parser_combinators", + # Remove this dependency when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge + "io_bazel_rules_scala_scopt", ], maven_servers = maven_servers, fetch_sources = fetch_sources, overriden_artifacts = overriden_artifacts, ) + + # Remove this dependency when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge + native.bind( + name = "io_bazel_rules_scala/dependency/scala/scopt", + actual = "@io_bazel_rules_scala_scopt", + ) diff --git a/src/scala/io/bazel/rules_scala/scrooge_support/BUILD b/src/scala/io/bazel/rules_scala/scrooge_support/BUILD index b363e6454..c1aa7b601 100644 --- a/src/scala/io/bazel/rules_scala/scrooge_support/BUILD +++ b/src/scala/io/bazel/rules_scala/scrooge_support/BUILD @@ -11,5 +11,18 @@ scala_library( visibility = ["//visibility:public"], deps = [ "//twitter_scrooge:compiler_classpath", + # Remove this dependency when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge + ":option_parser", + ], +) + +# Remove this target when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge +scala_library( + name = "option_parser", + srcs = ["ScroogeOptionParser.scala"], + visibility = ["//visibility:public"], + deps = [ + "//external:io_bazel_rules_scala/dependency/scala/scopt", + "//external:io_bazel_rules_scala/dependency/thrift/scrooge_generator", ], ) diff --git a/src/scala/io/bazel/rules_scala/scrooge_support/Compiler.scala b/src/scala/io/bazel/rules_scala/scrooge_support/Compiler.scala index 5251acdd5..0224582ba 100644 --- a/src/scala/io/bazel/rules_scala/scrooge_support/Compiler.scala +++ b/src/scala/io/bazel/rules_scala/scrooge_support/Compiler.scala @@ -33,8 +33,9 @@ package io.bazel.rules_scala.scrooge_support import com.twitter.scrooge._ import com.twitter.scrooge.ast.Document -import com.twitter.scrooge.backend.{GeneratorFactory, ScalaGenerator, ServiceOption} +import com.twitter.scrooge.backend.{GeneratorFactory, ScalaGenerator} import com.twitter.scrooge.frontend.{FileParseException, TypeResolver, ThriftParser, Importer, MultiImporter, ZipImporter} +import com.twitter.scrooge.java_generator.ApacheJavaGenerator import java.io.{File, FileWriter} import java.nio.file.Paths import java.util.jar.{ JarFile, JarEntry } @@ -44,9 +45,6 @@ import scala.collection.mutable import scala.collection.JavaConverters._ object CompilerDefaults { - var language: String = "scala" - var defaultNamespace: String = "thrift" - def listJar(_jar: File): List[String] = try { val files = List.newBuilder[String] @@ -66,46 +64,34 @@ object CompilerDefaults { } } -class Compiler { - val defaultDestFolder = "." - var destFolder: String = defaultDestFolder +class Compiler(val config: ScroogeConfig) { // These are jars we are including, but are not compiling - val includeJars = new mutable.ListBuffer[String] + val includeJars = config.includePaths // these are the jars we want to compile into scala source jars - val compileJars = new mutable.ListBuffer[String] - val flags = new mutable.HashSet[ServiceOption] - val namespaceMappings = new mutable.HashMap[String, String] - var verbose = false - var strict = true - var skipUnchanged = false - var experimentFlags = new mutable.ListBuffer[String] - var fileMapPath: scala.Option[String] = None + val compileJars = config.thriftFiles + val experimentFlags = config.languageFlags var fileMapWriter: scala.Option[FileWriter] = None - var dryRun: Boolean = false - var language: String = CompilerDefaults.language - var defaultNamespace: String = CompilerDefaults.defaultNamespace - var scalaWarnOnJavaNSFallback: Boolean = false def run() { // if --gen-file-map is specified, prepare the map file. - fileMapWriter = fileMapPath.map { path => + fileMapWriter = config.fileMapPath.map { path => val file = new File(path) val dir = file.getParentFile if (dir != null && !dir.exists()) { dir.mkdirs() } - if (verbose) { + if (config.verbose) { println("+ Writing file mapping to %s".format(path)) } new FileWriter(file) } val allJars: List[File] = - ((includeJars.toList) ::: (compileJars.toList)) + ((includeJars) ::: (compileJars)) .map { path => (new File(path)).getCanonicalFile } - val isJava = language.equals("java") + val isJava = config.language.equals("java") val documentCache = new TrieMap[String, Document] // compile @@ -126,36 +112,37 @@ class Compiler { val importer = rootImporter.copy(focus = focus) +: rootImporter val parser = new ThriftParser( importer, - strict, + config.strict, defaultOptional = isJava, skipIncludes = false, documentCache ) parser.logger.setLevel(Level.OFF) // scrooge warns on file names with "/" - val doc = parser.parseFile(inputFile).mapNamespaces(namespaceMappings.toMap) + val doc = parser.parseFile(inputFile).mapNamespaces(config.namespaceMappings) - if (verbose) println("+ Compiling %s".format(inputFile)) + if (config.verbose) println("+ Compiling %s".format(inputFile)) val resolvedDoc = TypeResolver()(doc) val generator = GeneratorFactory( - language, + config.language, resolvedDoc, - defaultNamespace, - experimentFlags.toList - ) + config.defaultNamespace, + experimentFlags) generator match { - case g: ScalaGenerator => g.warnOnJavaNamespaceFallback = scalaWarnOnJavaNSFallback + case g: ScalaGenerator => g.warnOnJavaNamespaceFallback = config.scalaWarnOnJavaNSFallback + case g: ApacheJavaGenerator => g.serEnumType = config.javaSerEnumType case _ => () } val generatedFiles = generator( - flags.toSet, - new File(destFolder), - dryRun + config.flags, + new File(config.destFolder), + config.dryRun, + config.genAdapt ).map { _.getPath } - if (verbose) { + if (config.verbose) { println("+ Generated %s".format(generatedFiles.mkString(", "))) } fileMapWriter.foreach { w => diff --git a/src/scala/io/bazel/rules_scala/scrooge_support/ScroogeOptionParser.scala b/src/scala/io/bazel/rules_scala/scrooge_support/ScroogeOptionParser.scala new file mode 100644 index 000000000..af8670d97 --- /dev/null +++ b/src/scala/io/bazel/rules_scala/scrooge_support/ScroogeOptionParser.scala @@ -0,0 +1,203 @@ +/* + * Copyright 2020 Twitter, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.bazel.rules_scala.scrooge_support + +import com.twitter.scrooge.backend.{ + GeneratorFactory, + WithFinagle, + WithJavaPassThrough, + ServiceOption +} +import java.io.File +import java.util.Properties +import scopt.OptionParser + +case class ScroogeConfig( + destFolder: String = ".", + includePaths: List[String] = List(), + thriftFiles: List[String] = List(), + flags: Set[ServiceOption] = Set(), + namespaceMappings: Map[String, String] = Map(), + verbose: Boolean = false, + strict: Boolean = true, + genAdapt: Boolean = false, + skipUnchanged: Boolean = false, + languageFlags: Seq[String] = Seq(), + fileMapPath: scala.Option[String] = None, + dryRun: Boolean = false, + language: String = "scala", + defaultNamespace: String = "thrift", + scalaWarnOnJavaNSFallback: Boolean = false, + javaSerEnumType: Boolean = false) + +object ScroogeOptionParser { + + /** Optionally returns config with parsed values from args. + * @param args command line arguments. + * @param defaultConfig config with configurable defaults that is used to store parsed args. + */ + def parseOptions( + args: Seq[String], + defaultConfig: ScroogeConfig = ScroogeConfig() + ): Option[ScroogeConfig] = { + val buildProperties = new Properties + scala.Option(getClass.getResource("build.properties")) foreach { resource => + buildProperties.load(resource.openStream) + } + + val parser = new OptionParser[ScroogeConfig]("scrooge") { + help("help").text("show this help screen") + + override def showUsageOnError: Option[Boolean] = Some(true) + + opt[Unit]('V', "version") + .action { (_, c) => + println("scrooge " + buildProperties.getProperty("version", "0.0")) + println(" build " + buildProperties.getProperty("build_name", "unknown")) + println(" git revision " + buildProperties.getProperty("build_revision", "unknown")) + System.exit(0) + c + } + .text("print version and quit") + + opt[Unit]('v', "verbose") + .action((_, c) => c.copy(verbose = true)) + .text("log verbose messages about progress") + + opt[String]('d', "dest") + .valueName("") + .action((d, c) => c.copy(destFolder = d)) + .text("write generated code to a folder (default: %s)".format(defaultConfig.destFolder)) + + opt[String]("import-path") + .unbounded() + .valueName("") + .action { (path, c) => + val includePaths = path.split(File.pathSeparator) ++: c.includePaths + c.copy(includePaths = includePaths) + } + .text( + "[DEPRECATED] path(s) to search for included thrift files (may be used multiple times)" + ) + + opt[String]('i', "include-path") + .unbounded() + .valueName("") + .action { (path, c) => + val includePaths = path.split(File.pathSeparator) ++: c.includePaths + c.copy(includePaths = includePaths) + } + .text("path(s) to search for included thrift files (may be used multiple times)") + + opt[String]('n', "namespace-map") + .unbounded() + .valueName("=") + .action { (mapping, c) => + mapping.split("=") match { + case Array(from, to) => + c.copy(namespaceMappings = c.namespaceMappings + (from -> to)) + } + } + .text("map old namespace to new (may be used multiple times)") + + opt[String]("default-java-namespace") + .unbounded() + .valueName("") + .action((name, c) => c.copy(defaultNamespace = name)) + .text( + "Use as default namespace if the thrift file doesn't define its own namespace. " + + "If this option is not specified either, then use \"thrift\" as default namespace" + ) + + opt[Unit]("disable-strict") + .action((_, c) => c.copy(strict = false)) + .text("issue warnings on non-severe parse errors instead of aborting") + + opt[String]("gen-file-map") + .valueName("") + .action((path, c) => c.copy(fileMapPath = Some(path))) + .text( + "generate map.txt in the destination folder to specify the mapping from input thrift files to output Scala/Java files" + ) + + opt[Unit]("dry-run") + .action((_, c) => c.copy(dryRun = true)) + .text( + "parses and validates source thrift files, reporting any errors, but" + + " does not emit any generated source code. can be used with " + + "--gen-file-mapping to get the file mapping" + ) + + opt[Unit]('s', "skip-unchanged") + .action((_, c) => c.copy(skipUnchanged = true)) + .text("Don't re-generate if the target is newer than the input") + + opt[String]('l', "language") + .action((languageString, c) => c.copy(language = languageString)) + .validate { language => + if (GeneratorFactory.languages.toList contains language.toLowerCase) + success + else + failure("language option %s not supported".format(language)) + } + .text( + "name of language to generate code in (currently supported languages: " + + GeneratorFactory.languages.toList.mkString(", ") + ")" + ) + + opt[Unit]("java-ser-enum-type") + .action((_, c) => c.copy(javaSerEnumType = true)) + .text("Encode a thrift enum as o.a.t.p.TType.ENUM instead of TType.I32") + + opt[String]("language-flag") + .valueName("") + .unbounded() + .action { (flag, c) => + val languageFlags = c.languageFlags :+ flag + c.copy(languageFlags = languageFlags) + } + .text( + "Pass arguments to supported language generators" + ) + + opt[Unit]("scala-warn-on-java-ns-fallback") + .action((_, c) => c.copy(scalaWarnOnJavaNSFallback = true)) + .text("Print a warning when the scala generator falls back to the java namespace") + + opt[Unit]("finagle") + .action((_, c) => c.copy(flags = c.flags + WithFinagle)) + .text("generate finagle classes") + + opt[Unit]("gen-adapt") + .action((_, c) => c.copy(genAdapt = true)) + .text("Generate code for adaptive decoding for scala.") + + opt[Unit]("java-passthrough") + .action((_, c) => c.copy(flags = c.flags + WithJavaPassThrough)) + .text("Generate Java code with PassThrough") + + arg[String]("") + .unbounded() + .action { (files, c) => + // append files to preserve the order of thrift files from command line. + val thriftFiles = c.thriftFiles :+ files + c.copy(thriftFiles = thriftFiles) + } + .text("thrift files to compile") + } + parser.parse(args, defaultConfig) + } +} diff --git a/src/scala/scripts/BUILD b/src/scala/scripts/BUILD index cd1643579..024cba09f 100644 --- a/src/scala/scripts/BUILD +++ b/src/scala/scripts/BUILD @@ -10,6 +10,8 @@ scala_library( "//src/java/io/bazel/rulesscala/worker", "//src/scala/io/bazel/rules_scala/scrooge_support:compiler", "//twitter_scrooge:scrooge_generator_classpath", + # Remove this dependency when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge + "//src/scala/io/bazel/rules_scala/scrooge_support:option_parser", ], ) diff --git a/src/scala/scripts/ScroogeWorker.scala b/src/scala/scripts/ScroogeWorker.scala index 0fa0ef063..3218d879c 100644 --- a/src/scala/scripts/ScroogeWorker.scala +++ b/src/scala/scripts/ScroogeWorker.scala @@ -1,8 +1,11 @@ package scripts + import java.io.File import java.nio.file.{Files, Path, Paths} +// Move ScroogeConfig and ScroogeOptionParser to com.twitter.scrooge when these changes are added to scrooge +import io.bazel.rules_scala.scrooge_support.{ Compiler, CompilerDefaults, ScroogeConfig, ScroogeOptionParser } import com.twitter.scrooge.backend.WithFinagle import io.bazel.rules_scala.scrooge_support.{Compiler, CompilerDefaults} import io.bazel.rulesscala.io_utils.DeleteRecursively @@ -51,27 +54,12 @@ object ScroogeWorker extends Worker.Interface { // Further configuration options for scrooge. val additionalFlags = getIdx(5) + val thriftSources = immediateThriftSrcJars ++ remoteSelfThriftSources + val flags = additionalFlags :+ thriftSources.mkString(" ") val tmp = Paths.get(Option(System.getProperty("java.io.tmpdir")).getOrElse("/tmp")) val scroogeOutput = Files.createTempDirectory(tmp, "scrooge") - val scrooge = new Compiler - - additionalFlags.foreach { - case "--with-finagle" => { - scrooge.flags += WithFinagle - } - case f if f.startsWith("--language=") => { - scrooge.language = f.split("=")(1) - } - case flag => sys.error(s"Unrecognized scrooge flag: ${flag}. This is probably coming from your rule using our custom scrooge wrapper directly. Please modify this wrappers script to correctly handle this additional flag, or remove that flag from the call to the wrapper.") - } - - scrooge.compileJars ++= immediateThriftSrcJars - scrooge.compileJars ++= remoteSelfThriftSources - scrooge.includeJars ++= onlyTransitiveThriftSrcJars - scrooge.includeJars ++= remoteSrcJars - // should check that we have no overlap in any of the types // we are just going to try extracting EVERYTHING to the same tree, and we will just error if there // are more than one. @@ -86,10 +74,19 @@ object ScroogeWorker extends Worker.Interface { sys.error("onlyTransitiveThriftSrcs and immediateThriftSrcs should " + s"have not intersection, found: ${intersect.mkString(",")}") + // To preserve current default behaviour. + val defaultConfig = ScroogeConfig( + destFolder = scroogeOutput.toString, + includePaths = onlyTransitiveThriftSrcJars ++ remoteSrcJars, + // always add finagle option which is a no-op if there are no services + flags = Set(WithFinagle), + strict = false) + + val scrooge = ScroogeOptionParser.parseOptions(flags, defaultConfig) + .map(cfg => new Compiler(cfg)) + .getOrElse(throw new IllegalArgumentException(s"Failed to parse compiler args: ${args.mkString(",")}")) + val dirsToDelete = Set(scroogeOutput) - scrooge.destFolder = scroogeOutput.toString - //TODO we should make this configurable - scrooge.strict = false scrooge.run() JarCreator.buildJar(Array(jarOutput, scroogeOutput.toString)) diff --git a/third_party/repositories/scala_2_11.bzl b/third_party/repositories/scala_2_11.bzl index f72847d7c..fc2f31c06 100644 --- a/third_party/repositories/scala_2_11.bzl +++ b/third_party/repositories/scala_2_11.bzl @@ -413,6 +413,11 @@ artifacts = { "artifact": "javax.annotation:javax.annotation-api:1.3.2", "sha256": "e04ba5195bcd555dc95650f7cc614d151e4bcd52d29a10b8aa2197f3ab89ab9b", }, + # Remove this artifact when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge + "io_bazel_rules_scala_scopt": { + "artifact": "com.github.scopt:scopt_2.11:4.0.0-RC2", + "sha256": "956dfc89d3208e4a6d8bbfe0205410c082cee90c4ce08be30f97c044dffc3435", + }, # test only "com_twitter__scalding_date": { diff --git a/third_party/repositories/scala_2_12.bzl b/third_party/repositories/scala_2_12.bzl index 86d9df818..d742f7e4d 100644 --- a/third_party/repositories/scala_2_12.bzl +++ b/third_party/repositories/scala_2_12.bzl @@ -413,6 +413,11 @@ artifacts = { "artifact": "javax.annotation:javax.annotation-api:1.3.2", "sha256": "e04ba5195bcd555dc95650f7cc614d151e4bcd52d29a10b8aa2197f3ab89ab9b", }, + # Remove this artifact when ScroogeConfig and ScroogeOptionParser are added to the com.twitter.scrooge. + "io_bazel_rules_scala_scopt": { + "artifact": "com.github.scopt:scopt_2.12:4.0.0-RC2", + "sha256": "d19a4e8b8c013a56e03bc57bdf87abe6297c974cf907585d00284eae61c6ac91", + }, # test only "com_twitter__scalding_date": { diff --git a/thrift/thrift.bzl b/thrift/thrift.bzl index 5aeefb5bd..d3e87542b 100644 --- a/thrift/thrift.bzl +++ b/thrift/thrift.bzl @@ -160,6 +160,7 @@ thrift_library = rule( # This is a list of JARs which only Thrift files # these files WILL be compiled as part of the current target "external_jars": attr.label_list(allow_files = [".jar"]), + "compiler_args": attr.string_list(), "_zipper": attr.label( executable = True, cfg = "exec", diff --git a/twitter_scrooge/BUILD b/twitter_scrooge/BUILD index 1bd1a0a98..35cfd084f 100644 --- a/twitter_scrooge/BUILD +++ b/twitter_scrooge/BUILD @@ -51,6 +51,7 @@ declare_deps_provider( deps_id = "compiler_classpath", visibility = ["//visibility:public"], deps = [ + "//external:io_bazel_rules_scala/dependency/thrift/mustache", "//external:io_bazel_rules_scala/dependency/thrift/scrooge_generator", "//external:io_bazel_rules_scala/dependency/thrift/util_core", "//external:io_bazel_rules_scala/dependency/thrift/util_logging", diff --git a/twitter_scrooge/twitter_scrooge.bzl b/twitter_scrooge/twitter_scrooge.bzl index f4061b9da..4c1286c65 100644 --- a/twitter_scrooge/twitter_scrooge.bzl +++ b/twitter_scrooge/twitter_scrooge.bzl @@ -165,15 +165,16 @@ def _generate_jvm_code(ctx, label, compile_thrifts, include_thrifts, jar_output, worker_arg_pad + _colon_paths(ps) for ps in [compile_thrifts, include_thrifts, [], []] ]) + + compiler_args = getattr(ctx.rule.attr, "compiler_args", []) + lang_flag = ["--language", language] + flags = compiler_args + lang_flag + worker_content = "{output}\n{paths}\n{flags}".format( output = jar_output.path, paths = path_content, - flags = worker_arg_pad + ":".join([ - # always add finagle option which is a no-op if there are no services - # we could put "include_services" on thrift_info, if needed - "--with-finagle", - "--language={}".format(language), - ]), + # we could put "include_services" on thrift_info, if needed + flags = worker_arg_pad + ":".join(flags), ) # Since we may want to generate several languages from this thrift target,