Skip to content

Use java_common.run_ijar #538

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 6 commits into from
Jun 27, 2018
Merged

Use java_common.run_ijar #538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee0922a
Use java_common.run_ijar
johnynek Jun 26, 2018
fefefe3
merge with master
johnynek Jun 27, 2018
664cb76
run lint fix
johnynek Jun 27, 2018
c9a70a4
try to fix a test
johnynek Jun 27, 2018
e11597d
fix up the tests
johnynek Jun 27, 2018
adf0cd9
try one more time
johnynek Jun 27, 2018
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
79 changes: 24 additions & 55 deletions scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,9 @@ def _add_resources_cmd(ctx):
res_cmd.extend([line])
return "".join(res_cmd)

def _build_nosrc_jar(ctx, buildijar):
def _build_nosrc_jar(ctx):
resources = _add_resources_cmd(ctx)
ijar_cmd = ""
if buildijar:
ijar_cmd = "\ncp {jar_output} {ijar_output}\n".format(
jar_output = ctx.outputs.jar.path, ijar_output = ctx.outputs.ijar.path)

# this ensures the file is not empty
resources += "META-INF/MANIFEST.MF=%s\n" % ctx.outputs.manifest.path
Expand All @@ -97,8 +94,6 @@ touch {statsfile}
)

outs = [ctx.outputs.jar, ctx.outputs.statsfile]
if buildijar:
outs.extend([ctx.outputs.ijar])

inputs = ctx.files.resources + [
ctx.outputs.manifest, ctx.executable._zipper, zipper_arg_path
Expand Down Expand Up @@ -132,31 +127,11 @@ def _expand_location(ctx, flags):
def _join_path(args, sep = ","):
return sep.join([f.path for f in args])

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 = []):
ijar_output_path = ""
ijar_cmd_path = ""
if buildijar:
ijar_output_path = ctx.outputs.ijar.path
ijar_cmd_path = ctx.executable._ijar.path

