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

[Cole Kissane via Phabricator via cfe-commits]

ckissane added a comment.

In D128465#3646601 <https://reviews.llvm.org/D128465#3646601>, @MaskRay wrote:

> 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.

Got it!, this patch already has such tests (CompressionTest.cpp unittest) in lib/Support, so I should just remove the untestable cmake config.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465