[CMake] Do not set CMP0116 explicitly to old #90385
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Last I checked, setting this to new causes issues with cmake consuming TableGens dep files. I don't fully remember the sympton but I think it'd either claim that a TableGen command is always out of date or it would not capture all dependencies and therefore not rereun a TableGen command if an included file changed. There was a PR that would address this here #72333. Is this still needed/the case? |
Looks like it's still needed. I didn't realize this was an issue. Thanks for bringing this up! I'll make sure that gets fixed before we merge this/merge this as part of the fix. |
boomanaiden154
force-pushed
the
drop-cmp0116-old
branch
from
ef2ba58 to
82e7883
Compare
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. This also removes two instances of a workaround related to the old behavior of CMP0116. Partially fixes llvm#83727.
boomanaiden154
force-pushed
the
drop-cmp0116-old
branch
from
82e7883 to
c4d53f3
Compare
|
Updated to fix the issues based on the patch in #72333. |
boomanaiden154
requested review from
chapuni,
zero9178 and
akirchhoff-modular
|
@llvm/pr-subscribers-mlir Author: Aiden Grossman (boomanaiden154) ChangesCMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. Partially fixes #83727. Full diff: https://github.com/llvm/llvm-project/pull/90385.diff 3 Files Affected:
diff --git a/cmake/Modules/CMakePolicy.cmake b/cmake/Modules/CMakePolicy.cmake
index f19dfd71657171..f6ecc40d8f1279 100644
--- a/cmake/Modules/CMakePolicy.cmake
+++ b/cmake/Modules/CMakePolicy.cmake
@@ -1,11 +1,5 @@
# CMake policy settings shared between LLVM projects
-# CMP0116: Ninja generators transform `DEPFILE`s from `add_custom_command()`
-# New in CMake 3.20. https://cmake.org/cmake/help/latest/policy/CMP0116.html
-if(POLICY CMP0116)
- cmake_policy(SET CMP0116 OLD)
-endif()
-
# MSVC debug information format flags are selected via
# CMAKE_MSVC_DEBUG_INFORMATION_FORMAT, instead of
# embedding flags in e.g. CMAKE_CXX_FLAGS_RELEASE.
diff --git a/llvm/cmake/modules/TableGen.cmake b/llvm/cmake/modules/TableGen.cmake
index ffcc718b47775f..d2ff1cfa553b39 100644
--- a/llvm/cmake/modules/TableGen.cmake
+++ b/llvm/cmake/modules/TableGen.cmake
@@ -23,18 +23,13 @@ function(tablegen project ofn)
# Use depfile instead of globbing arbitrary *.td(s) for Ninja.
if(CMAKE_GENERATOR MATCHES "Ninja")
- # Make output path relative to build.ninja, assuming located on
- # ${CMAKE_BINARY_DIR}.
# CMake emits build targets as relative paths but Ninja doesn't identify
- # absolute path (in *.d) as relative path (in build.ninja)
- # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
- file(RELATIVE_PATH ofn_rel
- ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
+ # absolute path (in *.d) as relative path (in build.ninja). Post CMP0116,
+ # CMake handles this discrepancy for us.
set(additional_cmdline
- -o ${ofn_rel}
- -d ${ofn_rel}.d
- WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
- DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
+ -o ${ofn}
+ -d ${ofn}.d
+ DEPFILE ${ofn}.d
)
set(local_tds)
set(global_tds)
diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index a3324705c525ce..bbe5976d7a3e76 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -47,15 +47,12 @@ function(_pdll_tablegen project ofn)
# Make output path relative to build.ninja, assuming located on
# ${CMAKE_BINARY_DIR}.
# CMake emits build targets as relative paths but Ninja doesn't identify
- # absolute path (in *.d) as relative path (in build.ninja)
- # Note that tblgen is executed on ${CMAKE_BINARY_DIR} as working directory.
- file(RELATIVE_PATH ofn_rel
- ${CMAKE_BINARY_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${ofn})
+ # absolute path (in *.d) as relative path (in build.ninja). Post CMP0116,
+ # CMake handles this discrepancy for us.
set(additional_cmdline
- -o ${ofn_rel}
- -d ${ofn_rel}.d
- WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
- DEPFILE ${CMAKE_CURRENT_BINARY_DIR}/${ofn}.d
+ -o ${ofn}
+ -d ${ofn}.d
+ DEPFILE ${ofn}.d
)
set(local_tds)
set(global_tds)
|
chapuni
reviewed
Oct 19, 2024
chapuni
reviewed
Oct 19, 2024
akirchhoff-modular
approved these changes
Oct 21, 2024
|
@chapuni Are you able to take another look at this when you get a chance? Thanks. |
chapuni
approved these changes
Oct 27, 2024
There was a problem hiding this comment.
@boomanaiden154 Sorry for delay. I was busy the last week.
I just wanted to reconfirm in my local tree, but it'd be fine now!
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/28/builds/8742 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/158/builds/8207 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/127/builds/3122 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/4304 Here is the relevant piece of the build log for the reference |
It's definitely broken in some cases. I've been able to reproduce the failure on Ubuntu 22.04 with CMake 3.22.1. The issue seems to be around Running the |
|
It looks like the issue was present all the way through v3.22 (including v3.22.6) and is gone by the time v3.23.0 lands. I'm going to bisect. |
|
It ended up bisecting down to e04a352cca523eba2ac0d60063a3799f5bb1c69e in CMake, which about makes sense given the commit description: I think we might just need to use fall back behavior for CMake <3.23.0. I'll push the patch for that and see how it goes. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2791 Here is the relevant piece of the build log for the reference |
jyli0116
pushed a commit
to jyli0116/llvm-project
that referenced
this pull request
Apr 28, 2025
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. Partially fixes llvm#83727.
jyli0116
pushed a commit
to jyli0116/llvm-project
that referenced
this pull request
Apr 28, 2025
This reverts commit ab405fb. This caused quite a few buildbot failures that need further investigation.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/49/builds/1388 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/98/builds/1333 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. Partially fixes llvm#83727.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
This reverts commit ab405fb. This caused quite a few buildbot failures that need further investigation.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. Partially fixes llvm#83727.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
This reverts commit ab405fb. This caused quite a few buildbot failures that need further investigation.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. Partially fixes llvm#83727.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
This reverts commit ab405fb. This caused quite a few buildbot failures that need further investigation.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now. Splitting off from removing the CMP0114 check just in case something breaks. Partially fixes llvm#83727.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
This reverts commit ab405fb. This caused quite a few buildbot failures that need further investigation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CMP0116 was originally set to old to get rid of warnings. However, this behavior is now set to new by default with the minimum CMake version that LLVM requires so does not produce any warnings, and setting it explicitly to old does produce a warning in newer CMake versions. Due to these reasons, remove this check for now.
Splitting off from removing the CMP0114 check just in case something breaks.
Partially fixes #83727.