Skip to content
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

extension: use bool_flag to control extension enablement #14094

Closed
wants to merge 2 commits into from
Closed

extension: use bool_flag to control extension enablement #14094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18622cb
extension: use bool_flag to control extension enablement
lizan Nov 19, 2020
104288a
fix
lizan Nov 19, 2020
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
2 changes: 1 addition & 1 deletion .bazelci/presubmit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ tasks:
test_flags:
- "--config=remote-clang-libc++"
- "--config=remote-ci"
- "--define=wasm=disabled"
- "--//source/extensions/wasm_runtime/v8:enabled=false"
- "--jobs=75"
coverage:
name: "Coverage"
Expand Down
18 changes: 1 addition & 17 deletions bazel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,25 +338,9 @@ config_setting(
values = {"define": "quiche=enabled"},
)

# TODO: consider converting WAVM VM support to an extension (https://github.com/envoyproxy/envoy/issues/12574)
config_setting(
name = "wasm_wavm",
values = {"define": "wasm=wavm"},
)

config_setting(
name = "wasm_v8",
values = {"define": "wasm=v8"},
)

config_setting(
name = "wasm_wasmtime",
values = {"define": "wasm=wasmtime"},
)

config_setting(
name = "wasm_none",
values = {"define": "wasm=disabled"},
values = {"define": "wasm_tests=disabled"},
)

