Commit 0785c950 authored by brettw's avatar brettw Committed by Commit bot

GN: forward_variables_from shouldn't clobber vars.

The documentation for forward_variables_from specifies that it will give an error if the variable already exists in the target scope. But this was not implemented. Instead, the value would be silently overwritten.

This change implements the error, and fixes the times this happens in the Linux and Android builds.

Review-Url: https://codereview.chromium.org/1943583002
Cr-Commit-Position: refs/heads/master@{#391136}
parent a2f94c99
......@@ -15,5 +15,5 @@ android_library("bootstrap_java") {
"java/org/chromium/incrementalinstall/Reflect.java",
]
emma_never_instrument = true
run_findbugs = false
run_findbugs_override = false
}
......@@ -81,8 +81,6 @@ template("compiled_action") {
"compiled_action doesn't take a sources arg. Use inputs instead.")
action(target_name) {
deps = []
inputs = []
forward_variables_from(invoker,
[
"deps",
......@@ -91,6 +89,12 @@ template("compiled_action") {
"testonly",
"visibility",
])
if (!defined(deps)) {
deps = []
}
if (!defined(inputs)) {
inputs = []
}
script = "//build/gn_run_binary.py"
......@@ -124,8 +128,6 @@ template("compiled_action_foreach") {
assert(defined(invoker.args), "args must be defined for $target_name")
action_foreach(target_name) {
deps = []
inputs = []
forward_variables_from(invoker,
[
"deps",
......@@ -135,6 +137,12 @@ template("compiled_action_foreach") {
"testonly",
"visibility",
])
if (!defined(deps)) {
deps = []
}
if (!defined(inputs)) {
inputs = []
}
script = "//build/gn_run_binary.py"
......
......@@ -25,7 +25,6 @@ if (is_debug) {
template("android_lint") {
action(target_name) {
deps = []
forward_variables_from(invoker,
[
"deps",
......@@ -33,6 +32,9 @@ template("android_lint") {
"public_deps",
"testonly",
])
if (!defined(deps)) {
deps = []
}
_cache_dir = "$root_build_dir/android_lint_cache"
_result_path = "$target_gen_dir/$target_name/result.xml"
_config_path = "$target_gen_dir/$target_name/config.xml"
......@@ -386,13 +388,15 @@ template("write_build_config") {
type == "android_assets" || type == "resource_rewriter" ||
type == "java_binary" || type == "group")
deps = []
forward_variables_from(invoker,
[
"deps",
"testonly",
"visibility",
])
if (!defined(deps)) {
deps = []
}
script = "//build/android/gyp/write_build_config.py"
depfile = "$target_gen_dir/$target_name.d"
......@@ -1346,7 +1350,6 @@ template("java_prebuilt_impl") {
if (defined(invoker.main_class)) {
_binary_script_target_name = "${_template_name}__java_binary_script"
java_binary_script(_binary_script_target_name) {
deps = []
forward_variables_from(invoker,
[
"bootclasspath",
......@@ -1354,6 +1357,9 @@ template("java_prebuilt_impl") {
"main_class",
"wrapper_script_args",
])
if (!defined(deps)) {
deps = []
}
build_config = _build_config
jar_path = _jar_path
script_name = _template_name
......@@ -1436,9 +1442,12 @@ template("compile_java") {
_enable_errorprone = invoker.enable_errorprone
}
_enable_incremental_javac = enable_incremental_javac
if (defined(invoker.enable_incremental_javac)) {
_enable_incremental_javac = invoker.enable_incremental_javac
if (defined(invoker.enable_incremental_javac_override)) {
# Use invoker-specified override.
_enable_incremental_javac = invoker.enable_incremental_javac_override
} else {
# Default to build arg if not overridden.
_enable_incremental_javac = enable_incremental_javac
}
_manifest_entries = []
......@@ -1615,7 +1624,12 @@ template("java_library_impl") {
_android_manifest = invoker.android_manifest
}
assert(_android_manifest != "") # Mark as used.
_run_findbugs = defined(invoker.run_findbugs) && invoker.run_findbugs
if (defined(invoker.run_findbugs_override)) {
_run_findbugs = invoker.run_findbugs_override
} else {
_run_findbugs = run_findbugs # Default to build arg if not overridden.
}
assert(_run_findbugs || true) # Mark as used.
# Don't enable coverage, lint, findbugs unless the target has some
......@@ -1701,7 +1715,7 @@ template("java_library_impl") {
"alternative_android_sdk_ijar_dep",
"dist_jar_path",
"enable_errorprone",
"enable_incremental_javac",
"enable_incremental_javac_override",
"jar_excluded_patterns",
"manifest_entries",
"proguard_config",
......@@ -1788,12 +1802,14 @@ template("java_library_impl") {
}
group(target_name) {
data_deps = []
forward_variables_from(invoker,
[
"data_deps",
"visibility",
])
if (!defined(data_deps)) {
data_deps = []
}
public_deps = _final_deps
if (_has_lint_target) {
data_deps += [ ":${_template_name}__analysis" ]
......@@ -1952,8 +1968,6 @@ template("process_resources") {
template("copy_ex") {
set_sources_assignment_filter([])
action(target_name) {
inputs = []
sources = []
forward_variables_from(invoker,
[
"data",
......@@ -1963,6 +1977,9 @@ template("copy_ex") {
"testonly",
"visibility",
])
if (!defined(sources)) {
sources = []
}
script = "//build/android/gyp/copy_ex.py"
depfile = "$target_gen_dir/$target_name.d"
......@@ -2099,13 +2116,17 @@ template("test_runner_script") {
defined(invoker.incremental_install) && invoker.incremental_install
action(target_name) {
data_deps = []
deps = []
forward_variables_from(invoker,
[
"data_deps",
"deps",
])
if (!defined(deps)) {
deps = []
}
if (!defined(data_deps)) {
data_deps = []
}
script = "//build/android/gyp/create_test_runner_script.py"
depfile = "$target_gen_dir/$target_name.d"
......
......@@ -85,13 +85,15 @@ template("generate_jni") {
}
group(target_name) {
public_deps = []
forward_variables_from(invoker,
[
"deps",
"public_deps",
"visibility",
])
if (!defined(public_deps)) {
public_deps = []
}
public_deps += [ ":$foreach_target_name" ]
public_configs = [ ":jni_includes_${target_name}" ]
}
......@@ -637,7 +639,6 @@ template("android_resources") {
process_resources(process_resources_target_name) {
visibility = [ ":$final_target_name" ]
deps = []
forward_variables_from(invoker,
[
"app_as_shared_lib",
......@@ -650,6 +651,9 @@ template("android_resources") {
"shared_resources",
"v14_skip",
])
if (!defined(deps)) {
deps = []
}
deps += [ ":$build_config_target_name" ]
# Always generate R.onResourcesLoaded() method, it is required for
......@@ -908,7 +912,8 @@ template("java_strings_grd_prebuilt") {
# android_library target, for example.
# chromium_code: If true, extra analysis warning/errors will be enabled.
# enable_errorprone: If true, enables the errorprone compiler.
# enable_incremental_javac: Overrides the global enable_incremental_javac.
# enable_incremental_javac_override: Overrides the
# global enable_incremental_javac.
# main_class: When specified, a wrapper script is created within
# $root_build_dir/bin to launch the binary with the given class as the
# entrypoint.
......@@ -1007,7 +1012,8 @@ template("junit_binary") {
#
# chromium_code: If true, extra analysis warning/errors will be enabled.
# enable_errorprone: If true, enables the errorprone compiler.
# enable_incremental_javac: Overrides the global enable_incremental_javac.
# enable_incremental_javac_override: Overrides the global
# enable_incremental_javac.
#
# jar_excluded_patterns: List of patterns of .class files to exclude from the
# final jar.
......@@ -1096,7 +1102,8 @@ template("java_prebuilt") {
#
# chromium_code: If true, extra analysis warning/errors will be enabled.
# enable_errorprone: If true, enables the errorprone compiler.
# enable_incremental_javac: Overrides the global enable_incremental_javac.
# enable_incremental_javac_override: Overrides the global
# enable_incremental_javac.
#
# jar_excluded_patterns: List of patterns of .class files to exclude from the
# final jar.
......@@ -1269,6 +1276,8 @@ template("android_java_prebuilt") {
# installation only on Android M or later. In these releases the system
# linker does relocation unpacking, so we can enable it unconditionally.
# secondary_native_libs: the path of native libraries for secondary app abi.
# run_findbugs_override: Forces run_findbugs on or off. If undefined, the
# default will use the build arg run_findbugs.
#
# Example
# android_apk("foo_apk") {
......@@ -1556,7 +1565,7 @@ template("android_apk") {
[
"chromium_code",
"java_files",
"run_findbugs",
"run_findbugs_override",
])
supports_android = true
requires_android = true
......@@ -1659,7 +1668,6 @@ template("android_apk") {
}
dex("$final_dex_target_name") {
forward_variables_from(invoker, [ "enable_multidex" ])
deps = _dex_deps + [ ":$build_config_target" ]
inputs = [
_build_config,
......@@ -1750,7 +1758,6 @@ template("android_apk") {
_final_deps += [ ":${_template_name}__create" ]
create_apk("${_template_name}__create") {
deps = []
forward_variables_from(invoker,
[
"alternative_android_sdk_jar",
......@@ -1766,6 +1773,9 @@ template("android_apk") {
"uncompress_shared_libraries",
"write_asset_list",
])
if (!defined(deps)) {
deps = []
}
apk_path = _final_apk_path
android_manifest = _android_manifest
assets_build_config = _build_config
......@@ -1929,12 +1939,14 @@ template("android_apk") {
public_deps = _final_deps
}
group("${target_name}_incremental") {
data_deps = []
forward_variables_from(invoker,
[
"data",
"data_deps",
])
if (!defined(data_deps)) {
data_deps = []
}
# device/commands is used by the installer script to push files via .zip.
data_deps += [ "//build/android/pylib/device/commands" ] +
......@@ -2037,8 +2049,11 @@ template("instrumentation_test_apk") {
}
create_dist_ijar = true
run_findbugs = defined(invoker.run_findbugs) && invoker.run_findbugs &&
defined(invoker.java_files)
if (defined(invoker.run_findbugs_override)) {
# Only allow findbugs when there are java files.
run_findbugs_override =
invoker.run_findbugs_override && defined(invoker.java_files)
}
}
group(target_name) {
......
......@@ -21,8 +21,6 @@ template("generate_nmf") {
assert(defined(invoker.nmf), "Must define nmf")
action(target_name) {
nmfflags = []
forward_variables_from(invoker,
[
"deps",
......@@ -36,6 +34,9 @@ template("generate_nmf") {
"testonly",
"visibility",
])
if (!defined(nmfflags)) {
nmfflags = []
}
# TODO(phosek): Remove this conditional once
# https://bugs.chromium.org/p/nativeclient/issues/detail?id=4339 is
......@@ -119,15 +120,12 @@ template("generate_nonsfi_test_nmf") {
assert(defined(invoker.nmf), "Must define nmf")
action(target_name) {
nmfflags = []
forward_variables_from(invoker,
[
"deps",
"data_deps",
"executable",
"nmf",
"nmfflags",
"testonly",
"public_deps",
"visibility",
......@@ -152,9 +150,12 @@ template("generate_nonsfi_test_nmf") {
arch = target_cpu
}
args = [
"--program=" + rebase_path(executable, root_build_dir),
"--arch=${arch}",
"--output=" + rebase_path(nmf, root_build_dir),
] + nmfflags
"--program=" + rebase_path(executable, root_build_dir),
"--arch=${arch}",
"--output=" + rebase_path(nmf, root_build_dir),
]
if (defined(invoker.nmfflags)) {
args += invoker.nmfflags
}
}
}
......@@ -412,14 +412,21 @@ template("gcc_toolchain") {
forward_variables_from(invoker,
[
"is_clang",
"is_component_build",
"is_nacl_glibc",
"use_allocator",
"use_gold",
"symbol_level",
])
if (defined(invoker.is_clang)) {
is_clang = invoker.is_clang
}
if (defined(invoker.is_component_build)) {
is_component_build = invoker.is_component_build
}
if (defined(invoker.is_nacl_glibc)) {
is_nacl_glibc = invoker.is_nacl_glibc
}
if (defined(invoker.clear_sanitizers) && invoker.clear_sanitizers) {
is_asan = false
is_cfi = false
......
......@@ -38,8 +38,6 @@ template("nacl_toolchain") {
"cc",
"cxx",
"deps",
"is_clang",
"is_nacl_glibc",
"ld",
"link_outputs",
"nm",
......@@ -49,6 +47,13 @@ template("nacl_toolchain") {
"toolchain_cpu",
])
if (defined(invoker.is_clang)) {
is_clang = invoker.is_clang
}
if (defined(invoker.is_nacl_glibc)) {
is_nacl_glibc = invoker.is_nacl_glibc
}
# We do not support component builds or sanitizers with the NaCl toolchains.
is_component_build = false
clear_sanitizers = true
......
......@@ -267,7 +267,7 @@ android_library("cronet_api") {
":network_quality_observations_java",
]
run_findbugs = true
run_findbugs_override = true
}
android_library("cronet_java") {
......@@ -304,7 +304,7 @@ android_library("cronet_java") {
":net_request_priority_java",
]
run_findbugs = true
run_findbugs_override = true
}
android_resources("cronet_sample_apk_resources") {
......@@ -325,7 +325,7 @@ android_library("cronet_sample_apk_java") {
"//base:base_java",
]
run_findbugs = true
run_findbugs_override = true
}
android_apk("cronet_sample_apk") {
......@@ -341,7 +341,7 @@ android_apk("cronet_sample_apk") {
"//third_party/jsr-305:jsr_305_javalib",
]
run_findbugs = true
run_findbugs_override = true
if (!is_debug) {
proguard_enabled = true
proguard_configs = [
......@@ -380,7 +380,7 @@ instrumentation_test_apk("cronet_sample_test_apk") {
]
additional_apks = [ "//net/android:net_test_support_apk" ]
run_findbugs = true
run_findbugs_override = true
proguard_enabled = !is_debug
}
......@@ -485,7 +485,7 @@ android_library("cronet_test_apk_java") {
"//third_party/netty4:netty_all",
]
run_findbugs = true
run_findbugs_override = true
}
android_assets("cronet_test_apk_assets") {
......@@ -544,7 +544,7 @@ android_apk("cronet_test_apk") {
"//third_party/netty-tcnative:netty-tcnative_all",
]
run_findbugs = true
run_findbugs_override = true
}
android_library("cronet_javatests") {
......@@ -597,7 +597,7 @@ android_library("cronet_javatests") {
"//net/android:net_java_test_support",
]
run_findbugs = true
run_findbugs_override = true
}
instrumentation_test_apk("cronet_test_instrumentation_apk") {
......@@ -627,7 +627,7 @@ instrumentation_test_apk("cronet_test_instrumentation_apk") {
# TODO(jbudorick): Remove this once GN uses the generated .isolate files.
isolate_file = "cronet_test_instrumentation_apk.isolate"
run_findbugs = true
run_findbugs_override = true
}
android_library("cronet_perf_test_apk_java") {
......@@ -643,7 +643,7 @@ android_library("cronet_perf_test_apk_java") {
"//base:base_java",
]
run_findbugs = true
run_findbugs_override = true
}
android_apk("cronet_perf_test_apk") {
......@@ -659,7 +659,7 @@ android_apk("cronet_perf_test_apk") {
"//base:base_java",
]
run_findbugs = true
run_findbugs_override = true
proguard_enabled = true
proguard_configs = [
"proguard.cfg",
......
......@@ -388,7 +388,7 @@ template("mojom") {
}
srcjar_deps = [ ":$java_srcjar_target_name" ]
run_findbugs = false
run_findbugs_override = false
}
}
}
......
......@@ -97,7 +97,6 @@ template("test") {
_device_isolate_path = "$root_out_dir/gen.runtime/$_target_dir_name/$target_name.device.isolate"
_gen_isolate_target_name = "${target_name}__isolate"
_gen_isolate(_gen_isolate_target_name) {
data_deps = []
forward_variables_from(invoker,
[
"data",
......
......@@ -9,5 +9,5 @@ android_library("gif_player_java") {
"src/jp/tomorrowkey/android/gifplayer/BaseGifDrawable.java",
"src/jp/tomorrowkey/android/gifplayer/BaseGifImage.java",
]
run_findbugs = false
run_findbugs_override = false
}
......@@ -27,5 +27,5 @@ java_binary("jmake") {
"src/org/pantsbuild/jmake/BinaryProjectDatabaseReader.java",
]
main_class = "org.pantsbuild.jmake.Main"
enable_incremental_javac = false
enable_incremental_javac_override = false
}
......@@ -98,7 +98,7 @@ android_library("netty-tcnative") {
"src/java/src/org/apache/tomcat/jni/Time.java",
"src/java/src/org/apache/tomcat/jni/User.java",
]
run_findbugs = false
run_findbugs_override = false
deps = [
"//base:base_java",
]
......
......@@ -54,6 +54,18 @@ void ForwardValuesFromList(Scope* source,
return;
}
// Don't allow clobbering existing values.
const Value* existing_value = dest->GetValue(storage_key);
if (existing_value) {
*err = Err(cur, "Clobbering existing value.",
"The current scope already defines a value \"" +
cur.string_value() + "\".\nforward_variables_from() won't clobber "
"existing values. If you want to\nmerge lists, you'll need to "
"do this explicitly.");
err->AppendSubErr(Err(*existing_value, "value being clobbered."));
return;
}
// Keep the origin information from the original value. The normal
// usage is for this to be used in a template, and if there's an error,
// the user expects to see the line where they set the variable
......
......@@ -8,28 +8,46 @@
TEST(FunctionForwardVariablesFrom, List) {
Scheduler scheduler;
TestWithScope setup;
// Defines a template and copy the two x and y, and z values out.
TestParseInput input(
"template(\"a\") {\n"
" forward_variables_from(invoker, [\"x\", \"y\", \"z\"])\n"
" assert(!defined(z))\n" // "z" should still be undefined.
" print(\"$target_name, $x, $y\")\n"
"}\n"
"a(\"target\") {\n"
" x = 1\n"
" y = 2\n"
"}\n");
ASSERT_FALSE(input.has_error());
Err err;
input.parsed()->Execute(setup.scope(), &err);
ASSERT_FALSE(err.has_error()) << err.message();
EXPECT_EQ("target, 1, 2\n", setup.print_output());
setup.print_output().clear();
std::string program =
"template(\"a\") {\n"
" forward_variables_from(invoker, [\"x\", \"y\", \"z\"])\n"
" assert(!defined(z))\n" // "z" should still be undefined.
" print(\"$target_name, $x, $y\")\n"
"}\n"
"a(\"target\") {\n"
" x = 1\n"
" y = 2\n"
"}\n";
{
TestWithScope setup;
// Defines a template and copy the two x and y, and z values out.
TestParseInput input(program);
ASSERT_FALSE(input.has_error());
input.parsed()->Execute(setup.scope(), &err);
ASSERT_FALSE(err.has_error()) << err.message();
EXPECT_EQ("target, 1, 2\n", setup.print_output());
setup.print_output().clear();
}
{
TestWithScope setup;
// Test that the same input but forwarding a variable with the name of
// something in the given scope throws an error rather than clobbering it.
// This uses the same known-good program as before, but adds another
// variable in the scope before it.
TestParseInput clobber("x = 1\n" + program);
ASSERT_FALSE(clobber.has_error());
clobber.parsed()->Execute(setup.scope(), &err);
ASSERT_TRUE(err.has_error()); // Should thow a clobber error.
EXPECT_EQ("Clobbering existing value.", err.message());
}
}
TEST(FunctionForwardVariablesFrom, ListWithExclusion) {
......