This is an archive of the discontinued LLVM Phabricator instance.

[Bazel][GN] Reuse the GN LLVM config file generation code
ClosedPublic

Authored by rnk on May 27 2022, 5:29 PM.

Details

Summary

Currently, the Bazel build uses static, checked in [llvm-]config.h files
in combination with global macro definitions to mimic CMake's generated
headers. This change reuses the write_cmake_config.py script from the GN
build to generate the headers from source in the same way. The purpose
is to ensure that the Bazel build stays up to date with any changes to
the CMake config files. The write_cmake_config.py script has good error
checking to ensure that unneeded, stale variables are not passed, and
that any missing variables are reported as errors.

I tried to closely follow the logic in the GN build here:

llvm/utils/gn/secondary/llvm/include/Config/BUILD.gn

The duplication between this file and config.bzl is significant, and we
could consider going further, but I'd like to hold off on it for now.

The GN build changes are to move the write_cmake_config.py script up to
//llvm/utils/write_cmake_config.py, and update the paths accordingly.

The next logical change is to generate Clang's config.h header.

Diff Detail

Event Timeline

rnk created this revision.May 27 2022, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 5:29 PM
rnk requested review of this revision.May 27 2022, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 5:29 PM
thakis accepted this revision.May 29 2022, 12:11 PM

Cool! LG from the GN side.

(I think chances are low that someone changing the .py script to fix the GN build would break the bazel build or the other way round: The script hasn't meaningfully changed in well over 2 years.)

This revision is now accepted and ready to land.May 29 2022, 12:11 PM

(I think chances are low that someone changing the .py script to fix the GN build would break the bazel build or the other way round: The script hasn't meaningfully changed in well over 2 years.)

(If we end up doing a bunch of changes to the .py script later on for some reason or another, it might make sense to have a gn and a bazel version of that script at that point, though.)

MaskRay accepted this revision.May 29 2022, 9:26 PM
rnk added a comment.May 31 2022, 3:42 PM

(I think chances are low that someone changing the .py script to fix the GN build would break the bazel build or the other way round: The script hasn't meaningfully changed in well over 2 years.)

(If we end up doing a bunch of changes to the .py script later on for some reason or another, it might make sense to have a gn and a bazel version of that script at that point, though.)

Yeah, I agree, if it comes to that, we should split the script.

This revision was landed with ongoing or failed builds.May 31 2022, 7:43 PM
This revision was automatically updated to reflect the committed changes.

I see you've already landed and reverted this. Apologies I didn't get to review in time. In previous iterations, we did have a python script for writing these config files that came from TensorFlow. I think it was less good than this one in terms of error detection. @chandlerc had strong opinions about not using a custom script here, I believe arguing that behavior that is "like CMake but not quite CMake" was likely to be error-prone. FWIW, I think this is overall an improvement (especially with its error detection instead of our existing config file copies), but I think it's worth bringing this up again. I think I even looked at using the GN script in Bazel and Chandler was against it.

utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h