Skip to content

Make compile scala callable #540

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Make compile scala callable #540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 47 additions & 29 deletions scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,17 +132,33 @@ def _expand_location(ctx, flags):
def _join_path(args, sep = ","):
return sep.join([f.path for f in args])

def _compile_scala(ctx, output, sources, cjars, all_srcjars, buildijar,
transitive_compile_jars, resource_strip_prefix, resources,
resource_jars, labels, in_scalacopts):
def compile_scala(ctx,
target_label,
output,
manifest,
statsfile,
sources,
cjars,
all_srcjars,
buildijar,
transitive_compile_jars,
plugins,
resource_strip_prefix,
resources,
resource_jars,
labels,
in_scalacopts,
print_compile_time,
expect_java_output,
scalac_jvm_flags = []):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the scalac_jvn_flags have a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have virtually never used this in practice and I just put it in. Would you prefer I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"we have virtually never used this in practice" you mean stripe, right? Because the code uses it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just remove it since it's part of the signature. Maybe your default makes sense. It's mainly surprising that it's the only one with a default value so it can confuse a future reader about whether this means something in particular

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant removing the default. Of course we can (and I assume will) iterate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to keep it as is (if it's alright with you) and remove the default

ijar_output_path = ""
ijar_cmd_path = ""
if buildijar:
ijar_output_path = ctx.outputs.ijar.path
ijar_cmd_path = ctx.executable._ijar.path

# look for any plugins:
plugins = _collect_plugin_paths(ctx.attr.plugins)
plugins = _collect_plugin_paths(plugins)
dependency_analyzer_plugin_jars = []
dependency_analyzer_mode = "off"
compiler_classpath_jars = cjars
Expand All @@ -163,7 +179,7 @@ def _compile_scala(ctx, output, sources, cjars, all_srcjars, buildijar,
transitive_cjars_list = transitive_compile_jars.to_list()
indirect_jars = _join_path(transitive_cjars_list)
indirect_targets = ",".join([labels[j.path] for j in transitive_cjars_list])
current_target = str(ctx.label)
current_target = str(target_label)

optional_scalac_args = """
DirectJars: {direct_jars}
Expand All @@ -176,7 +192,8 @@ CurrentTarget: {current_target}
indirect_targets = indirect_targets,
current_target = current_target)

plugin_arg = _join_path(plugins.to_list())
plugins_list = plugins.to_list()
plugin_arg = _join_path(plugins_list)

separator = ctx.configuration.host_path_separator
compiler_classpath = _join_path(compiler_classpath_jars.to_list(), separator)
Expand Down Expand Up @@ -207,10 +224,10 @@ DependencyAnalyzerMode: {dependency_analyzer_mode}
StatsfileOutput: {statsfile_output}
""".format(
out = output.path,
manifest = ctx.outputs.manifest.path,
manifest = manifest.path,
scala_opts = ",".join(scalacopts),
print_compile_time = ctx.attr.print_compile_time,
expect_java_output = ctx.attr.expect_java_output,
print_compile_time = print_compile_time,
expect_java_output = expect_java_output,
plugin_arg = plugin_arg,
cp = compiler_classpath,
classpath_resource_src = _join_path(classpath_resources),
Expand All @@ -229,27 +246,26 @@ StatsfileOutput: {statsfile_output}
resource_strip_prefix = resource_strip_prefix,
resource_jars = _join_path(resource_jars),
dependency_analyzer_mode = dependency_analyzer_mode,
statsfile_output = ctx.outputs.statsfile.path)
statsfile_output = statsfile.path)
argfile = ctx.actions.declare_file(
"%s_worker_input" % ctx.label.name, sibling = output)
"%s_scalac_worker_input" % target_label.name, sibling = output)

ctx.actions.write(
output = argfile, content = scalac_args + optional_scalac_args)

