[PATCH] D143351: [bazel] Remove unused dependency on libxml2

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

GMNGeoffrey accepted this revision.
GMNGeoffrey added a comment.



In D143351#4108160 <https://reviews.llvm.org/D143351#4108160>, @aaronmondal wrote:

> This shouldn't affect anyone using default Bazel configs. The functionality that depends on libxml is disabled (as in, disabled on the preprocessor level). Not having xml2 support disables symbols in `llvm/lib/WindowsManifest/WindowsManifestMerger.cpp`, but linking xml2 it here without setting `LLVM_ENABLE_XML2` has no effect.
>
> This functionality should currently only be usable if you manually patched the Bazel configs.
>
> Ultimately I'd like to make this a configurable flag depending on/setting `LLVM_ENABLE_XML2`, but I think the CMake-to-Bazel scripts are't ready to support this yet. It's probably better to wait for https://reviews.llvm.org/D143295 to get through first and then add configurability separately.

Yeah that makes sense. I'm just pretty sure that at the time there was some reason that Chandler added it to begin with and I'm trying to make sure we understand why it was there before removing it (Chesterton's Fence 🙂). Maybe it was indeed in service of a downstream who was patching the config. I think at Google we turn this on, but we use a hermetic version so this doesn't affect us either. Anyways, I'm pretty sure the current state is nonsensical as you say, so I'm ok to go ahead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143351