This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Bumps minimum version to 3.20.0.
ClosedPublic

Authored by Mordante on Feb 21 2023, 11:15 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Mordante created this revision.Feb 21 2023, 11:15 AM
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, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante published this revision for review.Feb 21 2023, 11:20 AM
Mordante added reviewers: Restricted Project, Restricted Project.
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptFeb 21 2023, 11:20 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
sivachandra accepted this revision.Feb 21 2023, 11:23 AM
sivachandra added a subscriber: sivachandra.

OK for libc.

philnik accepted this revision.Feb 21 2023, 11:26 AM
phosek accepted this revision.Feb 21 2023, 11:35 AM

LGTM

This revision is now accepted and ready to land.Feb 21 2023, 11:35 AM
thieta accepted this revision as: thieta.Feb 21 2023, 12:47 PM
MaskRay accepted this revision as: MaskRay.Feb 21 2023, 12:59 PM
ChuanqiXu accepted this revision.Feb 21 2023, 5:53 PM
to268 accepted this revision.Feb 22 2023, 3:45 AM
tschuett accepted this revision.Feb 22 2023, 3:48 AM
zibi accepted this revision.Feb 22 2023, 5:56 AM
Mordante updated this revision to Diff 499569.Feb 22 2023, 10:06 AM

Try to fix AIX CI.

mehdi_amini accepted this revision.Feb 22 2023, 10:25 AM
Mordante updated this revision to Diff 500547.Feb 26 2023, 4:47 AM

Rebased to test CI.

This revision was landed with ongoing or failed builds.Mar 4 2023, 3:41 AM
This revision was automatically updated to reflect the committed changes.

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

See D150688 - I believe that might fix the issue you're seeing, as that one mentions compiler-rt.

See D150688 - I believe that might fix the issue you're seeing, as that one mentions compiler-rt.

Unfortunately, it doesn't.

FWIW, the errors looks like:

lld-link: error: undefined symbol: __declspec(dllimport) _getpid
>>> referenced by clang_rt.profile-i386.lib(InstrProfilingFile.c.obj):(_getCurFilename)
>>> referenced by clang_rt.profile-i386.lib(InstrProfilingFile.c.obj):(_parseAndSetFilename)
>>> referenced by oldnames.lib(getpid.obi)

etc.

It's related, though, because now that I look at my build logs, the difference between when it works and when it doesn't is /MT vs -MD when compiler-rt is compiled. The main peculiarity on our end, though, is that the Windows compiler-rt is cross-compiled.

So the problem is that CMakePolicy.cmake is not included in compiler-rt/CMakeLists.txt.

dyung added a subscriber: dyung.May 17 2023, 1:44 AM

I'm not really sure where else to post this, but the pre-merge linux bot still seems to be running 3.18.4.

https://buildkite.com/llvm-project/premerge-checks/builds/152678#018828d2-1837-4fc2-baec-fca595969219

CMake Error at CMakeLists.txt:3 (cmake_minimum_required):
  CMake 3.20.0 or higher is required.  You are running version 3.18.4
hans added a subscriber: hans.May 17 2023, 1:54 AM

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

