[libcxx-commits] [PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante marked 3 inline comments as done.
Mordante added a comment.

In D151344#4368926 <https://reviews.llvm.org/D151344#4368926>, @MaskRay wrote:

> Thank you for making another try for the treewide change (which is admittedly very painful and not many people do such work).

Thanks. And it indeed takes quite a lot of effort.

> Can you include the original patch description to the summary? That is the main part and the message "This reverts commit " is secondary. Readers should not need to trace through the revert history to obtain the original motivation.

Good suggestion I've updated the message.

Since @glandium and @hans seem happy with the changes I'll land this patch.



================
Comment at: libunwind/src/CMakeLists.txt:28-35
 
 # See add_asm_sources() in compiler-rt for explanation of this workaround.
 # CMake doesn't work correctly with assembly on AIX. Workaround by compiling
 # as C files as well.
 if((APPLE AND CMAKE_VERSION VERSION_LESS 3.19) OR
-   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17) OR
-   (${CMAKE_SYSTEM_NAME} MATCHES "AIX"))
+   (MINGW AND CMAKE_VERSION VERSION_LESS 3.17))
   set_source_files_properties(${LIBUNWIND_ASM_SOURCES} PROPERTIES LANGUAGE C)
----------------
h-vetinari wrote:
> mstorsjo wrote:
> > h-vetinari wrote:
> > > Shouldn't it be possible to remove that entire block (as it only fires for CMake <3.20)?
> > The intention was to do such cleanups in a later patch, as mentioned in the original commit message in https://reviews.llvm.org/D144509. (I guess it would be good to bring the original commit message along with the reland attempts too.)
> Well sure, but "workarounds" is a broad term. It makes sense to not try to clean up everything that newer CMake enables, but this PR does remove a lot of (all?) other lines with `CMAKE_VERSION VERSION_LESS <x>` where x<3.20. On top of that, since this line is already being changed (which is why I noticed in the first place), I'd suggest to just remove it. But I don't feel strongly about this, just thought I'd point it out.
Yes the intention is to do these cleanups later. The patch does more than I'd like already. However that is due to the original patch and followups being reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151344