From cec58d3174941c6dd0d43879da743a10534cc7c5 Mon Sep 17 00:00:00 2001 From: Jaden Peterson Date: Wed, 30 Oct 2024 12:39:51 -0400 Subject: [PATCH 1/5] Generate OutputGroupInfo in a new phase Previously, it was generated in the "javainfo" phase, which runs before the "depscheck" phase. "javainfo" can't run after "depscheck" because "compile" depends on "javainfo", and "depscheck" can't run before "javainfo" because it depends on "compile". --- docs/newdocs/phases.md | 1 + rules/private/phases.bzl | 3 +++ rules/private/phases/phase_javainfo.bzl | 13 ++----------- rules/private/phases/phase_outputgroupinfo.bzl | 8 ++++++++ rules/scala.bzl | 4 ++++ 5 files changed, 18 insertions(+), 11 deletions(-) create mode 100644 rules/private/phases/phase_outputgroupinfo.bzl diff --git a/docs/newdocs/phases.md b/docs/newdocs/phases.md index 82cbc938..d4cb088b 100644 --- a/docs/newdocs/phases.md +++ b/docs/newdocs/phases.md @@ -15,6 +15,7 @@ def _scala_binary_implementation(ctx): ("ijinfo", _phase_ijinfo), ("binary_deployjar", _phase_binary_deployjar), ("binary_launcher", _phase_binary_launcher), + ("outputgroupinfo", _phase_outputgroupinfo), ("coda", _phase_coda), ]).coda ``` diff --git a/rules/private/phases.bzl b/rules/private/phases.bzl index a5167edd..5d0d02a2 100644 --- a/rules/private/phases.bzl +++ b/rules/private/phases.bzl @@ -9,6 +9,7 @@ load(":phases/phase_ijinfo.bzl", _phase_ijinfo = "phase_ijinfo") load(":phases/phase_javainfo.bzl", _phase_javainfo = "phase_javainfo") load(":phases/phase_library_defaultinfo.bzl", _phase_library_defaultinfo = "phase_library_defaultinfo") load(":phases/phase_noop.bzl", _phase_noop = "phase_noop") +load(":phases/phase_outputgroupinfo.bzl", _phase_outputgroupinfo = "phase_outputgroupinfo") load(":phases/phase_resources.bzl", _phase_resources = "phase_resources") load(":phases/phase_scalafmt_nondefault_outputs.bzl", _phase_scalafmt_nondefault_outputs = "phase_scalafmt_nondefault_outputs") load(":phases/phase_semanticdb.bzl", _phase_semanticdb = "phase_semanticdb") @@ -41,6 +42,8 @@ phase_library_defaultinfo = _phase_library_defaultinfo phase_noop = _phase_noop +phase_outputgroupinfo = _phase_outputgroupinfo + phase_resources = _phase_resources phase_scalafmt_nondefault_outputs = _phase_scalafmt_nondefault_outputs diff --git a/rules/private/phases/phase_javainfo.bzl b/rules/private/phases/phase_javainfo.bzl index c9d19199..4b2c11ad 100644 --- a/rules/private/phases/phase_javainfo.bzl +++ b/rules/private/phases/phase_javainfo.bzl @@ -13,7 +13,6 @@ load( # PHASE: javainfo # # Builds up the JavaInfo provider. And the ScalaInfo, while we're at it. -# And DefaultInfo. # def phase_javainfo(ctx, g): @@ -61,18 +60,10 @@ def phase_javainfo(ctx, g): scala_configuration = g.init.scala_configuration, ) - output_group_info = OutputGroupInfo( - **g.out.output_groups - ) - - g.out.providers.extend([ - output_group_info, - java_info, - scala_info, - ]) + g.out.providers.append(java_info) + g.out.providers.append(scala_info) return struct( java_info = java_info, - output_group_info = output_group_info, scala_info = scala_info, ) diff --git a/rules/private/phases/phase_outputgroupinfo.bzl b/rules/private/phases/phase_outputgroupinfo.bzl new file mode 100644 index 00000000..5a046d8e --- /dev/null +++ b/rules/private/phases/phase_outputgroupinfo.bzl @@ -0,0 +1,8 @@ +# +# PHASE: outputgroupinfo +# +# Generates the `OutputGroupInfo` provider. +# + +def phase_outputgroupinfo(ctx, g): + g.out.providers.append(OutputGroupInfo(**g.out.output_groups)) diff --git a/rules/scala.bzl b/rules/scala.bzl index c2b8d8a8..9729295f 100644 --- a/rules/scala.bzl +++ b/rules/scala.bzl @@ -29,6 +29,7 @@ load( _phase_javainfo = "phase_javainfo", _phase_library_defaultinfo = "phase_library_defaultinfo", _phase_noop = "phase_noop", + _phase_outputgroupinfo = "phase_outputgroupinfo", _phase_resources = "phase_resources", _phase_singlejar = "phase_singlejar", _phase_test_launcher = "phase_test_launcher", @@ -216,6 +217,7 @@ def _scala_library_implementation(ctx): ("coverage", _phase_coverage_jacoco), ("ijinfo", _phase_ijinfo), ("library_defaultinfo", _phase_library_defaultinfo), + ("outputgroupinfo", _phase_outputgroupinfo), ("coda", _phase_coda), ]).coda @@ -230,6 +232,7 @@ def _scala_binary_implementation(ctx): ("ijinfo", _phase_ijinfo), ("binary_deployjar", _phase_binary_deployjar), ("binary_launcher", _phase_binary_launcher), + ("outputgroupinfo", _phase_outputgroupinfo), ("coda", _phase_coda), ]).coda @@ -243,6 +246,7 @@ def _scala_test_implementation(ctx): ("coverage", _phase_coverage_jacoco), ("ijinfo", _phase_ijinfo), ("test_launcher", _phase_test_launcher), + ("outputgroupinfo", _phase_outputgroupinfo), ("coda", _phase_coda), ]).coda From 4809814b9c137da0410f17123d05146f037a58e5 Mon Sep 17 00:00:00 2001 From: Jaden Peterson Date: Wed, 30 Oct 2024 12:42:56 -0400 Subject: [PATCH 2/5] Made dependency checking a validation action --- rules/private/phases/phase_zinc_depscheck.bzl | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/rules/private/phases/phase_zinc_depscheck.bzl b/rules/private/phases/phase_zinc_depscheck.bzl index bd429bbd..ac12a77b 100644 --- a/rules/private/phases/phase_zinc_depscheck.bzl +++ b/rules/private/phases/phase_zinc_depscheck.bzl @@ -20,11 +20,10 @@ def phase_zinc_depscheck(ctx, g): return deps_configuration = ctx.attr.scala[_DepsConfiguration] - - deps_checks = {} labeled_jar_groups = depset(transitive = [dep[_LabeledJars].values for dep in ctx.attr.deps]) - worker_inputs, _, worker_input_manifests = ctx.resolve_command(tools = [deps_configuration.worker]) + outputs = [] + for name in ("direct", "used"): deps_check = ctx.actions.declare_file("{}/depscheck_{}.success".format(ctx.label.name, name)) deps_args = ctx.actions.args() @@ -61,21 +60,16 @@ def phase_zinc_depscheck(ctx, g): ), arguments = [deps_args], ) - deps_checks[name] = deps_check - outputs = [] - if deps_configuration.direct == "error": - outputs.append(deps_checks["direct"]) - if deps_configuration.used == "error": - outputs.append(deps_checks["used"]) + if getattr(deps_configuration, name) == "error": + outputs.append(deps_check) - g.out.output_groups["depscheck"] = depset(outputs) + if "_validation" in g.out.output_groups: + validation_transitive = [g.out.output_groups["_validation"]] + else: + validation_transitive = None - return struct( - checks = deps_checks, - outputs = outputs, - toolchain = deps_configuration, - ) + g.out.output_groups["_validation"] = depset(outputs, transitive = validation_transitive) # If you use avoid using map_each, then labels are converted to their apparent repo name rather than # their canonical repo name. The apparent repo name is the human readable one that we want for use From ce737b012362cfa9a62b5109a0657188d8a6cf3b Mon Sep 17 00:00:00 2001 From: Jaden Peterson Date: Wed, 30 Oct 2024 13:11:27 -0400 Subject: [PATCH 3/5] Don't include "phantom inputs" in singlejar actions This shouldn't be necessary, now that we're using validation actions. --- rules/common/private/utils.bzl | 5 +---- rules/private/phases/phase_binary_launcher.bzl | 2 +- rules/private/phases/phase_library_defaultinfo.bzl | 2 +- rules/private/phases/phase_singlejar.bzl | 5 +---- rules/private/phases/phase_test_launcher.bzl | 7 +------ rules/scala.bzl | 4 ++++ rules/scala/private/provider.bzl | 2 -- 7 files changed, 9 insertions(+), 18 deletions(-) diff --git a/rules/common/private/utils.bzl b/rules/common/private/utils.bzl index b2f044d9..a2df3166 100644 --- a/rules/common/private/utils.bzl +++ b/rules/common/private/utils.bzl @@ -162,7 +162,6 @@ def action_singlejar( ctx, inputs, output, - phantom_inputs = depset(), main_class = None, progress_message = None, resources = {}, @@ -174,8 +173,6 @@ def action_singlejar( if type(inputs) == "list": inputs = depset(inputs) - if type(phantom_inputs) == "list": - phantom_inputs = depset(phantom_inputs) args = ctx.actions.args() args.add("--exclude_build_data") @@ -190,7 +187,7 @@ def action_singlejar( args.set_param_file_format("multiline") args.use_param_file("@%s", use_always = True) - all_inputs = depset(resources.values(), transitive = [inputs, phantom_inputs]) + all_inputs = depset(resources.values(), transitive = [inputs]) ctx.actions.run( arguments = [args], diff --git a/rules/private/phases/phase_binary_launcher.bzl b/rules/private/phases/phase_binary_launcher.bzl index 5d969cea..256664a4 100644 --- a/rules/private/phases/phase_binary_launcher.bzl +++ b/rules/private/phases/phase_binary_launcher.bzl @@ -30,7 +30,7 @@ def phase_binary_launcher(ctx, g): g.out.providers.append(DefaultInfo( executable = ctx.outputs.bin, - files = depset([ctx.outputs.bin, ctx.outputs.jar]), + files = depset([ctx.outputs.bin, ctx.outputs.jar] + g.semanticdb.outputs), runfiles = ctx.runfiles( files = inputs + files, transitive_files = depset( diff --git a/rules/private/phases/phase_library_defaultinfo.bzl b/rules/private/phases/phase_library_defaultinfo.bzl index 3a3ca8bf..029dfdfc 100644 --- a/rules/private/phases/phase_library_defaultinfo.bzl +++ b/rules/private/phases/phase_library_defaultinfo.bzl @@ -6,5 +6,5 @@ def phase_library_defaultinfo(ctx, g): g.out.providers.append(DefaultInfo( - files = depset([ctx.outputs.jar]), + files = depset([ctx.outputs.jar] + g.semanticdb.outputs), )) diff --git a/rules/private/phases/phase_singlejar.bzl b/rules/private/phases/phase_singlejar.bzl index b12d6b50..4bcbc1cf 100644 --- a/rules/private/phases/phase_singlejar.bzl +++ b/rules/private/phases/phase_singlejar.bzl @@ -21,12 +21,9 @@ def phase_singlejar(ctx, g): # cause the build to fail, cleanly, if any declared outputs are # missing from previous phases. inputs = [f for f in ctx.files.resource_jars if f.extension.lower() in ["jar"]] - phantom_inputs = [] for v in [getattr(g, k) for k in dir(g) if k not in ["to_json", "to_proto"]]: if hasattr(v, "jar"): jar = getattr(v, "jar") inputs.append(jar) - if hasattr(v, "outputs"): - phantom_inputs.extend(getattr(v, "outputs")) - _action_singlejar(ctx, inputs, ctx.outputs.jar, phantom_inputs) + _action_singlejar(ctx, inputs, ctx.outputs.jar) diff --git a/rules/private/phases/phase_test_launcher.bzl b/rules/private/phases/phase_test_launcher.bzl index 11854c69..7af3a461 100644 --- a/rules/private/phases/phase_test_launcher.bzl +++ b/rules/private/phases/phase_test_launcher.bzl @@ -2,12 +2,7 @@ load( "@rules_scala_annex//rules/private:coverage_replacements_provider.bzl", _coverage_replacements_provider = "coverage_replacements_provider", ) -load( - "//rules/common:private/utils.bzl", - _action_singlejar = "action_singlejar", - _collect = "collect", - _write_launcher = "write_launcher", -) +load("//rules/common:private/utils.bzl", _collect = "collect", _write_launcher = "write_launcher") # # PHASE: test_launcher diff --git a/rules/scala.bzl b/rules/scala.bzl index 9729295f..59740346 100644 --- a/rules/scala.bzl +++ b/rules/scala.bzl @@ -31,6 +31,7 @@ load( _phase_noop = "phase_noop", _phase_outputgroupinfo = "phase_outputgroupinfo", _phase_resources = "phase_resources", + _phase_semanticdb = "phase_semanticdb", _phase_singlejar = "phase_singlejar", _phase_test_launcher = "phase_test_launcher", _run_phases = "run_phases", @@ -212,6 +213,7 @@ def _scala_library_implementation(ctx): ("resources", _phase_resources), ("classpaths", _phase_classpaths), ("javainfo", _phase_javainfo), + ("semanticdb", _phase_semanticdb), ("compile", _phase_noop), ("singlejar", _phase_singlejar), ("coverage", _phase_coverage_jacoco), @@ -226,6 +228,7 @@ def _scala_binary_implementation(ctx): ("resources", _phase_resources), ("classpaths", _phase_classpaths), ("javainfo", _phase_javainfo), + ("semanticdb", _phase_semanticdb), ("compile", _phase_noop), ("singlejar", _phase_singlejar), ("coverage", _phase_coverage_jacoco), @@ -241,6 +244,7 @@ def _scala_test_implementation(ctx): ("resources", _phase_resources), ("classpaths", _phase_classpaths), ("javainfo", _phase_javainfo), + ("semanticdb", _phase_semanticdb), ("compile", _phase_noop), ("singlejar", _phase_singlejar), ("coverage", _phase_coverage_jacoco), diff --git a/rules/scala/private/provider.bzl b/rules/scala/private/provider.bzl index f377e308..ea82132a 100644 --- a/rules/scala/private/provider.bzl +++ b/rules/scala/private/provider.bzl @@ -9,7 +9,6 @@ load( load( "//rules/private:phases.bzl", _phase_bootstrap_compile = "phase_bootstrap_compile", - _phase_semanticdb = "phase_semanticdb", _phase_zinc_compile = "phase_zinc_compile", _phase_zinc_depscheck = "phase_zinc_depscheck", ) @@ -60,7 +59,6 @@ def configure_zinc_scala_implementation(ctx): _ScalaRulePhase( phases = [ ("=", "compile", "compile", _phase_zinc_compile), - ("-", "compile", "semanticdb", _phase_semanticdb), ("+", "compile", "depscheck", _phase_zinc_depscheck), ], ), From 3704408f187e4ceb619dd39dde08c525a731b942 Mon Sep 17 00:00:00 2001 From: Jaden Peterson Date: Tue, 12 Nov 2024 13:27:28 -0500 Subject: [PATCH 4/5] Test that ScalaCheckDeps outputs aren't inputs to other actions --- .../validation_action/BUILD.bazel | 7 ++++++ tests/dependencies/validation_action/test | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 tests/dependencies/validation_action/BUILD.bazel create mode 100755 tests/dependencies/validation_action/test diff --git a/tests/dependencies/validation_action/BUILD.bazel b/tests/dependencies/validation_action/BUILD.bazel new file mode 100644 index 00000000..bc597af7 --- /dev/null +++ b/tests/dependencies/validation_action/BUILD.bazel @@ -0,0 +1,7 @@ +load("@rules_scala_annex//rules:scala.bzl", "scala_library") + +scala_library( + name = "validation_action", + scala = "//scala:2_13", + tags = ["manual"], +) diff --git a/tests/dependencies/validation_action/test b/tests/dependencies/validation_action/test new file mode 100755 index 00000000..24729326 --- /dev/null +++ b/tests/dependencies/validation_action/test @@ -0,0 +1,24 @@ +#!/bin/bash -e +. "$(dirname "$0")"/../../common.sh + +scalacheckdeps_outputs="$( + bazel aquery 'mnemonic("^ScalaCheckDeps$", //dependencies/validation_action)' | + grep -oP '(?<=^ Outputs: \[).*(?=\]$)' | + sed 's/, /\n/g' | + sort -u +)" + +action_inputs="$( + bazel aquery '//dependencies/validation_action' | + grep -oP '(?<=^ Inputs: \[).*(?=\]$)' | + sed 's/, /\n/g' | + sort -u +)" + +common_files="$(comm -12 <(echo "$scalacheckdeps_outputs") <(echo "$action_inputs"))" + +if [ -n "$common_files" ]; then + echo "Found some ScalaCheckDeps outputs that are the inputs to other actons:" + echo "$common_files" + exit 1 +fi From ac07080ac48500f9ff7a5b9829df045cb1a0a0e2 Mon Sep 17 00:00:00 2001 From: jjudd Date: Tue, 12 Nov 2024 12:54:30 -0700 Subject: [PATCH 5/5] Fix race condition. Also make sure to flush output stream before writing the worker response There was a race condition where a task could be cancelled with the worker output streams still set to null, which would cause a null pointer exception in the worker. We also weren't always flushing the print stream before responding to Bazel about the work request. This fixes that. --- .../common/worker/WorkerMain.scala | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/main/scala/higherkindness/rules_scala/common/worker/WorkerMain.scala b/src/main/scala/higherkindness/rules_scala/common/worker/WorkerMain.scala index 8adf48ba..960e4a44 100644 --- a/src/main/scala/higherkindness/rules_scala/common/worker/WorkerMain.scala +++ b/src/main/scala/higherkindness/rules_scala/common/worker/WorkerMain.scala @@ -117,12 +117,18 @@ abstract class WorkerMain[S](stdin: InputStream = System.in, stdout: PrintStream // close them after the async work in the Future is all done. // If we do something synchronous with Using, then there's a race condition where the // streams can get closed before the Future is completed. - var outStream: ByteArrayOutputStream = null - var out: PrintStream = null + var maybeOutStream: Option[ByteArrayOutputStream] = None + var maybeOut: Option[PrintStream] = None + + def flushOut(): Unit = { + maybeOut.map(_.flush()) + } val workTask = CancellableTask { - outStream = new ByteArrayOutputStream - out = new PrintStream(outStream) + val outStream = new ByteArrayOutputStream() + val out = new PrintStream(outStream) + maybeOutStream = Some(outStream) + maybeOut = Some(out) try { work(ctx, args, out, sandboxDir, verbosity) 0 @@ -137,14 +143,15 @@ abstract class WorkerMain[S](stdin: InputStream = System.in, stdout: PrintStream .andThen { // Work task succeeded or failed in an expected way case Success(code) => - out.flush() - writeResponse(requestId, Some(outStream), Some(code)) + flushOut() + writeResponse(requestId, maybeOutStream, Some(code)) logVerbose(s"WorkResponse $requestId sent with code $code") case Failure(e: ExecutionException) => e.getCause() match { // Task successfully cancelled case cancelError: InterruptedException => + flushOut() writeResponse(requestId, None, None, wasCancelled = true) logVerbose( s"Cancellation WorkResponse sent for request id: $requestId in response to an" + @@ -152,9 +159,9 @@ abstract class WorkerMain[S](stdin: InputStream = System.in, stdout: PrintStream ) // Work task threw a non-fatal error case e => - e.printStackTrace(out) - out.flush() - writeResponse(requestId, Some(outStream), Some(-1)) + maybeOut.map(e.printStackTrace(_)) + flushOut() + writeResponse(requestId, maybeOutStream, Some(-1)) logVerbose( "Encountered an uncaught exception that was wrapped in an ExecutionException while" + s" proccessing the Future for WorkRequest $requestId. This usually means a non-fatal" + @@ -165,6 +172,7 @@ abstract class WorkerMain[S](stdin: InputStream = System.in, stdout: PrintStream // Task successfully cancelled case Failure(e: CancellationException) => + flushOut() writeResponse(requestId, None, None, wasCancelled = true) logVerbose( s"Cancellation WorkResponse sent for request id: $requestId in response to a" + @@ -173,15 +181,15 @@ abstract class WorkerMain[S](stdin: InputStream = System.in, stdout: PrintStream // Work task threw an uncaught exception case Failure(e) => - e.printStackTrace(out) - out.flush() - writeResponse(requestId, Some(outStream), Some(-1)) + maybeOut.map(e.printStackTrace(_)) + flushOut() + writeResponse(requestId, maybeOutStream, Some(-1)) logVerbose(s"Uncaught exception in Future while proccessing WorkRequest $requestId:") e.printStackTrace(System.err) }(scala.concurrent.ExecutionContext.global) .andThen { case _ => - out.close() - outStream.close() + maybeOut.map(_.close()) + maybeOutStream.map(_.close()) }(scala.concurrent.ExecutionContext.global) // putIfAbsent will return a non-null value if there was already a value in the map