Page MenuHomePhabricator

[libc][bazel] Add tests to the bazel build
ClosedPublic

Authored by gchatelet on Feb 11 2022, 7:36 AM.

Details

Summary

This patch adds bazel tests for llvm-libc.

Some math tests rely on the mpfr library. This is controlled via the --@llvm-project//libc:libc_math_mpfr flag. It can take three values:

  • external (default) will build mpfr and gmp from source.
  • system will use the system installed mpfr library.
  • disable will skip tests relying on mpfr.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Add licence to libc_test_rules.bzl
  • Rename kw into kwargs
  • Disable math:sqrtl_test on aarch64
  • Suppress warnings for mpfr build
gchatelet added a subscriber: lntue.Wed, Nov 9, 4:19 AM
gchatelet added inline comments.
utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl
17
utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl
37

It is not that bad. Here are the top tests sorted by runtime:

@llvm-project//libc/test/src/math:hypot_test                             PASSED in 44.1s
@llvm-project//libc/test/src/math:hypotf_test                            PASSED in 31.4s
@llvm-project//libc/test/src/math:remquol_test                           PASSED in 19.7s
@llvm-project//libc/test/src/math:sincosf_test                           PASSED in 12.8s
@llvm-project//libc/test/src/math:sqrtf_test                             PASSED in 6.3s
@llvm-project//libc/test/src/math:sqrt_test                              PASSED in 6.0s
@llvm-project//libc/test/src/math:sqrtl_test                             PASSED in 4.8s
@llvm-project//libc/test/src/math:sinf_test                              PASSED in 3.8s
...

@lntue can we add the size tag only on the few bad ones?

gchatelet updated this revision to Diff 474224.Wed, Nov 9, 4:46 AM
  • Use selects.with_or as platform can be a list.
gchatelet updated this revision to Diff 474240.Wed, Nov 9, 5:47 AM
  • Add licence to libc_test_rules.bzl
  • Rename kw into kwargs
  • Disable math:sqrtl_test on aarch64
  • Suppress warnings for mpfr build
  • Use selects.with_or as platform can be a list.
  • Also add -w to gmp
  • Remove unneeded visibility
  • rebase
GMNGeoffrey added inline comments.Wed, Nov 9, 2:11 PM
utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl
37

Thos are all within the timeouts for "short", which is implied by "small". It doesn't seem like any of these should be tagged "enormous"

gchatelet added inline comments.Thu, Nov 10, 6:54 AM
utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl
37

So I've run more tests on different archs and build modes.

  • for local machine x86_64 test time is < 45s in opt and < 80s in fastbuild.
  • for local machine aarch64 test time is < 45s in optand < 87s in fastbuild.
  • for Google aarch64 test time is < 100s in opt and < 160s in fastbuild.
  • for Google qemued aarch64 test time is < 30m in opt and timing out after 1h in fastbuild.

So indeed these are enormous but only when run through an emulator. Let's mark the bigger tests wit the correct size and handle the timeouts internally.

gchatelet updated this revision to Diff 474564.Thu, Nov 10, 9:03 AM
  • Add missing library for sqrtl
  • Remove size from math_test
  • add --with-pic configure option to gmp and mpfr (necessary for aarch64)
gchatelet added inline comments.Thu, Nov 10, 9:04 AM
utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl
37

Actually I'm just removing the size attribute as the default is fine for all tests here.

edit Even if Bazel is able to build make, m4 is required to build gmp. It's actually a known issue : https://github.com/bazelbuild/rules_foreign_cc/issues/755
So for now we need at least m4, autoconf and automake to be installed on the machine anyways. So let's assume that make is available as well.

@aeubanks can we see about installing these dependencies on the builders?

was at the llvm dev meeting, I'll try to find time to look into this

@GMNGeoffrey @sivachandra can we update the bazel builder?

Sorry, I missed this - I do not maintain the bazel builder so this question should probably be answered by someone who maintains it.

utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl
17

Fine with calling it deps. IIRC, I named it additional_deps and not deps so that we do not mistakenly end up putting libc_function_deps in deps.

it took ~90 seconds to build libgmp and then ~60 seconds to build libmpfr on the bots, then ~40 for the longest test, that's not a great build experience even if it'll be cached most times

but anyway, make and m4 are now installed on the pre and post submit bazel bots

is there an issue with using a preinstalled version of mpfr/gmp on the bots?

