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

Chris Bieneman beanz at apple.com
Fri Jul 17 11:13:56 PDT 2015


> On Jul 17, 2015, at 2:03 AM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:
> 
>> -----Original Message-----
>> From: Chris Bieneman [mailto:beanz at apple.com <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>
>> 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] On Behalf Of Chris Bieneman
>>>> Sent: 14 July 2015 02:18
>>>> To: 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
>>>> 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/20150717/2091848b/attachment.html>


More information about the llvm-commits mailing list