def compile_scala(ctx, target_label, output, manifest, statsfile, sources,
cjars, all_srcjars, transitive_compile_jars, plugins,
resource_strip_prefix, resources, resource_jars, labels,
in_scalacopts, print_compile_time, expect_java_output,
scalac_jvm_flags):
# look for any plugins:
plugins = _collect_plugin_paths(plugins)
dependency_analyzer_plugin_jars = []
Expand Down Expand Up @@ -204,10 +179,7 @@ CurrentTarget: {current_target}
scalac_args = """
Classpath: {cp}
ClasspathResourceSrcs: {classpath_resource_src}
EnableIjar: {enableijar}
Files: {files}
IjarCmdPath: {ijar_cmd_path}
IjarOutput: {ijar_out}
JarOutput: {out}
Manifest: {manifest}
Plugins: {plugin_arg}
Expand All @@ -232,9 +204,6 @@ StatsfileOutput: {statsfile_output}
cp = compiler_classpath,
classpath_resource_src = _join_path(classpath_resources),
files = _join_path(sources),
enableijar = buildijar,
ijar_out = ijar_output_path,
ijar_cmd_path = ijar_cmd_path,
srcjars = _join_path(all_srcjars.to_list()),
# the resource paths need to be aligned in order
resource_src = ",".join([f.path for f in resources]),
Expand All @@ -254,12 +223,10 @@ StatsfileOutput: {statsfile_output}
output = argfile, content = scalac_args + optional_scalac_args)

outs = [output, statsfile]
if buildijar:
outs.extend([ctx.outputs.ijar])
ins = (
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])
ins = (compiler_classpath_jars.to_list() + all_srcjars.to_list() +
list(sources) + plugins_list + dependency_analyzer_plugin_jars +
classpath_resources + resources + resource_jars + [manifest, argfile])

ctx.actions.run(
inputs = ins,
outputs = outs,
Expand Down Expand Up @@ -334,7 +301,7 @@ def _compile_or_empty(ctx, manifest, jars, srcjars, buildijar,
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:
_build_nosrc_jar(ctx, buildijar)
_build_nosrc_jar(ctx)
# no need to build ijar when empty
return struct(
ijar = ctx.outputs.jar,
Expand All @@ -355,28 +322,30 @@ def _compile_or_empty(ctx, manifest, jars, srcjars, buildijar,
f for f in ctx.files.srcs if f.basename.endswith(_scala_extension)
] + java_srcs
compile_scala(ctx, ctx.label, ctx.outputs.jar, manifest,
ctx.outputs.statsfile, sources, jars, all_srcjars, buildijar,
ctx.outputs.statsfile, sources, jars, all_srcjars,
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
else:
scala_output = ctx.outputs.jar
java_jar = try_to_compile_java_jar(
ctx, scala_output, all_srcjars, java_srcs,
implicit_junit_deps_needed_for_java_compilation)

ijar = None
# build ijar if needed
if buildijar:
ijar = ctx.outputs.ijar
ijar = java_common.run_ijar(
ctx.actions,
jar = ctx.outputs.jar,
target_label = ctx.label,
java_toolchain = ctx.attr._java_toolchain)
else:
# macro code needs to be available at compile-time,
# so set ijar == jar
ijar = ctx.outputs.jar

# compile the java now
java_jar = try_to_compile_java_jar(
ctx, ijar, all_srcjars, java_srcs,
implicit_junit_deps_needed_for_java_compilation)

full_jars = [ctx.outputs.jar]
ijars = [ijar]
if java_jar:
Expand Down
8 changes: 0 additions & 8 deletions scala/scala.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ _implicit_deps = {
cfg = "host",
default = Label("@bazel_tools//tools/jdk:singlejar"),
allow_files = True),
"_ijar": attr.label(
executable = True,
cfg = "host",
default = Label("@bazel_tools//tools/jdk:ijar"),
allow_files = True),
"_scalac": attr.label(
executable = True,
cfg = "host",
Expand Down Expand Up @@ -146,9 +141,6 @@ _common_outputs = {

_library_outputs = {}
_library_outputs.update(_common_outputs)
_library_outputs.update({
"ijar": "%{name}_ijar.jar",
})

_scala_library_attrs = {}
_scala_library_attrs.update(_implicit_deps)
Expand Down
13 changes: 0 additions & 13 deletions src/java/io/bazel/rulesscala/scalac/CompileOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ public class CompileOptions {
public final String classpath;
public final String[] files;
public final String[] sourceJars;
public final boolean iJarEnabled;
public final String ijarOutput;
public final String ijarCmdPath;
public final String[] javaFiles;
public final Map<String, Resource> resourceFiles;
public final String resourceStripPrefix;
Expand Down Expand Up @@ -49,16 +46,6 @@ public CompileOptions(List<String> args) {
}

sourceJars = getCommaList(argMap, "SourceJars");
iJarEnabled = booleanGetOrFalse(argMap, "EnableIjar");
if (iJarEnabled) {
ijarOutput =
getOrError(argMap, "IjarOutput", "Missing required arg ijarOutput when ijar enabled");
ijarCmdPath =
getOrError(argMap, "IjarCmdPath", "Missing required arg ijarCmdPath when ijar enabled");
} else {
ijarOutput = null;
ijarCmdPath = null;
}
resourceFiles = getResources(argMap);
resourceStripPrefix = getOrEmpty(argMap, "ResourceStripPrefix");
resourceJars = getCommaList(argMap, "ResourceJars");
Expand Down
13 changes: 0 additions & 13 deletions src/java/io/bazel/rulesscala/scalac/ScalacProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,6 @@ public void processRequest(List<String> args) throws Exception {
/** Now build the output jar */
String[] jarCreatorArgs = {"-m", ops.manifestPath, outputPath.toString(), tmpPath.toString()};
JarCreator.main(jarCreatorArgs);

/** Now build the output ijar */
if (ops.iJarEnabled) {
Process iostat =
new ProcessBuilder()
.command(ops.ijarCmdPath, ops.outputName, ops.ijarOutput)
.inheritIO()
.start();
int exitCode = iostat.waitFor();
if (exitCode != 0) {
throw new RuntimeException("ijar process failed!");
}
}
} finally {
removeTmp(tmpPath);
}
Expand Down
9 changes: 3 additions & 6 deletions test_rules_scala.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,9 @@ test_scala_library_expect_failure_on_missing_direct_deps_warn_mode() {

test_scala_library_expect_failure_on_missing_direct_java() {
dependency_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency'
#since bazel 0.12.0 the labels are only emmitted if ijar is in play
dependency_file='test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_ijar.jar'
test_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency_java_user'

expected_message="$dependency_file.*$test_target"
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't sound like the right approach. Can you share what the error message was when it failed? this expected message is too generic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it no longer prints the ijar. Which you shouldn't have as far as I can tell. Users shouldn't need to worry about it.

It still fails, and if you follow the recommendation, it still gives the correct next step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: I'm pretty convinced we need to move the jdeps approach and away from this custom approach that runs at analysis time (vs build time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the failure though: https://travis-ci.org/bazelbuild/rules_scala/jobs/397490178#L919

Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain why you shouldn't have the ijar?

Re approach:

  1. As far as I understand they are trying to get rid of jdeps since the params passing is too expensive and instead just stamp each jar with a canonical label and if an infraction is spotted pay the cost of extracting the label from the jar.
  2. Our approach is very similar to the jdeps one where we only warn/error in build time (in the compiler plugin); Our difference is in how we collect the list of labels/jars to pass.
  3. We want to align ourselves with the mainstream approach and I've been working with Google people about a shared design for this

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, also the test that fails now was changed in #505 and it looks like that for it too the above assertion should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I see how I goofed. I think this is better. What do you think of the current change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrote on the commit. I don’t think it’s enough.
IMHO the point is to show the transitive_target and the test_target since they’re both related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I pushed one more time.

These tests don't work when run multiple times, which is annoying. It looks like you are relying on the side effect of building, which doesn't happen on the second build with a warn mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it’s annoying. There’s no way to re trigger warnings and this issue on CI only started happening with the caching which is pretty recent. The real solution is bazel integration testing since we have a reproducible sandbox for each scenario

expected_message="$dependency_target.*$test_target"

test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message}" $test_target "--strict_java_deps=error"
}
Expand All @@ -194,7 +192,7 @@ test_scala_library_expect_failure_on_java_in_src_jar_when_disabled() {
}

test_scala_library_expect_better_failure_message_on_missing_transitive_dependency_labels_from_other_jvm_rules() {
transitive_target='.*transitive_dependency_ijar.jar'
transitive_target='.*transitive_dependency-ijar.jar'
direct_target='//test_expect_failure/missing_direct_deps/internal_deps:direct_java_provider_dependency'
test_target='//test_expect_failure/missing_direct_deps/internal_deps:dependent_on_some_java_provider'

Expand All @@ -205,10 +203,9 @@ test_scala_library_expect_better_failure_message_on_missing_transitive_dependenc

test_scala_library_expect_failure_on_missing_direct_deps_warn_mode_java() {
dependency_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency'
dependency_file='test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_ijar.jar'
test_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency_java_user'

local expected_message="$dependency_file.*$test_target"
local expected_message="$dependency_target.*$test_target"

test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message}" ${test_target} "--strict_java_deps=warn" "ne"
}
Expand Down