gchatelet updated this revision to Diff 475086.Mon, Nov 14, 2:23 AM
  • Use 'deps' instead of 'additional_deps'

is there an issue with using a preinstalled version of mpfr/gmp on the bots?

I don't think there are any issues, I think we can do the same as for zlib and terminfo.
Let me tinker with it a bit.

gchatelet updated this revision to Diff 475473.Tue, Nov 15, 7:26 AM
  • libc math mpfr=(disable|system|eternal)
gchatelet added a comment.EditedTue, Nov 15, 7:30 AM

@aeubanks I've added a way to use the system's mpfr.
We need to add --@llvm-project//libc:libc_math_mpfr=system on the build bot command line. Do you know where to change this?
Also could you add libmpfr-dev to the build bot? (libgmp-dev will automatically come as a dependency)

aeubanks added a comment.EditedTue, Nov 15, 8:57 AM

@aeubanks I've added a way to use the system's mpfr.
We need to add --@llvm-project//libc:libc_math_mpfr=system on the build bot command line. Do you know where to change this?
Also could you add libmpfr-dev to the build bot? (libgmp-dev will automatically come as a dependency)

see --config=ci at the bottom of utils/bazel/.bazelrc

and added libmpfr-dev to the bots

  • Use system's mpfr for CI
  • Make mpfr dependency explicit

@aeubanks I've added a way to use the system's mpfr.
We need to add --@llvm-project//libc:libc_math_mpfr=system on the build bot command line. Do you know where to change this?
Also could you add libmpfr-dev to the build bot? (libgmp-dev will automatically come as a dependency)

see --config=ci at the bottom of utils/bazel/.bazelrc

Thx!

  • Fix documentation
  • Add licence to libc_test_rules.bzl
  • Rename kw into kwargs
  • Disable math:sqrtl_test on aarch64
  • Suppress warnings for mpfr build
  • Use selects.with_or as platform can be a list.
  • Also add -w to gmp
  • Remove unneeded visibility
  • Add missing library for sqrtl
  • Remove size from math_test
  • add --with-pic configure option to gmp and mpfr (necessary for aarch64)
  • Use 'deps' instead of 'additional_deps'
  • libc math mpfr=(disable|system|eternal)
  • Use system's mpfr for CI
  • Make mpfr dependency explicit
  • Fix documentation
  • Add missing dependencies
  • Use canonical version of mpfr_impl
  • Rebase
gchatelet updated this revision to Diff 475586.Tue, Nov 15, 2:39 PM
  • Add licence to libc_test_rules.bzl
  • Rename kw into kwargs
  • Disable math:sqrtl_test on aarch64
  • Suppress warnings for mpfr build
  • Use selects.with_or as platform can be a list.
  • Also add -w to gmp
  • Remove unneeded visibility
  • Add missing library for sqrtl
  • Remove size from math_test
  • add --with-pic configure option to gmp and mpfr (necessary for aarch64)
  • Use 'deps' instead of 'additional_deps'
  • libc math mpfr=(disable|system|eternal)
  • Use system's mpfr for CI
  • Make mpfr dependency explicit
  • Fix documentation
  • Add missing dependencies
  • Use canonical version of mpfr_impl
  • Fix __support_unit deps after rebase
  • Fix wrong target
  • Rebase
  • Fix typo
  • Fix comment

I think it's good to go. @sivachandra @GMNGeoffrey looks good to you?

gchatelet marked 4 inline comments as done.Tue, Nov 15, 11:52 PM
gchatelet edited the summary of this revision. (Show Details)Wed, Nov 16, 12:37 AM
lntue added inline comments.Wed, Nov 16, 6:55 AM
utils/bazel/llvm-project-overlay/libc/test/src/math/BUILD.bazel
263–264

I think this comment does not apply anymore. Can you add a TODO for me to double check this on an aarch64 machine and re-enable the test? Thanks,

gchatelet updated this revision to Diff 475904.Wed, Nov 16, 1:08 PM
gchatelet marked an inline comment as done.
  • Add licence to libc_test_rules.bzl
  • Rename kw into kwargs
  • Disable math:sqrtl_test on aarch64
  • Suppress warnings for mpfr build
  • Use selects.with_or as platform can be a list.
  • Also add -w to gmp
  • Remove unneeded visibility
  • Add missing library for sqrtl
  • Remove size from math_test
  • add --with-pic configure option to gmp and mpfr (necessary for aarch64)
  • Use 'deps' instead of 'additional_deps'
  • libc math mpfr=(disable|system|eternal)
  • Use system's mpfr for CI
  • Make mpfr dependency explicit
  • Fix documentation
  • Add missing dependencies
  • Use canonical version of mpfr_impl
  • Fix __support_unit deps after rebase
  • Fix wrong target
  • Fix typo
  • Fix comment
  • Remove unneeded dependencies and aarch64 test restriction
