Get CPACK config variables from the cache to allow overriding via cache file

[jonathanreeves]

Summary

For custom-built toolchains, it's extremely useful to be able to override the CPack packaging instructions, especially on Windows.

Elsewhere in the build it's already possible to override the Vendor that appears in the binary. It would be nice to be able to carry this through to installers.

This patch does introduce a CMake warning about the CPACK_NSIS_COMPRESSOR variable having a newline character. Note that no changes have been made to the variable values themselves here, just the caching.

[ben.boeckel]

llvm/CMakeLists.txt

232–282

Please either add docstrings for each of these or use mark_as_advanced to hide them from regular developer cache editing activities. Preferably both.

234

There is also the "sticky cache" issue here. If I configure by build tree first with 13.0.0, this gets set to 13. If I then continue to use this build tree through to 14.0.0…it is still 13. I suggest, instead doing something like this:

set(CPACK_PACKAGE_VERSION_MAJOR "<DEFAULT>" CACHE STRING "")
if (CPACK_PACKAGE_VERSION_MAJOR STREQUAL "<DEFAULT>")
  set(CPACK_PACKAGE_VERSION_MAJOR "${LLVM_VERSION_MAJOR}")
elseif (NOT CPACK_PACKAGE_VERSION_MAJOR STREQUAL LLVM_VERSION_MAJOR)
  # This condition is optional, but can help when manual settings are stale.
  message(AUTHOR_WARNING
    "Manually set `CPACK_PACKAGE_VERSION_MAJOR` does not match the "
    "detected major version of `${LLVM_VERSION_MAJOR}`; your cache may "
    "be out-of-date."
endif ()

so that it actually acts like a default rather than an initial value that "sticks around" in cases where "no one" would want it to.

[jonathanreeves]

llvm/CMakeLists.txt

232–282

Makes sense, will do.

234

Good point. I wonder if it actually makes sense to just do something like this for all of them:

if(NOT DEFINED CPACK_PACKAGE_VERSION_MAJOR)
  set(CPACK_PACKAGE_VERSION_MAJOR "${LLVM_VERSION_MAJOR}")
endif()

Basically if someone wants to override the CPack variables, they will do it via cache. If no overrides exist, the build will take the regularly scoped variables, which will be uncached. The only case that's potentially not covered here is one where you're trying to go from a custom build (which will have these variables in cache) to a stock one. In this case the variables will be sticky. I think this is a pretty pathological case though, probably one that can be safely ignored.

The other advantage to doing this is that we would no longer need the mark_as_advanced. The stock build just wouldn't use the cache.

This is probably a simpler solution overall though. Thoughts on this?

[ben.boeckel]

llvm/CMakeLists.txt

234

Something like this could work then:

cmake
if (LLVM_RESET_PACKAGING_SETTINGS)
  foreach (package_var IN ITEMS …)
    unset("${package_var}" CACHE)
  endforeach ()
  unset(LLVM_RESET_PACKAGING_SETTINGS CACHE) # make it a simple toggle
endif ()

# if (NOT DEFINED) blocks

[jonathanreeves]

llvm/CMakeLists.txt

234

Yep that would cover the custom -> stock case. I'll put it together and resubmit something hopefully later today.

[jonathanreeves]

PR feedback: ability to clear cached packaging settings, and making version variables non-cached by default

[jonathanreeves]

Sorry for the delay on this, I was out of town until yesterday, just finished up testing the changes. Note that I do think several of these variables deserve to be cached and "sticky", I just protected the version settings from the unintentional caching problem. Let me know what you think of this. Trying to avoid a bunch of conditionals on things that are unlikely or undesirable to frequently change.

[jonathanreeves]

I realize I was a little slow responding to this, but it's been idle for a little while now. @ben.boeckel do you have an suggestions for other reviewers I should add? It would still be nice to get this into a release. Thanks!

[ben.boeckel]

Sorry for the delay. The changes look good to me.

[jonathanreeves]

Thanks @ben.boeckel, much appreciated! Forgive the noob question, but what are the next steps for getting it merged? I didn't see much in the LLVM documentation. Is there anything I need to do to actually see this all the way through?

[ben.boeckel]

In D107532#2963225, @jonathanreeves wrote:

what are the next steps for getting it merged? I didn't see much in the LLVM documentation. Is there anything I need to do to actually see this all the way through?

I can't say I know that. I don't have powers to merge here and am just another community member (that also happens to be a CMake developer).

llvm/CMakeLists.txt

 set(PACKAGE_NAME LLVM)
 set(PACKAGE_STRING "${PACKAGE_NAME} ${PACKAGE_VERSION}")
 set(PACKAGE_BUGREPORT "https://bugs.llvm.org/")
 
 set(BUG_REPORT_URL "${PACKAGE_BUGREPORT}" CACHE STRING
   "Default URL where bug reports are to be submitted.")
 
 # Configure CPack.
-set(CPACK_PACKAGE_INSTALL_DIRECTORY "LLVM")
-set(CPACK_PACKAGE_VENDOR "LLVM")
+
+# optional toggle to clear any cached packaging settings:
+if(LLVM_RESET_PACKAGING_SETTINGS)
+  foreach(package_var IN ITEMS
+            CPACK_PACKAGE_INSTALL_DIRECTORY
+            CPACK_PACKAGE_VENDOR
+            CPACK_PACKAGE_VERSION_MAJOR
+            CPACK_PACKAGE_VERSION_MINOR
+            CPACK_PACKAGE_VERSION_PATCH
+            CPACK_PACKAGE_VERSION
+            CPACK_RESOURCE_FILE_LICENSE
+            CPACK_NSIS_COMPRESSOR
+            CPACK_PACKAGE_INSTALL_REGISTRY_KEY
+            CPACK_PACKAGE_ICON
+            CPACK_NSIS_MUI_ICON
+            CPACK_NSIS_MUI_UNIICON
+            CPACK_NSIS_MODIFY_PATH
+            CPACK_NSIS_ENABLE_UNINSTALL_BEFORE_INSTALL
+            CPACK_NSIS_INSTALL_ROOT)
+    unset("${package_var}" CACHE)
+  endforeach ()
+  unset(LLVM_RESET_PACKAGING_SETTINGS CACHE)
+endif()
+
+set(CPACK_PACKAGE_INSTALL_DIRECTORY "LLVM" CACHE STRING "")
+mark_as_advanced(CPACK_PACKAGE_INSTALL_DIRECTORY)
+set(CPACK_PACKAGE_VENDOR "LLVM" CACHE STRING "")
+mark_as_advanced(CPACK_PACKAGE_VENDOR)
+
+# note: we want the ability to override version variables via the cache, but if
+# we're using the LLVM defaults, we don't want them to "stick" from build to
+# build, thus we don't write them to the cache. Cached versions of these
+# variables can always be cleared using LLVM_RESET_PACKAGING_SETTINGS (see above)
+if(NOT DEFINED CPACK_PACKAGE_VERSION_MAJOR)
   set(CPACK_PACKAGE_VERSION_MAJOR ${LLVM_VERSION_MAJOR})
+endif()
+if(NOT DEFINED CPACK_PACKAGE_VERSION_MINOR)
   set(CPACK_PACKAGE_VERSION_MINOR ${LLVM_VERSION_MINOR})
+endif()
+if(NOT DEFINED CPACK_PACKAGE_VERSION_PATCH)
   set(CPACK_PACKAGE_VERSION_PATCH ${LLVM_VERSION_PATCH})
+endif()
+if(NOT DEFINED CPACK_PACKAGE_VERSION)
   set(CPACK_PACKAGE_VERSION ${PACKAGE_VERSION})
-set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/LICENSE.TXT")
-set(CPACK_NSIS_COMPRESSOR "/SOLID lzma \r\n SetCompressorDictSize 32")
+endif()
+
+set(CPACK_RESOURCE_FILE_LICENSE "${CMAKE_CURRENT_SOURCE_DIR}/LICENSE.TXT" CACHE STRING "")
+mark_as_advanced(CPACK_RESOURCE_FILE_LICENSE)
+set(CPACK_NSIS_COMPRESSOR "/SOLID lzma \r\n SetCompressorDictSize 32" CACHE STRING "")
+mark_as_advanced(CPACK_NSIS_COMPRESSOR)
+
 if(WIN32 AND NOT UNIX)
-  set(CPACK_PACKAGE_INSTALL_REGISTRY_KEY "LLVM")
-  set(CPACK_PACKAGE_ICON "${CMAKE_CURRENT_SOURCE_DIR}\\\\cmake\\\\nsis_logo.bmp")
-  set(CPACK_NSIS_MUI_ICON "${CMAKE_CURRENT_SOURCE_DIR}\\\\cmake\\\\nsis_icon.ico")
-  set(CPACK_NSIS_MUI_UNIICON "${CMAKE_CURRENT_SOURCE_DIR}\\\\cmake\\\\nsis_icon.ico")
-  set(CPACK_NSIS_MODIFY_PATH "ON")
-  set(CPACK_NSIS_ENABLE_UNINSTALL_BEFORE_INSTALL "ON")
+  set(CPACK_PACKAGE_INSTALL_REGISTRY_KEY "LLVM" CACHE STRING "")
+  mark_as_advanced(CPACK_PACKAGE_INSTALL_REGISTRY_KEY)
+  set(CPACK_PACKAGE_ICON "${CMAKE_CURRENT_SOURCE_DIR}\\\\cmake\\\\nsis_logo.bmp" CACHE STRING "")
+  mark_as_advanced(CPACK_PACKAGE_ICON)
+  set(CPACK_NSIS_MUI_ICON "${CMAKE_CURRENT_SOURCE_DIR}\\\\cmake\\\\nsis_icon.ico" CACHE STRING "")
+  mark_as_advanced(CPACK_NSIS_MUI_ICON)
+  set(CPACK_NSIS_MUI_UNIICON "${CMAKE_CURRENT_SOURCE_DIR}\\\\cmake\\\\nsis_icon.ico" CACHE STRING "")
+  mark_as_advanced(CPACK_NSIS_MUI_UNIICON)
+  set(CPACK_NSIS_MODIFY_PATH "ON" CACHE STRING "")
+  mark_as_advanced(CPACK_NSIS_MODIFY_PATH)
+  set(CPACK_NSIS_ENABLE_UNINSTALL_BEFORE_INSTALL "ON" CACHE STRING "")
+  mark_as_advanced(CPACK_NSIS_ENABLE_UNINSTALL_BEFORE_INSTALL)
   if( CMAKE_CL_64 )
-    set(CPACK_NSIS_INSTALL_ROOT "$PROGRAMFILES64")
+    set(CPACK_NSIS_INSTALL_ROOT "$PROGRAMFILES64" CACHE STRING "")
+    mark_as_advanced(CPACK_NSIS_INSTALL_ROOT)
   endif()
 endif()
 include(CPack)
 
 # Sanity check our source directory to make sure that we are not trying to
 # generate an in-source build (unless on MSVC_IDE, where it is ok), and to make
 # sure that we don't have any stray generated files lying around in the tree
 # (which would end up getting picked up by header search, instead of the correct