[PATCH] D107532: Get CPACK config variables from the cache to allow overriding via cache file

[Jon Reeves via Phabricator via llvm-commits]

jonathanreeves marked an inline comment as not done.
jonathanreeves added inline comments.


================
Comment at: llvm/CMakeLists.txt:234
+set(CPACK_PACKAGE_VENDOR "LLVM" CACHE STRING "")
+set(CPACK_PACKAGE_VERSION_MAJOR ${LLVM_VERSION_MAJOR} CACHE STRING "")
+set(CPACK_PACKAGE_VERSION_MINOR ${LLVM_VERSION_MINOR} CACHE STRING "")
----------------
ben.boeckel wrote:
> jonathanreeves wrote:
> > ben.boeckel wrote:
> > > 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.  
> > 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?
> 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
> ```
Yep that would cover the custom -> stock case. I'll put it together and resubmit something hopefully later today.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107532/new/

https://reviews.llvm.org/D107532