[llvm] r242102 - [CMake] We shouldn't be storing values in the cache unless they actually need CMake cache behavior.

[Daniel Sanders]

No worries. I thought it was probably just the words being swapped but I wanted to make sure I wasn't misunderstanding something.

From: Chris Bieneman [mailto:beanz at apple.com]
Sent: 17 July 2015 19:14
To: Daniel Sanders
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm] r242102 - [CMake] We shouldn't be storing values in the cache unless they actually need CMake cache behavior.


On Jul 17, 2015, at 2:03 AM, Daniel Sanders <Daniel.Sanders at imgtec.com<mailto:Daniel.Sanders at imgtec.com>> wrote:

-----Original Message-----
From: Chris Bieneman [mailto:beanz at apple.com]
Sent: 16 July 2015 17:54
To: Daniel Sanders
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: Re: [llvm] r242102 - [CMake] We shouldn't be storing values in the
cache unless they actually need CMake cache behavior.



On Jul 16, 2015, at 8:35 AM, Daniel Sanders <Daniel.Sanders at imgtec.com<mailto:Daniel.Sanders at imgtec.com>>
wrote:


Hi Chris,

This isn't a problem but I thought I should mention it. One side effect of this
change is that these variables are no longer present in ccmake's interface
unless you have already set them manually. I'm in the habit of running cmake
to get an initial config then ccmake to edit these variables so I was confused
by their sudden disappearance :-).



There is also a temporary change to remove non-default values from the
cache if they are already present. This should have the impact of cleaning
out

unncecissary values from the caches on the buildbots and people's local
build

directories.

Did you mean "non-default" there?

That is basically what I meant. If they are non-default then caching them has
no value other than populating the UIs and causing issues because CMake
doesn’t implement any form of cache invalidation.

I generally thing these options are advanced user options that are not
commonly used, so I really want to avoid having them in the cache unless
they are specified by the user explicitly.

-Chris

My reading of the patch is that it removes default-valued variables from the cache and continues to cache non-default values. This makes sense to me since we can reconstruct default values (by using the default) but not non-default values (we'd need to ask the user for it again). However, it's the opposite way around to my reading of the commit message.

That’s exactly what it does. I am apparently suffering from severe befuddling of words.

The point of this patch is to remove values from the cache for default, in-tree paths to tools. After running CMake with this patch the only LLVM_EXTERNAL_*_SOURCE_DIR values that will remain in the cache are non-default values.

I needed to land this change so that I could re-land r242059. As per Takumi’s feedback that patch changes the behavior of LLVM_EXTERNAL_*_SOURCE_DIR slightly so that it is ignored if the default in-tree path exists. The change in behavior and how the value is used in CMake caused failures across the board because the default values were all previously cached.

Most of this patch (specifically the unset bit) I’m going to remove today when I re-land r242059 because the caches on all the builders (and most developer machines) should be sufficiently cleaned up.

I’m sorry for the poor wording in my commit message and my email responses.

-Chris



-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu<mailto:llvm-commits-bounces at cs.uiuc.edu> [mailto:llvm-commits-
bounces at cs.uiuc.edu<mailto:bounces at cs.uiuc.edu>] On Behalf Of Chris Bieneman
Sent: 14 July 2015 02:18
To: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: [llvm] r242102 - [CMake] We shouldn't be storing values in the
cache

unless they actually need CMake cache behavior.

Author: cbieneman
Date: Mon Jul 13 20:17:43 2015
New Revision: 242102

URL: http://llvm.org/viewvc/llvm-project?rev=242102&view=rev
Log:
[CMake] We shouldn't be storing values in the cache unless they actually
need CMake cache behavior.

add_llvm_external_project puts
LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR into the cache even if it is
just the in-tree default path. This causes all sorts of oddness, and makes it
so

that I can't change the behavior of this variable.

This patch never puts LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR
into

the cache. It will only end up in the cache if it is specified on the command
line, which is the correct behavior.

There is also a temporary change to remove non-default values from the
cache if they are already present. This should have the impact of cleaning
out

unncecissary values from the caches on the buildbots and people's local
build

directories. This part of the change is marked with a TODO and can be
removed in a few days.

Modified:
  llvm/trunk/cmake/modules/AddLLVM.cmake

Modified: llvm/trunk/cmake/modules/AddLLVM.cmake
URL: http://llvm.org/viewvc/llvm-
project/llvm/trunk/cmake/modules/AddLLVM.cmake?rev=242102&r1=24210

1&r2=242102&view=diff
==========================================================

====================
--- llvm/trunk/cmake/modules/AddLLVM.cmake (original)
+++ llvm/trunk/cmake/modules/AddLLVM.cmake Mon Jul 13 20:17:43
2015

@@ -689,10 +689,15 @@ macro(add_llvm_external_project name)
 list(APPEND LLVM_IMPLICIT_PROJECT_IGNORE
"${CMAKE_CURRENT_SOURCE_DIR}/${add_llvm_external_dir}")
 string(REPLACE "-" "_" nameUNDERSCORE ${name})
 string(TOUPPER ${nameUNDERSCORE} nameUPPER)
-  set(LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR
"${CMAKE_CURRENT_SOURCE_DIR}/${add_llvm_external_dir}"
-      CACHE PATH "Path to ${name} source directory")
-  if (NOT ${LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR} STREQUAL ""
-      AND EXISTS
${LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR}/CMakeLists.txt)
+  #TODO: Remove this check in a few days once it has circulated through
+  # buildbots and people's checkouts (cbieneman - July 14, 2015)
+  if(${LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR} STREQUAL
"${CMAKE_CURRENT_SOURCE_DIR}/${add_llvm_external_dir}")
+    unset(LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR CACHE)
+  endif()
+  if(NOT LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR)
+    set(LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR
"${CMAKE_CURRENT_SOURCE_DIR}/${add_llvm_external_dir}")
+  endif()
+  if (EXISTS
${LLVM_EXTERNAL_${nameUPPER}_SOURCE_DIR}/CMakeLists.txt)
   option(LLVM_EXTERNAL_${nameUPPER}_BUILD
          "Whether to build ${name} as part of LLVM" ON)
   if (LLVM_EXTERNAL_${nameUPPER}_BUILD)


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150720/e037f04e/attachment.html>