outs = [output, ctx.outputs.statsfile]
outs = [output, statsfile]
if buildijar:
outs.extend([ctx.outputs.ijar])
ins = (
compiler_classpath_jars.to_list() + all_srcjars.to_list() +
list(sources) + ctx.files.plugins + dependency_analyzer_plugin_jars +
classpath_resources + resources + resource_jars + ctx.files._java_runtime
+ [ctx.outputs.manifest, ctx.executable._ijar, argfile])
compiler_classpath_jars.to_list() + all_srcjars.to_list() + list(sources)
+ plugins_list + dependency_analyzer_plugin_jars + classpath_resources +
resources + resource_jars + [manifest, ctx.executable._ijar, argfile])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to ctx.files._java_runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should have been removed a long time ago. It was for when we were calling the java compiler ourselves. It is not needed to call ijar which is a c++ program (which we shouldn’t be calling this way anyway).

ctx.actions.run(
inputs = ins,
outputs = outs,
executable = ctx.executable._scalac,
mnemonic = "Scalac",
progress_message = "scala %s" % ctx.label,
progress_message = "scala %s" % target_label,
execution_requirements = {"supports-workers": "1"},
# when we run with a worker, the `@argfile.path` is removed and passed
# line by line as arguments in the protobuf. In that case,
Expand All @@ -260,8 +276,7 @@ StatsfileOutput: {statsfile_output}
# be correctly handled since the executable is a jvm app that will
# consume the flags on startup.
arguments = [
"--jvm_flag=%s" % f
for f in _expand_location(ctx, ctx.attr.scalac_jvm_flags)
"--jvm_flag=%s" % f for f in _expand_location(ctx, scalac_jvm_flags)
] + ["@" + argfile.path],
)

Expand Down Expand Up @@ -314,8 +329,8 @@ def collect_java_providers_of(deps):
providers.append(dep[JavaInfo])
return providers

def _compile_or_empty(ctx, jars, srcjars, buildijar, transitive_compile_jars,
jars2labels,
def _compile_or_empty(ctx, manifest, jars, srcjars, buildijar,
transitive_compile_jars, jars2labels,
implicit_junit_deps_needed_for_java_compilation):
# We assume that if a srcjar is present, it is not empty
if len(ctx.files.srcs) + len(srcjars.to_list()) == 0:
Expand All @@ -332,10 +347,13 @@ def _compile_or_empty(ctx, jars, srcjars, buildijar, transitive_compile_jars,
all_srcjars = depset(in_srcjars, transitive = [srcjars])
java_srcs = _java_filetype.filter(ctx.files.srcs)
sources = _scala_filetype.filter(ctx.files.srcs) + java_srcs
_compile_scala(ctx, ctx.outputs.jar, sources, jars, all_srcjars, buildijar,
transitive_compile_jars, ctx.attr.resource_strip_prefix,
ctx.files.resources, ctx.files.resource_jars, jars2labels,
ctx.attr.scalacopts)
compile_scala(ctx, ctx.label, ctx.outputs.jar, manifest,
ctx.outputs.statsfile, sources, jars, all_srcjars, buildijar,
transitive_compile_jars, ctx.attr.plugins,
ctx.attr.resource_strip_prefix, ctx.files.resources,
ctx.files.resource_jars, jars2labels, ctx.attr.scalacopts,
ctx.attr.print_compile_time, ctx.attr.expect_java_output,
ctx.attr.scalac_jvm_flags)
# compile the java now
if buildijar:
scala_output = ctx.outputs.ijar
Expand Down Expand Up @@ -511,9 +529,9 @@ def _lib(ctx, non_macro_lib):
(cjars, transitive_rjars) = (jars.compile_jars, jars.transitive_runtime_jars)

write_manifest(ctx)
outputs = _compile_or_empty(ctx, cjars, srcjars, non_macro_lib,
jars.transitive_compile_jars, jars.jars2labels,
[])
outputs = _compile_or_empty(ctx, ctx.outputs.manifest, cjars, srcjars,
non_macro_lib, jars.transitive_compile_jars,
jars.jars2labels, [])

transitive_rjars = depset(outputs.full_jars, transitive = [transitive_rjars])

Expand Down Expand Up @@ -586,7 +604,7 @@ def _scala_binary_common(ctx,
java_wrapper,
implicit_junit_deps_needed_for_java_compilation = []):
write_manifest(ctx)
outputs = _compile_or_empty(ctx, cjars, depset(), False,
outputs = _compile_or_empty(ctx, ctx.outputs.manifest, cjars, depset(), False,
transitive_compile_time_jars, jars2labels,
implicit_junit_deps_needed_for_java_compilation
) # no need to build an ijar for an executable
Expand Down