[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3646561 <https://reviews.llvm.org/D128465#3646561>, @ckissane wrote:

> In D128465#3646258 <https://reviews.llvm.org/D128465#3646258>, @MaskRay wrote:
>
>> In D128465#3646203 <https://reviews.llvm.org/D128465#3646203>, @ckissane wrote:
>>
>>> In D128465#3642997 <https://reviews.llvm.org/D128465#3642997>, @MaskRay wrote:
>>>
>>>> As I mentioned, the proper approach is to add zstd functionality along with the CMake change, instead of adding CMake to all llvm-project components without a way to test them.
>>>
>>> @MaskRay, I have now done this and ran the ldd tests as requested:
>>>
>>>   With LLVM_ENABLE_ZSTD=ON
>>>   $ ninja check-lld
>>>   Testing Time: 8.98s
>>>     Unsupported      :   17
>>>     Passed           : 2638
>>>     Expectedly Failed:    1
>>>   With LLVM_ENABLE_ZSTD=OFF
>>>   $ ninja check-lld
>>>   Testing Time: 8.95s
>>>     Unsupported      :   17
>>>     Passed           : 2638
>>>     Expectedly Failed:    1
>>
>> I request that you abandon this patch and incorporate the llvm cmake part into the llvm patch which you actually use zstd.
>> It is not appropriate to add zstd cmake to llvm-project components which don't use zstd.
>
> Let me see if I understand this correctly:
> Are you saying that I should abandon this patch, and create a new patch that:
>
> - adds findZSTD.cmake
> - adds zstd to compression namespace
> - adds tests to CompressionTest.cpp
> - and does the **minimal** amount of cmake work to have the flags and test work
>   - thus differing per component cmake config to patches like how you are doing in https://reviews.llvm.org/D129406
>
> Is this correct?
>
> I realize though that then https://reviews.llvm.org/D129406 or similar would be "the [first] llvm patch which actually use[s] zstd"

You can find a component in llvm which uses zstd (e.g. lib/Support). Add the functionality with tests (e.g. a CompressionTest.cpp unittest) along with the CMake/(optional Bazel, gn) changes.
Keep clang, clang-tools-extra, lld, etc unchanged. These components don't yet use zstd and the CMake changes in this patch will be untestable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465