This is an archive of the discontinued LLVM Phabricator instance.

[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

gchatelet created this revision.Feb 11 2022, 7:36 AM
gchatelet requested review of this revision.Feb 11 2022, 7:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 7:36 AM

gmp and mpfr are built from source. Right now this is quite slow as Bazel does not parallelize the job yet (https://github.com/bazelbuild/rules_foreign_cc/issues/329).
Once the build is cached this is fast though.

We'd need to have autotools installed on the CI machine though.

gchatelet updated this revision to Diff 407886.Feb 11 2022, 8:16 AM
  • Add documentation on why -Wno-error is needed to compile gmp and mpfr.

I think we should provide at least the option for users to use these from their system, as we do with zlib

utils/bazel/WORKSPACE
79

Maybe switch to git_repository

@chandlerc any opinions on having multiple methods for depending on external libraries? Version 0.7 doesn't inspire confidence

gchatelet marked an inline comment as done.Mar 9 2022, 7:17 AM

@GMNGeoffrey I'm having an issue that you may know how to solve.
.bazelrc automatically appends -Wall -Werror to everything that is built.

  • Math tests depend on mpfr,
  • mpfr (and gmp) are built from source using rules_foreign_cc,
  • For hermeticity rules_foreign_cc compiles make

I could tweak the ./configure options for mpfr and gmp and explicitly disable -Werror (see file "mpfr.BUILD" and "gmp.BUILD") but I cannot tweak the compile options for when rules_foreign_cc compiles make.
I could work around this by preventing rules_foreign_cc to compiles make in the first place, but then the build is not hermetic anymore...

register_built_tools = False,
register_default_tools = False,

What do you think? Is .bazelrc too strict? Is there a clean way to work around this without lowering the error level?

Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 7:17 AM

Another hack is to modify the .bazelrc file to pass -Wall -Werror only to cxxopt, instead of copt. Since make, gmp and mpfr are C and not C++ it works. But yeah, quite a hacky solution as well,

@GMNGeoffrey I'm having an issue that you may know how to solve.
.bazelrc automatically appends -Wall -Werror to everything that is built.

  • Math tests depend on mpfr,
  • mpfr (and gmp) are built from source using rules_foreign_cc,
  • For hermeticity rules_foreign_cc compiles make

I could tweak the ./configure options for mpfr and gmp and explicitly disable -Werror (see file "mpfr.BUILD" and "gmp.BUILD") but I cannot tweak the compile options for when rules_foreign_cc compiles make.
I could work around this by preventing rules_foreign_cc to compiles make in the first place, but then the build is not hermetic anymore...

register_built_tools = False,
register_default_tools = False,

What do you think? Is .bazelrc too strict? Is there a clean way to work around this without lowering the error level?

You can use per_file_copt to limit where copts get applied, and for clang specifically the system_header_prefix copt. I think using the make on a user's machine instead of compiling it from source is entirely reasonable though. Only at Google would that be considered a good idea

gchatelet updated this revision to Diff 414349.Mar 10 2022, 5:07 AM
  • Use git_repository with tags, re-enable tool registration to check if CI works (m4 is missing), apply .bazelrc flags more specifically
gchatelet updated this revision to Diff 415782.Mar 16 2022, 4:42 AM
  • Fix bazel files after cpp_standalone splitting
gchatelet updated this revision to Diff 417606.Mar 23 2022, 7:35 AM
  • Update bazel files to get closer to internal files
gchatelet updated this revision to Diff 417639.Mar 23 2022, 8:40 AM
  • Try to use prebuilt tools
gchatelet updated this revision to Diff 472927.Nov 3 2022, 7:08 AM
  • rebase and update
gchatelet updated this revision to Diff 472966.Nov 3 2022, 9:36 AM
  • remove project prefix
gchatelet updated this revision to Diff 473135.Nov 4 2022, 12:00 AM
  • Update file structure

Is this ready for review?

Is this ready for review?

Almost, I'm still facing internal import issues that also need to be resolved upstream. I'll ping here when it's ready.

gchatelet updated this revision to Diff 473188.Nov 4 2022, 4:25 AM
  • update visibility

As-is this patch requires to have autotools installed (make, m4 and maybe other tools)
@GMNGeoffrey @sivachandra can we update the bazel builder?

gchatelet updated this revision to Diff 473229.Nov 4 2022, 7:25 AM

Let rules_foreign_cc_dependencies build make - this should fixe premerge bot.

gchatelet added a comment.EditedNov 4 2022, 7:27 AM

As-is this patch requires to have autotools installed (make, m4 and maybe other tools)
@GMNGeoffrey @sivachandra can we update the bazel builder?

I just updated the patch so it builds make hermetically but I had to disable copt_host -Werror
See diff https://reviews.llvm.org/D119547?vs=473188&id=473229#toc

Let me know which one of the two solutions you prefer.

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.

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?

GMNGeoffrey added inline comments.Nov 8 2022, 2:01 PM
utils/bazel/.bazelrc
50–51

Based on your other comment, I think this could be removed now? But also there's https://reviews.llvm.org/D123481 to remove -Werror from the default config. Note, though, that the CI still builds with -Werror

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

Please add a license header and further explanation of why tests are constructed this way.

14

Can you use **kwargs for consistency with the rest of the starlark macros?

17

It seems like calling the "additional_deps" argument "deps" would also be fine? Generally it's preferred to use standard names (https://bazel.build/rules/bzl-style). I do see the argument for different naming, thouhg. Your call

utils/bazel/llvm-project-overlay/libc/test/src/math/BUILD.bazel
218

Couldn't we just tag it to be excluded from Aarch64 tests?

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

Oh no 😨 How much does this extend CI runtime? Also, how many tests are like this? Instead of hardcoding "enormous" for every test, could you just pass that argument for the few that are in fact enormous? Separately, I think it is a better fit to use the timeout argument here timeout = "eternal". Test size is ill-defined, but I think by any reasonable definition this test isn't an enormous one. Those usually involve spinning up whole servers and such. I think it's a small test that just happens to take a very long time.

utils/bazel/third_party_build/mpfr.BUILD
12

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.

gchatelet updated this revision to Diff 474200.Nov 9 2022, 2:09 AM
  • Let rules_foreign_cc_dependencies build make
  • Revert "Let rules_foreign_cc_dependencies build make"
gchatelet updated this revision to Diff 474223.Nov 9 2022, 4:19 AM
gchatelet marked 5 inline comments as done.
  • 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.Nov 9 2022, 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.Nov 9 2022, 4:46 AM
  • Use selects.with_or as platform can be a list.
gchatelet updated this revision to Diff 474240.Nov 9 2022, 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.Nov 9 2022, 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.Nov 10 2022, 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.Nov 10 2022, 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.Nov 10 2022, 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.Nov 14 2022, 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.Nov 15 2022, 7:26 AM
  • libc math mpfr=(disable|system|eternal)
gchatelet added a comment.EditedNov 15 2022, 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.EditedNov 15 2022, 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.Nov 15 2022, 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.Nov 15 2022, 11:52 PM
gchatelet edited the summary of this revision. (Show Details)Nov 16 2022, 12:37 AM
lntue added inline comments.Nov 16 2022, 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.Nov 16 2022, 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.Nov 16 2022, 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.Nov 16 2022, 1:18 PM
GMNGeoffrey accepted this revision.Nov 16 2022, 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.Nov 17 2022, 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.Nov 17 2022, 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.Nov 18 2022, 1:29 AM

Reopening to use bazel_platforms incompatible_setting instead of creating my own.

This revision is now accepted and ready to land.Nov 18 2022, 1:29 AM
gchatelet updated this revision to Diff 476422.Nov 18 2022, 4:31 AM
  • Removed custom constrain_setttings and rely on @platform//:incompatible
  • Format build files
gchatelet updated this revision to Diff 476426.Nov 18 2022, 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.