# Alias pointing to the selected version of BoringSSL:
Expand Down
19 changes: 12 additions & 7 deletions bazel/envoy_build_system.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ load(
_envoy_select_hot_restart = "envoy_select_hot_restart",
_envoy_select_new_codecs_in_integration_tests = "envoy_select_new_codecs_in_integration_tests",
_envoy_select_wasm = "envoy_select_wasm",
_envoy_select_wasm_v8 = "envoy_select_wasm_v8",
_envoy_select_wasm_wasmtime = "envoy_select_wasm_wasmtime",
_envoy_select_wasm_wavm = "envoy_select_wasm_wavm",
)
load(
":envoy_test.bzl",
Expand All @@ -40,13 +37,24 @@ load(
"@envoy_build_config//:extensions_build_config.bzl",
"EXTENSION_PACKAGE_VISIBILITY",
)
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")

def envoy_package():
native.package(default_visibility = ["//visibility:public"])

def envoy_extension_package():
def envoy_extension_package(enabled_default = True):
native.package(default_visibility = EXTENSION_PACKAGE_VISIBILITY)

bool_flag(
name = "enabled",
build_setting_default = enabled_default,
)

native.config_setting(
name = "is_enabled",
flag_values = {":enabled": "True"},
)

# A genrule variant that can output a directory. This is useful when doing things like
# generating a fuzz corpus mechanically.
def _envoy_directory_genrule_impl(ctx):
Expand Down Expand Up @@ -181,9 +189,6 @@ envoy_select_boringssl = _envoy_select_boringssl
envoy_select_google_grpc = _envoy_select_google_grpc
envoy_select_hot_restart = _envoy_select_hot_restart
envoy_select_wasm = _envoy_select_wasm
envoy_select_wasm_wavm = _envoy_select_wasm_wavm
envoy_select_wasm_wasmtime = _envoy_select_wasm_wasmtime
envoy_select_wasm_v8 = _envoy_select_wasm_v8
envoy_select_new_codecs_in_integration_tests = _envoy_select_new_codecs_in_integration_tests

# Binary wrappers (from envoy_binary.bzl)
Expand Down
12 changes: 11 additions & 1 deletion bazel/envoy_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,17 @@ def envoy_cc_extension(
fail("Unknown extension status: " + status)
if "//visibility:public" not in visibility:
visibility = visibility + extra_visibility
envoy_cc_library(name, tags = tags, visibility = visibility, **kwargs)

lib_name = "_" + name + "_extension_lib"
envoy_cc_library(name = lib_name, tags = tags, visibility = visibility, **kwargs)
cc_library(
name = name,
deps = select({
":is_enabled": [":" + lib_name],
"//conditions:default": [],
}),
visibility = visibility,
)

# Envoy C++ library targets should be specified with this function.
def envoy_cc_library(
Expand Down
20 changes: 0 additions & 20 deletions bazel/envoy_select.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,26 +39,6 @@ def envoy_select_wasm(xs):
"//conditions:default": xs,
})

def envoy_select_wasm_v8(xs):
return select({
"@envoy//bazel:wasm_wasmtime": [],
"@envoy//bazel:wasm_wavm": [],
"@envoy//bazel:wasm_none": [],
"//conditions:default": xs,
})

def envoy_select_wasm_wavm(xs):
return select({
"@envoy//bazel:wasm_wavm": xs,
"//conditions:default": [],
})

def envoy_select_wasm_wasmtime(xs):
return select({
"@envoy//bazel:wasm_wasmtime": xs,
"//conditions:default": [],
})

# Select the given values if use legacy codecs in test is on in the current build.
def envoy_select_new_codecs_in_integration_tests(xs, repository = ""):
return select({
Expand Down
2 changes: 1 addition & 1 deletion ci/build_setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ BAZEL_BUILD_OPTIONS=(
"${BAZEL_EXTRA_TEST_OPTIONS[@]}")

[[ "${ENVOY_BUILD_ARCH}" == "aarch64" ]] && BAZEL_BUILD_OPTIONS+=(
"--define" "wasm=disabled"
"--define" "wasm_tests=disabled"
"--flaky_test_attempts=2"
"--test_env=HEAPCHECK=")

Expand Down
22 changes: 13 additions & 9 deletions ci/do_ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then
"--define" "deprecated_features=disabled"
"--define" "use_new_codecs_in_integration_tests=false"
"--define" "tcmalloc=gperftools"
"--define" "zlib=ng")
"--define" "zlib=ng"
"--//source/extensions/wasm_runtime/wavm:enabled")

ENVOY_STDLIB="${ENVOY_STDLIB:-libstdc++}"
setup_clang_toolchain
Expand All @@ -296,22 +297,25 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then
TEST_TARGETS=("@envoy//test/...")
fi
# Building all the dependencies from scratch to link them against libc++.
echo "Building and testing with wasm=wasmtime: ${TEST_TARGETS[*]}"
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wasmtime "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only
echo "Building and testing with wasmtime: ${TEST_TARGETS[*]}"
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" "${COMPILE_TIME_OPTIONS[@]}" \
--//source/extensions/wasm_runtime/wasmtime:enabled \
--//source/extensions/wasm_runtime/wavm:enabled=false \
-c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only

echo "Building and testing with wasm=wavm: ${TEST_TARGETS[*]}"
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wavm "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only
echo "Building and testing with wavm: ${TEST_TARGETS[*]}"
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" "${COMPILE_TIME_OPTIONS[@]}" -c dbg "${TEST_TARGETS[@]}" --test_tag_filters=-nofips --build_tests_only

# Legacy codecs "--define legacy_codecs_in_integration_tests=true" should also be tested in
# integration tests with asan.
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wavm "${COMPILE_TIME_OPTIONS[@]}" -c dbg @envoy//test/integration/... --config=clang-asan --build_tests_only
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" "${COMPILE_TIME_OPTIONS[@]}" -c dbg @envoy//test/integration/... --config=clang-asan --build_tests_only

# "--define log_debug_assert_in_release=enabled" must be tested with a release build, so run only
# these tests under "-c opt" to save time in CI.
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wavm "${COMPILE_TIME_OPTIONS[@]}" -c opt @envoy//test/common/common:assert_test @envoy//test/server:server_test
bazel_with_collection test "${BAZEL_BUILD_OPTIONS[@]}" "${COMPILE_TIME_OPTIONS[@]}" -c opt @envoy//test/common/common:assert_test @envoy//test/server:server_test

echo "Building binary with wasm=wavm..."
bazel build "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wavm "${COMPILE_TIME_OPTIONS[@]}" -c dbg @envoy//source/exe:envoy-static --build_tag_filters=-nofips
echo "Building binary with wavm..."
bazel build "${BAZEL_BUILD_OPTIONS[@]}" "${COMPILE_TIME_OPTIONS[@]}" -c dbg @envoy//source/exe:envoy-static --build_tag_filters=-nofips
Copy link
Member

Choose a reason for hiding this comment

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

It's no longer clear from the CLI that this is wavm, is there an explicit toggle possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

because it's now a flag and not a define, it can be override, so I moved back the wavm enable flag into L288 above, and disable when we compile with wasmtime.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I think this makes it harder to read; having this explicit here rather than under COMPILE_TIME_OPTIONS would make later maintenance/understandability better.

collect_build_profile build
exit 0
elif [[ "$CI_TARGET" == "bazel.api" ]]; then
Expand Down
2 changes: 1 addition & 1 deletion ci/windows_ci_steps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ BAZEL_BUILD_OPTIONS=(
-c opt
--show_task_finish
--verbose_failures
--define "wasm=disabled"
--define "wasm_tests=disabled"
"--test_output=errors"
"${BAZEL_BUILD_EXTRA_OPTIONS[@]}"
"${BAZEL_EXTRA_TEST_OPTIONS[@]}")
Expand Down
8 changes: 4 additions & 4 deletions examples/wasm-cc/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ envoy_package()

selects.config_setting_group(
name = "include_wasm_config",
match_all = ["//bazel:x86", "//bazel:wasm_v8"],
match_all = ["//bazel:x86"],
)

filegroup(
name = "configs",
srcs = glob(
[
"**/*.wasm",
],
[
"**/*.wasm",
],
) + select({
":include_wasm_config": glob(
[
Expand Down
4 changes: 1 addition & 3 deletions source/extensions/wasm_runtime/v8/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ load(
"envoy_cc_extension",
"envoy_extension_package",
)
load("//bazel:envoy_select.bzl", "envoy_select_wasm_v8")

licenses(["notice"]) # Apache 2

Expand All @@ -17,7 +16,6 @@ envoy_cc_extension(
deps = [
"//include/envoy/registry",
"//source/extensions/common/wasm:wasm_runtime_factory_interface",
] + envoy_select_wasm_v8([
"@proxy_wasm_cpp_host//:v8_lib",
]),
],
)
6 changes: 2 additions & 4 deletions source/extensions/wasm_runtime/wasmtime/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@ load(
"envoy_cc_extension",
"envoy_extension_package",
)
load("//bazel:envoy_select.bzl", "envoy_select_wasm_wasmtime")

licenses(["notice"]) # Apache 2

envoy_extension_package()
envoy_extension_package(enabled_default = False)

envoy_cc_extension(
name = "config",
Expand All @@ -17,7 +16,6 @@ envoy_cc_extension(
deps = [
"//include/envoy/registry",
"//source/extensions/common/wasm:wasm_runtime_factory_interface",
] + envoy_select_wasm_wasmtime([
"@proxy_wasm_cpp_host//:wasmtime_lib",
]),
],
)
6 changes: 2 additions & 4 deletions source/extensions/wasm_runtime/wavm/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@ load(
"envoy_cc_extension",
"envoy_extension_package",
)
load("//bazel:envoy_select.bzl", "envoy_select_wasm_wavm")

licenses(["notice"]) # Apache 2

envoy_extension_package()
envoy_extension_package(enabled_default = False)

envoy_cc_extension(
name = "config",
Expand All @@ -17,7 +16,6 @@ envoy_cc_extension(
deps = [
"//include/envoy/registry",
"//source/extensions/common/wasm:wasm_runtime_factory_interface",
] + envoy_select_wasm_wavm([
"@proxy_wasm_cpp_host//:wavm_lib",
]),
],
)
2 changes: 1 addition & 1 deletion tools/code_format/envoy_build_fixer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def FixPackageAndLicense(path, contents):
# Envoy package is inserted after the load block containing the
# envoy_package import.
package_and_parens = package_string + '()'
if package_and_parens not in contents:
if package_string + '(' not in contents:
contents = re.sub(regex_to_use, r'\1\n%s\n\n' % package_and_parens, contents)
if package_and_parens not in contents:
raise EnvoyBuildFixerError('Unable to insert %s' % package_and_parens)
Expand Down