diff --git a/.gemini/config.yaml b/.gemini/config.yaml new file mode 100644 index 000000000..2506a30c7 --- /dev/null +++ b/.gemini/config.yaml @@ -0,0 +1,2 @@ +code_review: + disable: true diff --git a/scala/private/phases/phase_runenvironmentinfo_provider.bzl b/scala/private/phases/phase_runenvironmentinfo_provider.bzl new file mode 100644 index 000000000..99286650f --- /dev/null +++ b/scala/private/phases/phase_runenvironmentinfo_provider.bzl @@ -0,0 +1,82 @@ +"""Phase implementing variable expansion for the env attribute""" + +def _expand_part(ctx, attr_name, part, targets, additional_vars): + """Perform `$(location)` and "Make variable" substitution for `expand_vars`. + + As for why we're using the "deprecated" `ctx.expand_make_variables`: + + - https://github.com/bazelbuild/bazel/issues/5859 + - https://github.com/bazelbuild/bazel-skylib/pull/486 + + The "deprecated" comment in the `ctx.expand_make_variables` docstring has + existed from the beginning of the file's existence (2018-05-11): + + - https://github.com/bazelbuild/bazel/commit/abbb9002c41bbd53588e7249756aab236f6fcb4b + """ + expanded = ctx.expand_location(part, targets) + return ctx.expand_make_variables(attr_name, expanded, additional_vars) + +def expand_vars(ctx, attr_name, value, targets, additional_vars): + """Perform `$(location)` and "Make variable" substitution on an attribute. + + - https://bazel.build/reference/be/make-variables#use + + Args: + ctx: Rule context object + attr_name: name of the attribute (for error messages) + value: the attribute value + targets: a list of `Target` values to use with `$(location)` expansion + additional_vars: additional values to use with "Make variable" expansion + + Returns: + the result of performing `$(location)` and "Make variable" substitution + on the specified attribute value + """ + # Splitting on `$$` ensures that escaped `$` values prevent `$(location)` + # and "Make variable" substitution for those portions of `value`. + return "$".join([ + _expand_part(ctx, attr_name, s, targets, additional_vars) + for s in value.split("$$") + ]) + +def run_environment_info(ctx, additional_attrs = []): + """Create a RunEnvironmentInfo provider from `ctx.attr.env` values. + + Implements the "values are subject to `$(location)` and "Make variable" + substitution" contract for the common `env` attribute for binary and test + rules: + + - https://bazel.build/reference/be/common-definitions#common-attributes-binaries + - https://bazel.build/reference/be/common-definitions#common-attributes-tests + - https://bazel.build/reference/be/make-variables#use + - https://bazel.build/rules/lib/providers/RunEnvironmentInfo + + Assigns `ctx.attr.env_inherit` to `RunEnvironmentInfo.inherited_environment` + if present. + + Args: + ctx: Rule context object + additional_attrs: `attr.label_list` values containing targets to use + when invoking `ctx.expand_location` in addition to "data", "deps", + and "srcs" + + Returns: + a RunEnvironmentInfo object containing `ctx.attr.env` values after + expanding location and Make variables + """ + targets = getattr(ctx.attr, "data", []) + for additional_attr in additional_attrs: + targets.extend(additional_attr) + + return RunEnvironmentInfo( + environment = { + k: expand_vars(ctx, "env", v, targets, ctx.var) + for k, v in ctx.attr.env.items() + }, + inherited_environment = getattr(ctx.attr, "env_inherit", []), + ) + +def phase_runenvironmentinfo_provider(ctx, _): + return struct( + external_providers = {"RunEnvironmentInfo": run_environment_info(ctx)}, + ) diff --git a/scala/private/phases/phase_test_environment.bzl b/scala/private/phases/phase_test_environment.bzl deleted file mode 100644 index cdc20f66a..000000000 --- a/scala/private/phases/phase_test_environment.bzl +++ /dev/null @@ -1,38 +0,0 @@ -# Adds testing.TestEnvironment provider if "env" attr is specified -# https://bazel.build/rules/lib/testing#TestEnvironment - -def phase_test_environment(ctx, p): - test_env = ctx.attr.env - inherited_environment = ctx.attr.env_inherit - - if inherited_environment and test_env: - return struct( - external_providers = { - "TestingEnvironment": testing.TestEnvironment( - test_env, - inherited_environment, - ), - }, - ) - - elif test_env: - return struct( - external_providers = { - "TestingEnvironment": testing.TestEnvironment( - test_env, - ), - }, - ) - - elif inherited_environment: - return struct( - external_providers = { - "TestingEnvironment": testing.TestEnvironment( - {}, - inherited_environment, - ), - }, - ) - - else: - return struct() diff --git a/scala/private/phases/phases.bzl b/scala/private/phases/phases.bzl index d4f1062dd..da292e88c 100644 --- a/scala/private/phases/phases.bzl +++ b/scala/private/phases/phases.bzl @@ -62,7 +62,10 @@ load( _phase_scalainfo_provider_non_macro = "phase_scalainfo_provider_non_macro", ) load("//scala/private:phases/phase_semanticdb.bzl", _phase_semanticdb = "phase_semanticdb") -load("//scala/private:phases/phase_test_environment.bzl", _phase_test_environment = "phase_test_environment") +load( + "//scala/private:phases/phase_runenvironmentinfo_provider.bzl", + _phase_runenvironmentinfo_provider = "phase_runenvironmentinfo_provider", +) load( "//scala/private:phases/phase_write_executable.bzl", _phase_write_executable_common = "phase_write_executable_common", @@ -149,8 +152,8 @@ phase_runfiles_common = _phase_runfiles_common # default_info phase_default_info = _phase_default_info -# test_environment -phase_test_environment = _phase_test_environment +# runenvironmentinfo_provider +phase_runenvironmentinfo_provider = _phase_runenvironmentinfo_provider # scalafmt phase_scalafmt = _phase_scalafmt diff --git a/scala/private/rules/scala_binary.bzl b/scala/private/rules/scala_binary.bzl index 1a364ddad..5b901fb4a 100644 --- a/scala/private/rules/scala_binary.bzl +++ b/scala/private/rules/scala_binary.bzl @@ -23,6 +23,7 @@ load( "phase_dependency_common", "phase_java_wrapper_common", "phase_merge_jars", + "phase_runenvironmentinfo_provider", "phase_runfiles_common", "phase_scalac_provider", "phase_scalacopts", @@ -54,12 +55,14 @@ def _scala_binary_impl(ctx): ("runfiles", phase_runfiles_common), ("write_executable", phase_write_executable_common), ("default_info", phase_default_info), + ("runenvironmentinfo_provider", phase_runenvironmentinfo_provider), ], ) _scala_binary_attrs = { "main_class": attr.string(mandatory = True), "classpath_resources": attr.label_list(allow_files = True), + "env": attr.string_dict(default = {}), "jvm_flags": attr.string_list(), "runtime_jdk": attr.label( default = "@rules_java//toolchains:current_java_runtime", diff --git a/scala/private/rules/scala_junit_test.bzl b/scala/private/rules/scala_junit_test.bzl index a6ba7ad17..59cbfeef3 100644 --- a/scala/private/rules/scala_junit_test.bzl +++ b/scala/private/rules/scala_junit_test.bzl @@ -24,12 +24,12 @@ load( "phase_java_wrapper_common", "phase_jvm_flags", "phase_merge_jars", + "phase_runenvironmentinfo_provider", "phase_runfiles_common", "phase_scalac_provider", "phase_scalacopts", "phase_scalainfo_provider_non_macro", "phase_semanticdb", - "phase_test_environment", "phase_write_executable_junit_test", "phase_write_manifest", "run_phases", @@ -62,7 +62,7 @@ def _scala_junit_test_impl(ctx): ("jvm_flags", phase_jvm_flags), ("write_executable", phase_write_executable_junit_test), ("default_info", phase_default_info), - ("test_environment", phase_test_environment), + ("runenvironmentinfo_provider", phase_runenvironmentinfo_provider), ], ) diff --git a/scala/private/rules/scala_test.bzl b/scala/private/rules/scala_test.bzl index 227db14eb..6a5c19048 100644 --- a/scala/private/rules/scala_test.bzl +++ b/scala/private/rules/scala_test.bzl @@ -23,12 +23,12 @@ load( "phase_dependency_common", "phase_java_wrapper_common", "phase_merge_jars", + "phase_runenvironmentinfo_provider", "phase_runfiles_scalatest", "phase_scalac_provider", "phase_scalacopts", "phase_scalainfo_provider_non_macro", "phase_semanticdb", - "phase_test_environment", "phase_write_executable_scalatest", "phase_write_manifest", "run_phases", @@ -56,7 +56,7 @@ def _scala_test_impl(ctx): ("coverage_runfiles", phase_coverage_runfiles), ("write_executable", phase_write_executable_scalatest), ("default_info", phase_default_info), - ("test_environment", phase_test_environment), + ("runenvironmentinfo_provider", phase_runenvironmentinfo_provider), ], ) diff --git a/test/BUILD b/test/BUILD index 23c7c6ce0..bf313505b 100644 --- a/test/BUILD +++ b/test/BUILD @@ -15,6 +15,7 @@ load( ) load("//scala:scala_cross_version.bzl", "repositories") load(":check_statsfile.bzl", "check_statsfile") +load(":env_vars.bzl", "env_vars") package( default_testonly = 1, @@ -751,3 +752,77 @@ scala_library( ], deps = [":InlinableExported"], ) + +TEST_ENV = { + "LOCATION": "West of House", + "DEP_PATH": "$(rootpath :HelloLib)", + "DATA_PATH": "$(rootpath //test/data:foo.txt)", + "BINDIR": "$(BINDIR)", + "FROM_TOOLCHAIN_VAR": "$(FOOBAR)", + "ESCAPED": "$$(rootpath //test/data:foo.txt) $$(BINDIR) $$UNKNOWN", +} + +scala_binary( + name = "EnvAttributeBinary", + srcs = ["EnvAttributeBinary.scala"], + main_class = "scalarules.test.EnvAttributeBinary", + data = ["//test/data:foo.txt"], + deps = [":HelloLib"], + env = TEST_ENV | {"SRC_PATH": "$(rootpath EnvAttributeBinary.scala)"}, + toolchains = [":test_vars"], + unused_dependency_checker_mode = "off", + testonly = True, +) + +scala_test( + name = "EnvAttributeTest", + size = "small", + srcs = ["EnvAttributeTest.scala"], + data = ["//test/data:foo.txt"], + deps = [":HelloLib"], + env = TEST_ENV | {"SRC_PATH": "$(rootpath EnvAttributeTest.scala)"}, + toolchains = [":test_vars"], + unused_dependency_checker_mode = "off", +) + +scala_junit_test( + name = "EnvAttributeJunitTest", + size = "small", + srcs = ["src/main/scala/scalarules/test/junit/EnvAttributeJunitTest.scala"], + suffixes = ["Test"], + data = ["//test/data:foo.txt"], + deps = [":HelloLib"], + env = TEST_ENV | { + "SRC_PATH": ( + "$(rootpath " + + "src/main/scala/scalarules/test/junit/EnvAttributeJunitTest.scala)" + ), + }, + toolchains = [":test_vars"], + unused_dependency_checker_mode = "off", +) + +env_vars( + name = "test_vars", + vars = {"FOOBAR": "bazquux"}, +) + +# Used by test_scala_test_env_attribute_with_env_inherit_and_test_env +# from test/shell/test_env_attribute_expansion.sh. +scala_test( + name = "EnvAttributeManualTest", + size = "small", + srcs = ["EnvAttributeManualTest.scala"], + env = { + "FROM_TEST_ENV_AND_ENV_ATTR": "env attribute value", + "FROM_ENV_INHERIT_AND_ENV_ATTR": "env attribute value", + "FROM_ALL_SOURCES": "env attribute value", + }, + env_inherit = [ + "FROM_ENV_INHERIT", + "FROM_ENV_INHERIT_AND_ENV_ATTR", + "FROM_ENV_INHERIT_AND_TEST_ENV", + "FROM_ALL_SOURCES", + ], + tags = ["manual"], +) diff --git a/test/EnvAttributeBinary.scala b/test/EnvAttributeBinary.scala new file mode 100644 index 000000000..1e2c17408 --- /dev/null +++ b/test/EnvAttributeBinary.scala @@ -0,0 +1,20 @@ +package scalarules.test + +object EnvAttributeBinary { + def main(args: Array[String]) { + val envVars = Array( + "LOCATION", + "DATA_PATH", + "DEP_PATH", + "SRC_PATH", + "BINDIR", + "FROM_TOOLCHAIN_VAR", + "ESCAPED", + ) + val env = System.getenv() + + for (envVar <- envVars) { + println(envVar + ": " + env.get(envVar)) + } + } +} diff --git a/test/EnvAttributeManualTest.scala b/test/EnvAttributeManualTest.scala new file mode 100644 index 000000000..6b4d863c6 --- /dev/null +++ b/test/EnvAttributeManualTest.scala @@ -0,0 +1,41 @@ +// Used by test_scala_test_env_attribute_with_env_inherit_and_test_env +// from test/shell/test_env_attribute_expansion.sh. + +package scalarules.test + +import org.scalatest.flatspec._ + +// Validates that the `run_environment_info` implementation from +// //scala/private:phases/phase_runenvironmentinfo_provider.bzl conforms to: +// - https://bazel.build/rules/lib/providers/RunEnvironmentInfo +class EnvAttributeManualTest extends AnyFlatSpec { + val env = System.getenv() + + "--test_env variables not in env" should "pass through" in { + assert(env.get("FROM_TEST_ENV") == "test env value") + } + + "env_inherit variables not in env" should "pass through" in { + assert(env.get("FROM_ENV_INHERIT") == "inherited value") + } + + "variables not in env_inherit" should "not pass through" in { + assert(env.get("NOT_IN_ENV_INHERIT") == null) + } + + "env" should "override --test_env" in { + assert(env.get("FROM_TEST_ENV_AND_ENV_ATTR") == "env attribute value") + } + + "env_inherit" should "override env" in { + assert(env.get("FROM_ENV_INHERIT_AND_ENV_ATTR") == "inherited value") + } + + "env_inherit" should "override --test_env" in { + assert(env.get("FROM_ENV_INHERIT_AND_TEST_ENV") == "inherited value") + } + + "env_inherit" should "override --test_env and env when all specified" in { + assert(env.get("FROM_ALL_SOURCES") == "inherited value") + } +} diff --git a/test/EnvAttributeTest.scala b/test/EnvAttributeTest.scala new file mode 100644 index 000000000..613416c71 --- /dev/null +++ b/test/EnvAttributeTest.scala @@ -0,0 +1,32 @@ +package scalarules.test + +import org.scalatest.flatspec._ + +class EnvAttributeTest extends AnyFlatSpec { + val env = System.getenv() + + "the env attribute" should "contain a plain value" in { + assert(env.get("LOCATION") == "West of House") + } + + "the env attribute" should "expand location variables" in { + assert(env.get("DATA_PATH") == "test/data/foo.txt", "in DATA_PATH") + assert(env.get("DEP_PATH") == "test/HelloLib.jar", "in DEP_PATH") + assert(env.get("SRC_PATH") == "test/EnvAttributeTest.scala", "in SRC_PATH") + } + + "the env attribute" should "expand Make variables" in { + assert(env.get("BINDIR").startsWith("bazel-out/")) + } + + "the env attribute" should "expand toolchain supplied variables" in { + assert(env.get("FROM_TOOLCHAIN_VAR") == "bazquux") + } + + "the env attribute" should "not expand escaped variables" in { + assert( + env.get("ESCAPED") == + "$(rootpath //test/data:foo.txt) $(BINDIR) $UNKNOWN" + ) + } +} diff --git a/test/env_vars.bzl b/test/env_vars.bzl new file mode 100644 index 000000000..270ab06c0 --- /dev/null +++ b/test/env_vars.bzl @@ -0,0 +1,8 @@ +"""Implements a custom environment variable map""" + +env_vars = rule( + implementation = lambda ctx: [ + platform_common.TemplateVariableInfo(ctx.attr.vars), + ], + attrs = {"vars": attr.string_dict(mandatory = True)}, +) diff --git a/test/shell/test_env_attribute_expansion.sh b/test/shell/test_env_attribute_expansion.sh new file mode 100755 index 000000000..efb759ab7 --- /dev/null +++ b/test/shell/test_env_attribute_expansion.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# +# Tests that `scala_binary` properly expands `env` attribute values. +# See: scala/private/phases/phase_expand_environment.bzl + +set -euo pipefail + +dir="$( cd "${BASH_SOURCE[0]%/*}" && echo "${PWD%/test/shell}" )" +test_source="${dir}/test/shell/${BASH_SOURCE[0]#*test/shell/}" +# shellcheck source=./test_runner.sh +. "${dir}"/test/shell/test_runner.sh + +setup_suite() { + original_dir="$PWD" + setup_test_tmpdir_for_file "$original_dir" "$test_source" + test_tmpdir="$PWD" + cd "$original_dir" + + bazel_bin='' + if ! bazel_bin="$(command -v bazel)" && + ! bazel_bin="$(command -v bazel.exe)"; then + printf 'bazel executable not found in: %s\n' "$PATH" >&2 + exit 1 + fi +} + +teardown_suite() { + rm -rf "$test_tmpdir" +} + +test_scala_binary_env_attribute_expansion() { + local bindir="$("$bazel_bin" info bazel-bin)" + bindir="bazel-out/${bindir#*/bazel-out/}" + + "$bazel_bin" run //test:EnvAttributeBinary > "${test_tmpdir}/actual.txt" + + printf '%s\n' \ + 'LOCATION: West of House' \ + 'DATA_PATH: test/data/foo.txt' \ + 'DEP_PATH: test/HelloLib.jar' \ + 'SRC_PATH: test/EnvAttributeBinary.scala' \ + "BINDIR: ${bindir}" \ + 'FROM_TOOLCHAIN_VAR: bazquux' \ + 'ESCAPED: $(rootpath //test/data:foo.txt) $(BINDIR) $UNKNOWN' \ + > "${test_tmpdir}/expected.txt" + + diff -u --strip-trailing-cr "${test_tmpdir}"/{expected,actual}.txt +} + +# If Bazel is actually bazel.exe, then Bash _will not_ export its environment +# variables to the Bazel process. In this case, we _must_ export them as +# Command Prompt variables. +_bazel_bat() { + local bazel_bat="${test_tmpdir}/bazel.bat" + + printf '%s\n' \ + 'set FROM_ENV_INHERIT=inherited value' \ + 'set NOT_IN_ENV_INHERIT=inherited value' \ + 'set FROM_ENV_INHERIT_AND_ENV_ATTR=inherited value' \ + 'set FROM_ENV_INHERIT_AND_TEST_ENV=inherited value' \ + 'set FROM_ALL_SOURCES=inherited value' \ + "bazel.exe %*" \ + > "$bazel_bat" + bazel_bat="${bazel_bat#$PWD/}" + cmd.exe '/c' "${bazel_bat//\//\\}" "$@" +} + +_bazel_bash() { + FROM_ENV_INHERIT="inherited value" \ + NOT_IN_ENV_INHERIT="inherited value" \ + FROM_ENV_INHERIT_AND_ENV_ATTR="inherited value" \ + FROM_ENV_INHERIT_AND_TEST_ENV="inherited value" \ + FROM_ALL_SOURCES="inherited value" \ + bazel "$@" +} + +test_scala_test_env_attribute_with_env_inherit_and_test_env() { + local bazel='_bazel_bash' + + if [[ "${bazel_bin##*.}" == 'exe' ]]; then + bazel='_bazel_bat' + fi + + "$bazel" test \ + --test_env=FROM_TEST_ENV="test env value" \ + --test_env=FROM_TEST_ENV_AND_ENV_ATTR="test env value" \ + --test_env=FROM_ENV_INHERIT_AND_TEST_ENV="test env value" \ + --test_env=FROM_ALL_SOURCES="test env value" \ + --test_output=all \ + //test:EnvAttributeManualTest +} + +setup_suite +run_tests "$test_source" "$(get_test_runner "${1:-local}")" +teardown_suite diff --git a/test/src/main/scala/scalarules/test/junit/EnvAttributeJunitTest.scala b/test/src/main/scala/scalarules/test/junit/EnvAttributeJunitTest.scala new file mode 100644 index 000000000..e9e083954 --- /dev/null +++ b/test/src/main/scala/scalarules/test/junit/EnvAttributeJunitTest.scala @@ -0,0 +1,47 @@ +package scalarules.test.junit + +import org.junit.Assert.assertEquals +import org.junit.Assert.fail +import org.junit.Test + +class EnvAttributeJunitTest { + val env = System.getenv() + + @Test + def plainValueRemainsUnchanged: Unit = { + assertEquals("West of House", env.get("LOCATION")) + } + + @Test + def expandsLocationVariables: Unit = { + assertEquals("in DATA_PATH", "test/data/foo.txt", env.get("DATA_PATH")) + assertEquals("in DEP_PATH", "test/HelloLib.jar", env.get("DEP_PATH")) + assertEquals( + "in SRC_PATH", + "test/src/main/scala/scalarules/test/junit/EnvAttributeJunitTest.scala", + env.get("SRC_PATH"), + ) + } + + @Test + def expandsMakeVariables: Unit = { + val bindir = env.get("BINDIR") + + if (! bindir.startsWith("bazel-out/")) { + fail("BINDIR does not start with bazel-out/: " + bindir) + } + } + + @Test + def expandsToolchainSuppliedVariables: Unit = { + assertEquals("bazquux", env.get("FROM_TOOLCHAIN_VAR")) + } + + @Test + def doesNotExpandEscapedVariables: Unit = { + assertEquals( + "$(rootpath //test/data:foo.txt) $(BINDIR) $UNKNOWN", + env.get("ESCAPED"), + ) + } +} diff --git a/test_rules_scala.sh b/test_rules_scala.sh index 7e856841e..5674085c4 100755 --- a/test_rules_scala.sh +++ b/test_rules_scala.sh @@ -29,6 +29,7 @@ $runner bazel test "$test_output_flag" //test/... --extra_toolchains="//test_exp $runner bazel build //test/sh_tests:ScalaBinaryInGenrule --nolegacy_external_runfiles $runner bazel build //test_statsfile:Simple_statsfile $runner bazel build //test_statsfile:SimpleNoStatsFile_statsfile --extra_toolchains="//test/toolchains:enable_stats_file_disabled_toolchain" +. "${test_dir}"/test_env_attribute_expansion.sh . "${test_dir}"/test_compiler_sources_integrity.sh . "${test_dir}"/test_build_event_protocol.sh . "${test_dir}"/test_compilation.sh