utils/bazel/llvm-project-overlay/libc/test/src/math/BUILD.bazel
263–264

Just tested on an aarch64 machine with the following dependencies and it passes.
Get:1 http://deb.debian.org/debian bullseye/main arm64 libgmpxx4ldbl arm64 2:6.2.1+dfsg-1+deb11u1 [337 kB]
Get:2 http://deb.debian.org/debian bullseye/main arm64 libgmp-dev arm64 2:6.2.1+dfsg-1+deb11u1 [625 kB]
Get:3 http://deb.debian.org/debian bullseye/main arm64 libmpfr-dev arm64 4.1.0-3 [236 kB]

I'll simplify the target then.

sivachandra accepted this revision.Wed, Nov 16, 1:18 PM

LGTM but please wait for @GMNGeoffrey . As a follow up, can you please add a doc for building and testing with Bazel similar to https://libc.llvm.org/build_and_test.html?

This revision is now accepted and ready to land.Wed, Nov 16, 1:18 PM
GMNGeoffrey accepted this revision.Wed, Nov 16, 5:47 PM

LGTM with some nits

utils/bazel/WORKSPACE
76

It seems like we don't actually use this anywhere?

90

bazelrc. Note that this is no longer set in the bazelr by default, but it is set in the CI

utils/bazel/llvm-project-overlay/libc/BUILD.bazel
28

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

utils/bazel/llvm-project-overlay/libc/test/libc_test_rules.bzl
2

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

10–26

It's a bit weird to introspect kwargs this way IMO. You can instead add deps as an explicit kwarg with a default

utils/bazel/llvm-project-overlay/libc/test/src/math/libc_math_test_rules.bzl
5

Again, more explanation of what's going on here and why would be helpful

14

Same comment about just making deps explicit

utils/bazel/llvm-project-overlay/libc/test/src/stdlib/BUILD.bazel
2

Please add license headers to all the files, as we have in the other Bazel files

utils/bazel/llvm-project-overlay/libc/utils/MPFRWrapper/BUILD.bazel
10–13

It is wild to me that this is how you disable something in Bazel...

utils/bazel/third_party_build/mpfr.BUILD
12

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 :-)

gchatelet marked 10 inline comments as done.Thu, Nov 17, 3:31 AM
gchatelet added inline comments.
utils/bazel/WORKSPACE
90

This is no more relevant, thx for the heads up.
I've added some documentation as well.

utils/bazel/llvm-project-overlay/libc/utils/MPFRWrapper/BUILD.bazel
10–13

This is the recommended way as far as I understand
https://youtu.be/01BHJT7GL9o

Please correct me if I'm wrong!

utils/bazel/third_party_build/mpfr.BUILD
12

Fair enough 🙂

gchatelet updated this revision to Diff 476077.Thu, Nov 17, 3:31 AM
gchatelet marked 3 inline comments as done.
  • Remove unused bazel_toolchain
  • Update documentation in WORKSPACE
  • Rename mpfr flag and improve documentation
  • Use deps in rules arguments
  • Added documentation to custom rules
  • Add license headers
  • Remove unneeded -Wno-error

Thx @GMNGeoffrey for the valuable comments. I'll submit as is. Let me know if you have any other modifications and I'll deal with them in subsequent patches.

This revision was automatically updated to reflect the committed changes.
gchatelet reopened this revision.Fri, Nov 18, 1:29 AM

Reopening to use bazel_platforms incompatible_setting instead of creating my own.

This revision is now accepted and ready to land.Fri, Nov 18, 1:29 AM
gchatelet updated this revision to Diff 476422.Fri, Nov 18, 4:31 AM
  • Removed custom constrain_setttings and rely on @platform//:incompatible
  • Format build files
gchatelet updated this revision to Diff 476426.Fri, Nov 18, 4:52 AM
  • Remove unused load directives
  • Fix dosctring in libc_math_test_rules.bzl
This revision was automatically updated to reflect the committed changes.