Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ZedThree opened this issue Jul 8, 2024 · 1 comment

Comments

@ZedThree
Copy link

ZedThree commented Jul 8, 2024

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
Copy link
Member

balos1 commented Jul 8, 2024

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

ZedThree added a commit to ZedThree/sundials that referenced this issue Nov 6, 2024
Fixes LLNL#538

Non-`SUNDIALS` namespaced variables may be used by other
projects so don't `unset` them
ZedThree added a commit to ZedThree/sundials that referenced this issue Nov 6, 2024
Fixes LLNL#538

Non-`SUNDIALS` namespaced variables may be used by other
projects so don't `unset` them

Signed-off-by: Peter Hill <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants