LLNL / sundials

Official development repository for SUNDIALS - a SUite of Nonlinear and DIfferential/ALgebraic equation Solvers. Pull requests are welcome for bug fixes and minor changes.
https://computing.llnl.gov/projects/sundials
BSD 3-Clause "New" or "Revised" License
500 stars 121 forks source link

[BUG] CMake: `sundials_option` can unset variables used by downstream projects #538

Open ZedThree opened 2 months ago

ZedThree commented 2 months ago

Current Behavior:

We (optionally) build SUNDIALS as part of our project, with something like:

    include(FetchContent)
    FetchContent_Declare(
      sundials
      GIT_REPOSITORY https://github.com/LLNL/sundials
      GIT_TAG        v7.0.0
      )
    set(EXAMPLES_ENABLE_C OFF CACHE BOOL "" FORCE)
    set(EXAMPLES_INSTALL OFF CACHE BOOL "" FORCE)
    FetchContent_MakeAvailable(sundials)

Unfortunately, the change in #135 causes a bunch of CMake variables we (or our users) set for our project to be unset when not using the corresponding SUNDIALS features, for example: PETSC_DIR, PETSC_INCLUDES, CMAKE_CUDA_ARCHITECTURES.

This then breaks our downstream users because in our built CMake config file PETSC_DIR is no longer set and so find_dependency(PETSC) won't find PETSc.

There is a warning:

CMake Warning at build/_deps/sundials-src/cmake/macros/SundialsCMakeMacros.cmake:65 (message):
  ------------------------------------------------------------------------

  WARNING: The variable PETSC_DIR was set to my/path/to/petsc but not all of
  its dependencies (ENABLE_PETSC,) evaluate to TRUE.

  Unsetting PETSC_DIR

  ------------------------------------------------------------------------
Call Stack (most recent call first):
  build/_deps/sundials-src/cmake/macros/SundialsOption.cmake:85 (print_warning)
  build/_deps/sundials-src/cmake/SundialsTPLOptions.cmake:195 (sundials_option)
  build/_deps/sundials-src/CMakeLists.txt:168 (include)

but these variables should really be left alone entirely.

Expected Behavior:

Un-namespaced and non-SUNDIALS-specific CMake variables are left alone, especially paths for third-party libraries.

I think this is maybe just as simple as removing the DEPENDS_ON option from most sundials_option calls in cmake/SundialsTPLOptions.cmake?

Steps To Reproduce:

Here's a minimal example:

cmake_minimum_required(VERSION 3.12)

project(mvce LANGUAGES C)

set(SUNDIALS_VERSION "v7.0.0" CACHE STRING "SUNDIALS tag")
include(FetchContent)
FetchContent_Declare(
  sundials
  GIT_REPOSITORY https://github.com/LLNL/sundials
  GIT_TAG        ${SUNDIALS_VERSION}
)
set(EXAMPLES_ENABLE_C OFF CACHE BOOL "" FORCE)
set(EXAMPLES_INSTALL OFF CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(sundials)

message(STATUS "***** Value of PETSC_DIR: ${PETSC_DIR}")

And observe the value of PETSC_DIR in the output between:

$ cmake . -Bbuild_620 -DPETSC_DIR=/my/path/to/petsc -DSUNDIALS_VERSION=v6.2.0
$ cmake . -Bbuild_700 -DPETSC_DIR=/my/path/to/petsc -DSUNDIALS_VERSION=v7.0.0

Environment:

SUNDIALS 7.0+

Anything else:

balos1 commented 2 months ago

Thanks for the report. I think we will either need to remove DEPENDS_ON or only apply it to SUNDIALS name-spaced variables.