It is indeed unfortunate that this seems to come with such a fundamental behavior change. (we already have https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for the issue you're seeing?

Since the breakages are piling up, perhaps it's time to revert this.

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

It is indeed unfortunate that this seems to come with such a fundamental behavior change. (we already have https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for the issue you're seeing?

Since the breakages are piling up, perhaps it's time to revert this.

Unfortunately, much of this is hard to sort out without actually trying to patch forward on main, since many of these issues don't show up easily in e.g. premerge testing or similar (the patch has been tried many times for months before in that form), so I'd appreciate if we'd give it a little more time to actually try to sort out the issues...

hans added a comment.May 17 2023, 7:40 AM

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

It is indeed unfortunate that this seems to come with such a fundamental behavior change. (we already have https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for the issue you're seeing?

Since the breakages are piling up, perhaps it's time to revert this.

Unfortunately, much of this is hard to sort out without actually trying to patch forward on main, since many of these issues don't show up easily in e.g. premerge testing or similar (the patch has been tried many times for months before in that form), so I'd appreciate if we'd give it a little more time to actually try to sort out the issues...

But now we have identified issues and there are reproducers available (e.g. on https://github.com/llvm/llvm-project/issues/62719). Keeping it in main is just disruptive and risks making reverting harder (it's already not a clean revert). I think the policy of reverting back to green still applies.

thakis added a subscriber: thakis.

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

(See also discussion over in D150688)

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

(See also discussion over in D150688)

I'm not happy that the patch needs to be reverted again.

It has taken me a lot of time to contact all buildbots maintainers to get all buildbots updated to the minimal CMake requirement.
Now that they are updated it turned out that one of the two last updated bots has an issue with this patch and that has been fixed. But now it seems to break Chromium. I don't have access to Windows so I don't know how I can test patches.

Do you have a suggestion how we can move this patch forward?

https://github.com/llvm/llvm-project/issues/62719 is independent of chromium and others have reported problems above too, from what I understand. Is that not accurate?

I'm not really sure where else to post this, but the pre-merge linux bot still seems to be running 3.18.4.

https://buildkite.com/llvm-project/premerge-checks/builds/152678#018828d2-1837-4fc2-baec-fca595969219

CMake Error at CMakeLists.txt:3 (cmake_minimum_required):
  CMake 3.20.0 or higher is required.  You are running version 3.18.4

Thanks, I've notified the owner of the build bot.

https://github.com/llvm/llvm-project/issues/62719 is independent of chromium and others have reported problems above too, from what I understand. Is that not accurate?

That bug report originally was independent of Chromium, that part was fixed by D150688. This wasn't noticed earlier since that happened on one of the buildbots that took the longest to update. For the earlier failed landings there was never a notification this patch caused issues on Chromium, so I'm quite taken by surprise.

The other issues I see are build errors on Chromium, unless I missed something.

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

It is indeed unfortunate that this seems to come with such a fundamental behavior change. (we already have https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for the issue you're seeing?

The Chromium issue that is discussed there is essentially the same thing I was seeing.

hans added a comment.May 19 2023, 6:06 AM

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

(See also discussion over in D150688)

I'm not happy that the patch needs to be reverted again.

It has taken me a lot of time to contact all buildbots maintainers to get all buildbots updated to the minimal CMake requirement.

I sympathize with this, but I still believe reverting in these situations is the right thing to do. It reduces disruption for everyone who needs their builds to keep working, while allowing the failures to be investigated without the pressure of knowing that head is currently broken. That this patch has been hard to land is in the nature of the change itself.

Now that they are updated it turned out that one of the two last updated bots has an issue with this patch and that has been fixed. But now it seems to break Chromium. I don't have access to Windows so I don't know how I can test patches.

I'm happy to test patches on my Windows machine.

Do you have a suggestion how we can move this patch forward?

IIRC, D150688 + the diff in https://github.com/llvm/llvm-project/issues/62719#issuecomment-1552903385 + upgrading the pre-merge linux bot should take care of all known issues.

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

(See also discussion over in D150688)

I'm not happy that the patch needs to be reverted again.

It has taken me a lot of time to contact all buildbots maintainers to get all buildbots updated to the minimal CMake requirement.

I sympathize with this, but I still believe reverting in these situations is the right thing to do. It reduces disruption for everyone who needs their builds to keep working, while allowing the failures to be investigated without the pressure of knowing that head is currently broken. That this patch has been hard to land is in the nature of the change itself.

I'm caught quite off-guard that it seems the LLVM buildbots don't cover our Windows support that well. It would also be great to know how we can improve the process to update dependency versions. The current way does not really encourage me to propose LLVM wide updates again.

Now that they are updated it turned out that one of the two last updated bots has an issue with this patch and that has been fixed. But now it seems to break Chromium. I don't have access to Windows so I don't know how I can test patches.

I'm happy to test patches on my Windows machine.

Thanks!

Do you have a suggestion how we can move this patch forward?

IIRC, D150688 + the diff in https://github.com/llvm/llvm-project/issues/62719#issuecomment-1552903385 + upgrading the pre-merge linux bot should take care of all known issues.

Would it make sense to put all these patches in one new review and then test that on Chromium and ask @glandium to test that too. Then we know whether it solves the issues. Do you want me to make a patch or do you want to do it?

hans added a comment.May 22 2023, 4:33 AM

Do you have a suggestion how we can move this patch forward?

IIRC, D150688 + the diff in https://github.com/llvm/llvm-project/issues/62719#issuecomment-1552903385 + upgrading the pre-merge linux bot should take care of all known issues.

Would it make sense to put all these patches in one new review and then test that on Chromium and ask @glandium to test that too. Then we know whether it solves the issues. Do you want me to make a patch or do you want to do it?

I tested https://github.com/llvm/llvm-project/issues/62719#issuecomment-1552903385 on Chromium already, and that looks good.

I've created D151344 @glandium @hans @thakis I really would appreciate when you can test the patch locally to avoid another revert round.