Skip to content

Commit 664a7ad

Browse files
cattibriecattibrie
authored
Support scrooge-generator compiler flags in thrift_library rule (#1228)
* Support scrooge-generator compiler flags in thrift_library rule (#6) * Add option parsing file * Add OptionParser * Add scopt dependency * Lint reformat * Add option parsing file * Add scopt dependency * Lint reformat * Add test for compiler_args in thrift_library * Fix lint error * Remove option parser from rules_scala code * Add scopt dependency * Update test and clean code
1 parent 06aec8d commit 664a7ad

File tree

13 files changed

+164
-68
lines changed

13 files changed

+164
-68
lines changed

‎src/scala/io/bazel/rules_scala/scrooge_support/Compiler.scala‎

Lines changed: 25 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -33,20 +33,16 @@ package io.bazel.rules_scala.scrooge_support
3333

3434
import com.twitter.scrooge._
3535
import com.twitter.scrooge.ast.Document
36-
import com.twitter.scrooge.backend.{GeneratorFactory, ScalaGenerator, ServiceOption}
37-
import com.twitter.scrooge.frontend.{FileParseException, TypeResolver, ThriftParser, Importer, MultiImporter, ZipImporter}
38-
import java.io.{File, FileWriter}
36+
import com.twitter.scrooge.backend.{ GeneratorFactory, ScalaGenerator }
37+
import com.twitter.scrooge.frontend.{ FileParseException, TypeResolver, ThriftParser }
38+
import com.twitter.scrooge.java_generator.ApacheJavaGenerator
39+
import java.io.{ File, FileWriter }
3940
import java.nio.file.Paths
4041
import java.util.jar.{ JarFile, JarEntry }
4142
import java.util.logging.Level
4243
import scala.collection.concurrent.TrieMap
43-
import scala.collection.mutable
44-
import scala.collection.JavaConverters._
4544

4645
object CompilerDefaults {
47-
var language: String = "scala"
48-
var defaultNamespace: String = "thrift"
49-
5046
def listJar(_jar: File): List[String] =
5147
try {
5248
val files = List.newBuilder[String]
@@ -66,46 +62,34 @@ object CompilerDefaults {
6662
}
6763
}
6864

69-
class Compiler {
70-
val defaultDestFolder = "."
71-
var destFolder: String = defaultDestFolder
65+
class Compiler(val config: ScroogeConfig) {
7266
// These are jars we are including, but are not compiling
73-
val includeJars = new mutable.ListBuffer[String]
67+
val includeJars = config.includePaths
7468
// these are the jars we want to compile into scala source jars
75-
val compileJars = new mutable.ListBuffer[String]
76-
val flags = new mutable.HashSet[ServiceOption]
77-
val namespaceMappings = new mutable.HashMap[String, String]
78-
var verbose = false
79-
var strict = true
80-
var skipUnchanged = false
81-
var experimentFlags = new mutable.ListBuffer[String]
82-
var fileMapPath: scala.Option[String] = None
69+
val compileJars = config.thriftFiles
70+
val experimentFlags = config.languageFlags
8371
var fileMapWriter: scala.Option[FileWriter] = None
84-
var dryRun: Boolean = false
85-
var language: String = CompilerDefaults.language
86-
var defaultNamespace: String = CompilerDefaults.defaultNamespace
87-
var scalaWarnOnJavaNSFallback: Boolean = false
8872

8973

9074
def run() {
9175
// if --gen-file-map is specified, prepare the map file.
92-
fileMapWriter = fileMapPath.map { path =>
76+
fileMapWriter = config.fileMapPath.map { path =>
9377
val file = new File(path)
9478
val dir = file.getParentFile
9579
if (dir != null && !dir.exists()) {
9680
dir.mkdirs()
9781
}
98-
if (verbose) {
82+
if (config.verbose) {
9983
println("+ Writing file mapping to %s".format(path))
10084
}
10185
new FileWriter(file)
10286
}
10387

10488
val allJars: List[File] =
105-
((includeJars.toList) ::: (compileJars.toList))
89+
(includeJars ::: compileJars)
10690
.map { path => (new File(path)).getCanonicalFile }
10791

108-
val isJava = language.equals("java")
92+
val isJava = config.language.equals("java")
10993
val documentCache = new TrieMap[String, Document]
11094

11195
// compile
@@ -126,36 +110,37 @@ class Compiler {
126110
val importer = rootImporter.copy(focus = focus) +: rootImporter
127111
val parser = new ThriftParser(
128112
importer,
129-
strict,
113+
config.strict,
130114
defaultOptional = isJava,
131115
skipIncludes = false,
132116
documentCache
133117
)
134118
parser.logger.setLevel(Level.OFF) // scrooge warns on file names with "/"
135-
val doc = parser.parseFile(inputFile).mapNamespaces(namespaceMappings.toMap)
119+
val doc = parser.parseFile(inputFile).mapNamespaces(config.namespaceMappings)
136120

137-
if (verbose) println("+ Compiling %s".format(inputFile))
121+
if (config.verbose) println("+ Compiling %s".format(inputFile))
138122
val resolvedDoc = TypeResolver()(doc)
139123
val generator = GeneratorFactory(
140-
language,
124+
config.language,
141125
resolvedDoc,
142-
defaultNamespace,
143-
experimentFlags.toList
144-
)
126+
config.defaultNamespace,
127+
experimentFlags)
145128

146129
generator match {
147-
case g: ScalaGenerator => g.warnOnJavaNamespaceFallback = scalaWarnOnJavaNSFallback
130+
case g: ScalaGenerator => g.warnOnJavaNamespaceFallback = config.scalaWarnOnJavaNSFallback
131+
case g: ApacheJavaGenerator => g.serEnumType = config.javaSerEnumType
148132
case _ => ()
149133
}
150134

151135
val generatedFiles = generator(
152-
flags.toSet,
153-
new File(destFolder),
154-
dryRun
136+
config.flags,
137+
new File(config.destFolder),
138+
config.dryRun,
139+
config.genAdapt
155140
).map {
156141
_.getPath
157142
}
158-
if (verbose) {
143+
if (config.verbose) {
159144
println("+ Generated %s".format(generatedFiles.mkString(", ")))
160145
}
161146
fileMapWriter.foreach { w =>

‎src/scala/scripts/ScroogeWorker.scala‎

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package scripts
22

3+
34
import java.io.File
4-
import java.nio.file.{Files, Path, Paths}
5+
import java.nio.file.{ Files, Path, Paths }
56

7+
import com.twitter.scrooge.{ ScroogeConfig, ScroogeOptionParser }
68
import com.twitter.scrooge.backend.WithFinagle
7-
import io.bazel.rules_scala.scrooge_support.{Compiler, CompilerDefaults}
9+
import io.bazel.rules_scala.scrooge_support.{ Compiler, CompilerDefaults }
810
import io.bazel.rulesscala.io_utils.DeleteRecursively
911
import io.bazel.rulesscala.jar.JarCreator
1012
import io.bazel.rulesscala.worker.Worker
@@ -51,27 +53,12 @@ object ScroogeWorker extends Worker.Interface {
5153

5254
// Further configuration options for scrooge.
5355
val additionalFlags = getIdx(5)
56+
val thriftSources = immediateThriftSrcJars ++ remoteSelfThriftSources
57+
val flags = additionalFlags :+ thriftSources.mkString(" ")
5458

5559
val tmp = Paths.get(Option(System.getProperty("java.io.tmpdir")).getOrElse("/tmp"))
5660
val scroogeOutput = Files.createTempDirectory(tmp, "scrooge")
5761

58-
val scrooge = new Compiler
59-
60-
additionalFlags.foreach {
61-
case "--with-finagle" => {
62-
scrooge.flags += WithFinagle
63-
}
64-
case f if f.startsWith("--language=") => {
65-
scrooge.language = f.split("=")(1)
66-
}
67-
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.")
68-
}
69-
70-
scrooge.compileJars ++= immediateThriftSrcJars
71-
scrooge.compileJars ++= remoteSelfThriftSources
72-
scrooge.includeJars ++= onlyTransitiveThriftSrcJars
73-
scrooge.includeJars ++= remoteSrcJars
74-
7562
// should check that we have no overlap in any of the types
7663
// we are just going to try extracting EVERYTHING to the same tree, and we will just error if there
7764
// are more than one.
@@ -86,10 +73,19 @@ object ScroogeWorker extends Worker.Interface {
8673
sys.error("onlyTransitiveThriftSrcs and immediateThriftSrcs should " +
8774
s"have not intersection, found: ${intersect.mkString(",")}")
8875

76+
// To preserve current default behaviour.
77+
val defaultConfig = ScroogeConfig(
78+
destFolder = scroogeOutput.toString,
79+
includePaths = onlyTransitiveThriftSrcJars ++ remoteSrcJars,
80+
// always add finagle option which is a no-op if there are no services
81+
flags = Set(WithFinagle),
82+
strict = false)
83+
84+
val scrooge = ScroogeOptionParser.parseOptions(flags, defaultConfig)
85+
.map(cfg => new Compiler(cfg))
86+
.getOrElse(throw new IllegalArgumentException(s"Failed to parse compiler args: ${args.mkString(",")}"))
87+
8988
val dirsToDelete = Set(scroogeOutput)
90-
scrooge.destFolder = scroogeOutput.toString
91-
//TODO we should make this configurable
92-
scrooge.strict = false
9389
scrooge.run()
9490

9591
JarCreator.buildJar(Array(jarOutput, scroogeOutput.toString))

‎test/src/main/scala/scalarules/test/twitter_scrooge/BUILD‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,12 @@ scrooge_scala_library(
133133
],
134134
)
135135

136+
scrooge_java_library(
137+
name = "scrooge5_java",
138+
visibility = ["//visibility:public"],
139+
deps = ["//test/src/main/scala/scalarules/test/twitter_scrooge/thrift/thrift_with_compiler_args:thrift5"],
140+
)
141+
136142
scrooge_scala_library(
137143
name = "scroogebarejar1",
138144
visibility = ["//visibility:public"],
@@ -265,6 +271,7 @@ scala_library(
265271
":scrooge2_a_java",
266272
":scrooge2_b_java",
267273
":scrooge3_java",
274+
":scrooge5_java",
268275
],
269276
)
270277

@@ -344,4 +351,28 @@ sh_test(
344351
data = [":justscrooges_java"],
345352
)
346353

354+
sh_test(
355+
name = "thrift999_in_namespace_in_java_jar",
356+
srcs = ["string_in_filename_in_jar.sh"],
357+
args = [
358+
"true",
359+
"thrift/thrift_with_compiler_args/thrift5_scrooge_java.jar",
360+
"thrift999",
361+
"$(locations :justscrooges_java)",
362+
],
363+
data = [":justscrooges_java"],
364+
)
365+
366+
sh_test(
367+
name = "thrift5_not_in_namespace_in_java_jar",
368+
srcs = ["string_in_filename_in_jar.sh"],
369+
args = [
370+
"false",
371+
"thrift/thrift_with_compiler_args/thrift5_scrooge_java.jar",
372+
"thrift5",
373+
"$(locations :justscrooges_java)",
374+
],
375+
data = [":justscrooges_java"],
376+
)
377+
347378
twitter_scrooge_test_suite()

‎test/src/main/scala/scalarules/test/twitter_scrooge/string_in_filename_in_jar.sh‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
#!/bin/sh
2+
3+
# This is used by a sh_test to test whether a string is
4+
# found in a name of a file inside a jar file.
5+
#
6+
# Args:
7+
# . $1: Whether the string should be in the name of a file (accepted values are "true" or "false").
8+
# . $2: Path to jar file relative to root directory.
9+
# . $3: String to look for in the name of files in the jar file.
10+
# . $4: File - is used to find the path to root directory of the given target.
11+
#
12+
# This is used for instance to check if
13+
# a given jar has a file with a given name.
14+
15+
should_be_in_file=$1
16+
17+
if test "$should_be_in_file" != "true" -a "$should_be_in_file" != "false" ; then
18+
echo "ERROR: Please use only (\"true\" or \"false\") to specify whether you need the substring to be in the file."
19+
echo "Refer to test/src/main/scala/scalarules/test/twitter_scrooge/string_in_jar_file.sh for documentation."
20+
exit 1
21+
fi
22+
23+
dir="$(dirname $4)"
24+
jar_file="$dir/$2"
25+
jar tf ${jar_file} | grep -q $3
26+
file_is_in_jar=$?
27+
28+
if [ $file_is_in_jar -eq 0 ] ; then
29+
if test "$should_be_in_file" = "true" ; then
30+
exit 0
31+
else
32+
echo "ERROR: Found file $3 in ${jar_file}, when we were expecting not to find it."
33+
exit 1
34+
fi
35+
else
36+
if test "$should_be_in_file" = "true" ; then
37+
echo "ERROR: Not found file $3 in ${jar_file}, when we were expecting to find it."
38+
exit 1
39+
else
40+
exit 0
41+
fi
42+
fi

‎test/src/main/scala/scalarules/test/twitter_scrooge/thrift/thrift_with_compiler_args/BUILD‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
load("//thrift:thrift.bzl", "thrift_library")
2+
3+
thrift_library(
4+
name = "thrift5",
5+
srcs = ["Thrift5.thrift"],
6+
compiler_args = [
7+
"--namespace-map",
8+
"scalarules.test.twitter_scrooge.thrift.thrift_with_compiler_args.thrift5=scalarules.test.twitter_scrooge.thrift.thrift_with_compiler_args.thrift999",
9+
],
10+
visibility = ["//visibility:public"],
11+
)

‎test/src/main/scala/scalarules/test/twitter_scrooge/thrift/thrift_with_compiler_args/Thrift5.thrift‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
namespace java scalarules.test.twitter_scrooge.thrift.thrift_with_compiler_args.thrift5
2+
3+
struct Struct5 {
4+
1: string msg
5+
}

‎test_version.sh‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ test_twitter_scrooge_versions() {
8989
"runtime_deps": [\
9090
"@io_bazel_rules_scala_guava",\
9191
"@io_bazel_rules_scala_mustache",\
92+
"@io_bazel_rules_scala_scopt",\
9293
],\
9394
},\
9495
"io_bazel_rules_scala_util_core": {\
@@ -112,6 +113,7 @@ test_twitter_scrooge_versions() {
112113
"runtime_deps": [\
113114
"@io_bazel_rules_scala_guava",\
114115
"@io_bazel_rules_scala_mustache",\
116+
"@io_bazel_rules_scala_scopt",\
115117
],\
116118
},\
117119
"io_bazel_rules_scala_util_core": {\

‎third_party/repositories/scala_2_11.bzl‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,7 @@ artifacts = {
399399
"runtime_deps": [
400400
"@io_bazel_rules_scala_guava",
401401
"@io_bazel_rules_scala_mustache",
402+
"@io_bazel_rules_scala_scopt",
402403
],
403404
},
404405
"io_bazel_rules_scala_util_core": {
@@ -413,6 +414,10 @@ artifacts = {
413414
"artifact": "javax.annotation:javax.annotation-api:1.3.2",
414415
"sha256": "e04ba5195bcd555dc95650f7cc614d151e4bcd52d29a10b8aa2197f3ab89ab9b",
415416
},
417+
"io_bazel_rules_scala_scopt": {
418+
"artifact": "com.github.scopt:scopt_2.11:4.0.0-RC2",
419+
"sha256": "956dfc89d3208e4a6d8bbfe0205410c082cee90c4ce08be30f97c044dffc3435",
420+
},
416421

417422
# test only
418423
"com_twitter__scalding_date": {

‎third_party/repositories/scala_2_12.bzl‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,7 @@ artifacts = {
399399
"runtime_deps": [
400400
"@io_bazel_rules_scala_guava",
401401
"@io_bazel_rules_scala_mustache",
402+
"@io_bazel_rules_scala_scopt",
402403
],
403404
},
404405
"io_bazel_rules_scala_util_core": {
@@ -413,6 +414,10 @@ artifacts = {
413414
"artifact": "javax.annotation:javax.annotation-api:1.3.2",
414415
"sha256": "e04ba5195bcd555dc95650f7cc614d151e4bcd52d29a10b8aa2197f3ab89ab9b",
415416
},
417+
"io_bazel_rules_scala_scopt": {
418+
"artifact": "com.github.scopt:scopt_2.12:4.0.0-RC2",
419+
"sha256": "d19a4e8b8c013a56e03bc57bdf87abe6297c974cf907585d00284eae61c6ac91",
420+
},
416421

417422
# test only
418423
"com_twitter__scalding_date": {

‎third_party/repositories/scala_2_13.bzl‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,7 @@ artifacts = {
405405
"runtime_deps": [
406406
"@io_bazel_rules_scala_guava",
407407
"@io_bazel_rules_scala_mustache",
408+
"@io_bazel_rules_scala_scopt",
408409
],
409410
},
410411
"io_bazel_rules_scala_util_core": {
@@ -419,6 +420,10 @@ artifacts = {
419420
"artifact": "javax.annotation:javax.annotation-api:1.3.2",
420421
"sha256": "e04ba5195bcd555dc95650f7cc614d151e4bcd52d29a10b8aa2197f3ab89ab9b",
421422
},
423+
"io_bazel_rules_scala_scopt": {
424+
"artifact": "com.github.scopt:scopt_2.13:4.0.0-RC2",
425+
"sha256": "07c1937cba53f7509d2ac62a0fc375943a3e0fef346625414c15d41b5a6cfb34",
426+
},
422427

423428
# test only
424429
"com_twitter__scalding_date": {

0 commit comments

Comments
 (0)