[PATCH] D119547: [libc][bazel] Add tests to the bazel build

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey accepted this revision.
GMNGeoffrey added a comment.

LGTM with some nits



================
Comment at: utils/bazel/WORKSPACE:76
+git_repository(
+    name = "bazel_toolchains",
+    tag = "5.1.2",
----------------
It seems like we don't actually use this anywhere?


================
Comment at: utils/bazel/WORKSPACE:90
+# We set register_built_tools to False to prevent building make from source.
+# Building make from source fails because of "-Wall -Werror" (.blazerc).
+rules_foreign_cc_dependencies(register_built_tools = False)
----------------
bazelrc. Note that this is no longer set in the bazelr by default, but it is set in the CI


================
Comment at: utils/bazel/llvm-project-overlay/libc/BUILD.bazel:28
+# A flag to pick which mpfr to use for math tests
+string_flag(
+    name = "libc_math_mpfr",
----------------
I think it would be good to throw a usage example in here and a link to the docs. This Bazel flag stuff is pretty new and I think not widely used. IIUC this would be passed like `bazel build @llvm-project//libc:libc_math_mpfr=disable ...`. Having written it out, I think you should drop the "libc" from the flag name: it already has to be referenced with a qualified name. Is "math" important? Maybe just `bazel build @llvm-project//libc:mpfr=disable ...`. That looks quite a bit cleaner


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl:9-25
+def libc_test(name, srcs, libc_function_deps, **kwargs):
+    """Add target for a libc test.
+
+    Args:
+      name: Test target name
+      srcs: List of sources for the test.
+      libc_function_deps: List of libc_function targets used by this test.
----------------
It's a bit weird to introspect kwargs this way IMO. You can instead add deps as an explicit kwarg with a default




================
Comment at: utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl:1
+"""LLVM libc starlark rules for tests."""
+
----------------
GMNGeoffrey wrote:
> Please add a license header and further explanation of why tests are constructed this way.
Would still like to see an explanation of why these tests are constructed this way. The docstrings kind of look like just rearranging the words in the file/function name


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl:4
+def math_test(name, hdrs = None, **kwargs):
+    """Add a target for the unittest of a math function.
+
----------------
Again, more explanation of what's going on here and why would be helpful


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl:13
+    srcs = [test_name + ".cpp"] + (hdrs or [])
+    if "deps" not in kwargs:
+        kwargs["deps"] = []
----------------
Same comment about just making deps explicit


================
Comment at: utils/bazel/llvm-project-overlay/libc/test/src/stdlib/BUILD.bazel:1
+# Tests for LLVM libc stdlib.h functions.
+
----------------
Please add license headers to all the files, as we have in the other Bazel files


================
Comment at: utils/bazel/llvm-project-overlay/libc/utils/MPFRWrapper/BUILD.bazel:9-12
+    target_compatible_with = select({
+        "//conditions:default": [],
+        "//libc:libc_math_mpfr_disable": ["//libc:not_compatible"],
+    }),
----------------
It is wild to me that this is how you disable something in Bazel...


================
Comment at: utils/bazel/third_party_build/mpfr.BUILD:11
+    toolchain = "@rules_foreign_cc//toolchains:preinstalled_autoconf_toolchain",
+    copts = ["-Wno-error"],  # ./configure crashes with `-Werror`
+    lib_name = "libmpfr",
----------------
GMNGeoffrey wrote:
> Probably want to turn off all warnings entirely. Getting warnings from this is not going to be useful for anyone. I think `-w` is the flag for both clang and gcc.
I think that with this you don't need to also turn off `-Werror`. `-w` -> no warnings so it doesn't matter that all warnings are errors :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119547/new/

https://reviews.llvm.org/D119547