Fix inconsistency between the building doc and CMakeLists.txt

[yong-wang]

• update the building doc to make it consistent with CMakeLists.txt
• fix typo in CMakeLists.txt

[rapids-bot]

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

[vyasr]

Thanks for the contribution! There's one default here that I'm not quite sure about, asked for some extra feedback.

[vyasr]

I think we probably want to leave this as defaulting to ${BUILD_TESTS}, otherwise tests will be extremely slow to compile. Actually, perhaps this really should default to ON if we're building tests or benchmarks, since the same is true in either case. @cjnolet WDYT?

I'm not sure what the best way is to document this default; it may just have to be written in words in build.md rather than relying on a simple ON/OFF entry in the table.

[cjnolet]

Yeah we definitely want to keep this defaulting to build tests (and I agree it probably should default BUILD_TESTS | BUILD_BENCH. We rely on this to make sure really expensive specializations are compiled once (in libraft-distance/libraft-nn) and reused.

As for the docs, I think we could update the description in this table to maybe mention that it defaults to ON because BUILD_TESTS defaults to on.

[yong-wang]

How about filling the "Default Value" column with ${BUILD_TESTS} in the table? Does it look reasonable?

[yong-wang]

I searched the web but didn't find any easy way to evaluate things like BUILD_TESTS OR BUILD_BENCH inside an option().

I'm not familiar with CMake. Any suggestion?

[cjnolet]

Cc @robertmaynard

[vyasr]

CMake doesn't support that kind of expression evaluation. AFAIK the canonical solution would be

set(RAFT_COMPILE_LIBRARIES_DEFAULT OFF)
if(${BUILD_TESTS} OR ${BUILD_BENCH})
  set(RAFT_COMPILE_LIBRARIES_DEFAULT OFF)
endif()
option(RAFT_COMPILE_LIBRARIES <description> ${RAFT_COMPILE_LIBRARIES_DEFAULT})

[vyasr]

Regarding the table, I think no matter what you put in the column you'll probably need to explain it a bit in words.

[robertmaynard]

This sounds exactly like the use case for cmake_dependent_option which allows for complex initialization of cache variables.

include(CMakeDependentOption)
cmake_dependent_option(RAFT_COMPILE_LIBRARIES "Enable building raft shared library instantiations" ON "BUILD_TESTS OR BUILD_BENCH" OFF )

[yong-wang]

Thanks for the suggestions.
I tried cmake_dependent_option, but when both BUILD_TESTS and BUILD_BENCH are OFF, could not set RAFT_COMPILE_LIBRARIES to ON from the command line. According to CMake doc, RAFT_COMPILE_LIBRARIES is hidden from the user in this case.
So I used Vyas' way to set the default value. Also updated the table.

[vyasr]

I was not aware of cmake_dependent_option, that's cool. It does seem from the docs like the way the option may be hidden is incompatible with our needs in this case, though.

[vyasr]

ok to test

[vyasr]

/ok to test

[vyasr]

This solution looks fine to me. If @robertmaynard would prefer using cmake_dependent_option and has a way to get that working, that works for me too. I don't have a strong preference. Thanks for the contribution @yong-wang!

[cjnolet]

/merge