From 296c34333288b721fb74c63bbdbd7c0a1e253ff5 Mon Sep 17 00:00:00 2001 From: MediaPipe Team Date: Fri, 10 Mar 2023 10:56:33 -0800 Subject: [PATCH] Revise the Halide Bazel build rules to be cleaner and more correct. The initial version we landed happened to work (at least in most cases) but had known workarounds in it. AFAICT, the issue seemed to be that the MediaPipe `config_setting()` values we were using didn't always resolve to the right thing when detecting the host cpu (vs the compile-target cpu), which is critical for Halide. I fixed this by avoiding the use of the MediaPipe config_settings entirely, using instead the public Bazel settings hosted at https://github.com/bazelbuild/platforms. This revision builds and runs //mediapipe/util/frame_buffer:all correctly on my mac x86-64 laptop; I haven't yet attempted to build or test on any other platform, so there may well still be glitches, but I think this is more fundamentally sound than what we had before. PiperOrigin-RevId: 515682507 --- third_party/halide.BUILD | 2 +- third_party/halide/BUILD.bazel | 18 ++++++++------- third_party/halide/halide.bzl | 41 +++++++++++++++++++++------------- 3 files changed, 36 insertions(+), 25 deletions(-) diff --git a/third_party/halide.BUILD b/third_party/halide.BUILD index 5578c5650..02e701585 100644 --- a/third_party/halide.BUILD +++ b/third_party/halide.BUILD @@ -42,7 +42,7 @@ cc_library( cc_library( name = "lib_halide_static", srcs = select({ - "@mediapipe//mediapipe:windows": [ + "@halide//:halide_config_windows_x86_64": [ "lib/Release/Halide.lib", "bin/Release/Halide.dll", ], diff --git a/third_party/halide/BUILD.bazel b/third_party/halide/BUILD.bazel index 659087da7..8b69a2503 100644 --- a/third_party/halide/BUILD.bazel +++ b/third_party/halide/BUILD.bazel @@ -26,14 +26,16 @@ halide_library_runtimes() [ alias( name = target_name, - actual = select({ - "//conditions:default": "@linux_halide//:" + target_name, - # TODO: this shouldn't be necessary. - "@mediapipe//mediapipe:macos_i386": "@macos_x86_64_halide//:" + target_name, - "@mediapipe//mediapipe:macos_x86_64": "@macos_x86_64_halide//:" + target_name, - "@mediapipe//mediapipe:macos_arm64": "@macos_arm_64_halide//:" + target_name, - "@mediapipe//mediapipe:windows": "@windows_halide//:" + target_name, - }), + actual = select( + { + ":halide_config_linux_x86_64": "@linux_halide//:%s" % target_name, + ":halide_config_macos_x86_64": "@macos_x86_64_halide//:%s" % target_name, + ":halide_config_macos_arm64": "@macos_arm_64_halide//:%s" % target_name, + ":halide_config_windows_x86_64": "@windows_halide//:%s" % target_name, + # deliberately no //condition:default clause here + }, + no_match_error = "Compiling Halide code requires that the build host is one of Linux x86-64, Windows x86-64, macOS x86-64, or macOS arm64.", + ), ) for target_name in [ "language", diff --git a/third_party/halide/halide.bzl b/third_party/halide/halide.bzl index 68d31fe48..8d0d48e32 100644 --- a/third_party/halide/halide.bzl +++ b/third_party/halide/halide.bzl @@ -82,26 +82,22 @@ def halide_runtime_linkopts(): # Map of halide-target-base -> config_settings _HALIDE_TARGET_CONFIG_SETTINGS_MAP = { # Android - "arm-32-android": ["@mediapipe//mediapipe:android_arm"], - "arm-64-android": ["@mediapipe//mediapipe:android_arm64"], - "x86-32-android": ["@mediapipe//mediapipe:android_x86"], - "x86-64-android": ["@mediapipe//mediapipe:android_x86_64"], + "arm-32-android": ["@halide//:halide_config_android_arm"], + "arm-64-android": ["@halide//:halide_config_android_arm64"], + "x86-32-android": ["@halide//:halide_config_android_i386"], + "x86-64-android": ["@halide//:halide_config_android_x86_64"], # iOS - "arm-32-ios": ["@mediapipe//mediapipe:ios_armv7"], - "arm-64-ios": ["@mediapipe//mediapipe:ios_arm64", "@mediapipe//mediapipe:ios_arm64e"], + "arm-32-ios": ["@halide//:halide_config_ios_arm"], + "arm-64-ios": ["@halide//:halide_config_ios_arm64"], # OSX (or iOS simulator) - "x86-64-osx": [ - "@mediapipe//mediapipe:macos_x86_64", - "@mediapipe//mediapipe:ios_x86_64", - # TODO: these should map to "x86-32-osx", but that causes Kokoro to fail. - "@mediapipe//mediapipe:macos_i386", - "@mediapipe//mediapipe:ios_i386", - ], - "arm-64-osx": ["@mediapipe//mediapipe:macos_arm64"], + "x86-32-osx": ["@halide//:halide_config_macos_i386", "@halide//:halide_config_ios_i386"], + "x86-64-osx": ["@halide//:halide_config_macos_x86_64", "@halide//:halide_config_ios_x86_64"], + "arm-64-osx": ["@halide//:halide_config_macos_arm64"], # Windows - "x86-64-windows": ["@mediapipe//mediapipe:windows"], + "x86-64-windows": ["@halide//:halide_config_windows_x86_64"], # Linux - "x86-64-linux": ["//conditions:default"], + "x86-64-linux": ["@halide//:halide_config_linux_x86_64"], + # deliberately nothing here using //conditions:default } _HALIDE_TARGET_MAP_DEFAULT = { @@ -622,6 +618,19 @@ def _standard_library_runtime_names(): return collections.uniq([_halide_library_runtime_target_name(f) for f in _standard_library_runtime_features()]) def halide_library_runtimes(compatible_with = []): + # Note that we don't use all of these combinations + # (and some are invalid), but that's ok. + for cpu in ["arm", "arm64", "i386", "x86_64"]: + for os in ["android", "linux", "windows", "ios", "macos"]: + native.config_setting( + name = "halide_config_%s_%s" % (os, cpu), + constraint_values = [ + "@platforms//os:%s" % os, + "@platforms//cpu:%s" % cpu, + ], + visibility = ["//visibility:public"], + ) + unused = [ _define_halide_library_runtime(f, compatible_with = compatible_with) for f in _standard_library_runtime_features()