This is an archive of the discontinued LLVM Phabricator instance.

Reland "[CMake] Bumps minimum version to 3.20.0.
ClosedPublic

Authored by Mordante on May 24 2023, 9:17 AM.

Details

Reviewers
glandium
hans
thakis
bollu
ldionne
rafauler
Amir
maksfb
NoQ
jdoerfert
Mordante
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGcbaa3597aaf6: Reland "[CMake] Bumps minimum version to 3.20.0.
Summary

This reverts commit d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6.

Adds the patch by @hans from
https://github.com/llvm/llvm-project/issues/62719
This patch fixes the Windows build.

d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 reverted the reviews

D144509 [CMake] Bumps minimum version to 3.20.0.

This partly undoes D137724.

This change has been discussed on discourse
https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-cmake-version/66193

Note this does not remove work-arounds for older CMake versions, that
will be done in followup patches.

D150532 [OpenMP] Compile assembly files as ASM, not C

Since CMake 3.20, CMake explicitly passes "-x c" (or equivalent)
when compiling a file which has been set as having the language
C. This behaviour change only takes place if "cmake_minimum_required"
is set to 3.20 or newer, or if the policy CMP0119 is set to new.

Attempting to compile assembly files with "-x c" fails, however
this is workarounded in many cases, as OpenMP overrides this with
"-x assembler-with-cpp", however this is only added for non-Windows
targets.

Thus, after increasing cmake_minimum_required to 3.20, this breaks
compiling the GNU assembly for Windows targets; the GNU assembly is
used for ARM and AArch64 Windows targets when building with Clang.
This patch unbreaks that.

D150688 [cmake] Set CMP0091 to fix Windows builds after the cmake_minimum_required bump

The build uses other mechanism to select the runtime.

Fixes #62719

Diff Detail

Event Timeline

Mordante created this revision.May 24 2023, 9:17 AM
Herald added a reviewer: ldionne. · View Herald Transcript
Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Mordante requested review of this revision.May 24 2023, 9:17 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thank you for making another try for the treewide change (which is admittedly very painful and not many people do such work).
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.

I haven't tested this patch, but it looks like it contains everything that unbreaks our builds.

hans added a comment.May 25 2023, 2:28 AM

This looks right to me. (I'm out of office at the moment, but this looks like what I tested in https://github.com/llvm/llvm-project/issues/62719 so it should work.)

h-vetinari added inline comments.
libunwind/src/CMakeLists.txt
28–35

Shouldn't it be possible to remove that entire block (as it only fires for CMake <3.20)?

mstorsjo added inline comments.May 25 2023, 9:09 PM
libunwind/src/CMakeLists.txt
28–35

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

h-vetinari added inline comments.May 25 2023, 10:04 PM
libunwind/src/CMakeLists.txt
28–35

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.

Mordante accepted this revision.May 27 2023, 3:50 AM
Mordante marked 3 inline comments as done.

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.

libunwind/src/CMakeLists.txt
28–35

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.

This revision was not accepted when it landed; it landed in state Needs Review.May 27 2023, 3:51 AM
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.