[PATCH] D150641: [bazel] Refactor entries in WORKSPACE to bzl files

[Aaron Siddhartha Mondal via Phabricator via llvm-commits]

aaronmondal accepted this revision.
aaronmondal added a comment.
This revision is now accepted and ready to land.

Sorry for the late reply. LGTM.

I'm not a fan of the `deps.bzl` pattern in general. For instance, with a Protobuf workspace for multiple languages the import order of things can get very confusing when every ruleset imports their own variant of grpc/proto dependencies with potentially clashing names. This has always been an issue though and virtually every other WORKSPACE-based Bazel project uses the `deps.bzl` pattern. So I'd expect users to be used to this and "know where to look" when seeing an `llvm_deps.bzl` file. So for WORKSPACEs I'd say this is about as "standardized" as we could get.

Overriding dependencies can be done by adding overrides of the same name as the `http_archives` in `llvm_deps.bzl` before calling `llvm_deps()` in the WORKSPACE. I.e.

  # WORKSPACE.bazel
  
  load("@llvm-raw//utils/bazel/repos:llvm_deps.bzl", "llvm_deps")
  
  http_archive(
      name = "myoverriddendep",
      ...
  )
  
  llvm_deps()

Whether the overriden dep is actually enabled or not in the build is then a configuration flag in `.bazelrc`. E.g.

  # .bazelrc
  
  build -- at llvm-project//clang-tools-extra/clang-tidy:enable_static_analyzer=false
  build -- at llvm_zstd//:llvm_enable_zstd=false

This should be fairly CI friendly since it allows easily setting different build configurations within a single `.bazelrc` via `--config` options and no longer requires custom environment variables. It also decouples customizations from the BUILD files.

I'm not sure whether the examples currently work, but they'll definitely be broken after this patch. I don't have a strong opinion on whether they should be updated as part of this patch or in a later patch. I'd actually lean towards "later" and then do it properly with a more explicit README etc. There is also the option to remove the examples entirely and add all of that to the main README. Considering that using the overlay is roughly just

  http_archive(
      name = "llvm-raw",
      ...
  )
  
  load("@llvm-raw//utils/bazel/repos:llvm_deps.bzl", "llvm_deps")
  
  # Optionally, override dependencies here.
  # http_archive(name = "myoverride", ...)
  
  # Some components of LLVM are configurable by adding the corresponding
  # config options in your `.bazelrc`. See third-party-deps for details.
  
  llvm_deps()
  
  load("@llvm-raw//utils/bazel/repos:llvm_repos.bzl", "llvm_repos")
  llvm_repos(name = "llvm-project")

(+- skylib/foreign_cc), I think this would also be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150641