Logic simplification

[gfardell]

Simplified logic slightly force a crash if find_package(CUDA) returns false if the user has requested DISABLE_CUDA=OFF

[KrisThielemans]

I'm afraid I don't agree with this change. DISABLE should mean "don't use it, even if you can". With the proposed change, it becomes "if not disabled, it is required". That's the wrong semantics...

However, we seem to have 2 (almost independent) variables: DISABLE_CUDA and USE_CUDA. That sounds like a bad idea to me, which is maybe why you had a problem.

What about

if (NOT DISABLE_CUDA)
  find_package(CUDA)
  set (USE_CUDA CUDA_FOUND)
else()
  set(USE_CUDA OFF)
endif()
if (USE_CUDA)
  message(...)
endif()`

(note that in the above USE_CUDA is not a cahced variable, hence not user-settable)
Of course, we wouldn't really need the variable USE_CUDA as it's identical to CUDA_FOUND, but I think the naming is more logical.

[paskino]

IMO, we should have ENABLE_CUDA rather than DISABLE_CUDA. The behaviour of the SuperBuild would then be predictable.

With the status before this change, one may have requested CUDA to be enabled, or thought they had by setting DISABLE_CUDA=OFF and they would not know what had happened. The SuperBuild may have found CUDA or not but it wouldn't tell them, or if it told them it'd be buried in several lines of output.

[KrisThielemans]

IMO, we should have ENABLE_CUDA rather than DISABLE_CUDA. The behaviour of the SuperBuild would then be predictable.

I disagree 😉 . Although both methods are plausible, I'm fairly sure we have chosen the DISABLE strategy for others things as well (STIR has of course plenty of them). In your proposed case, I think a better name would be REQUIRE_CUDA (as opposed to ENABLE).

For me, the advantage if DISABLE is that it always works: if you don't want CUDA, disable it. Otherwise, we use the normal CMake strategy of find_package(): It'll attempt to find it, and if it's present, we use it. It means we can always use the same installation scripts/CMake options, without having to know if CUDA is there or not. I find this essential.

With the status before this change, one may have requested CUDA to be enabled, or thought they had by setting DISABLE_CUDA=OFF

They did not request it to be disabled... They requested CMake to find it and use it if it's present.

Maybe my understanding if the English language is incorrect...

Of course, we could have both: DISABLE_CUDA (default to OFF) and REQUIRE_CUDA (defaulting to OFF), but if that would be clearer, I doubt it.

[paskino]

I see your point.

The thing I really do not like is the fact that the outcome of DISABLE_CUDA=OFF is not clear: the user does not know if CUDA is found and used or not.

[KrisThielemans]

The thing I really do not like is the fact that the outcome of DISABLE_CUDA=OFF is not clear: the user does not know if CUDA is found and used or not.

Can easily write it at the end together with some of the other diagnostics. Even then the SuperBuild doesn't know if a particular project used CUDA or not, so I'm not sure if it's very helpful, but I certainly don't mind.

We have a few other DISABLE options and no ENABLE nor REQUIRE, so I'm afraid I'm going to require that you keep DISABLE...

[gfardell]

I think there's a couple of different issues that complicate this.

This logic seems ambiguous, I don't know which order CMake evaluates the conditions in, and whether CUDA_FOUND is reset if cmake is reconfigured, but this may have been part of the issue:

SIRF-SuperBuild/SuperBuild.cmake

Line 52 in 1347466

if (NOT DISABLE_CUDA AND CUDA_FOUND)

This is maybe a separate issue but it took me a while to figure out what was going on and it's probably relevant to the variable naming and expected behaviour.

Initially I ran cmake with the default Disable_CUDA=OFF, and my system has CUDA so CUDA_FOUND would be True. Then I spotted the variable and set Disable_CUDA=ON and reconfigured. However at this point all the sub projects {proj}_USE_CUDA are not changed, and they are marked as advanced so searching for cuda in the ccmake cache browser didn't reveal them to me.

SIRF-SuperBuild/SuperBuild/External_Gadgetron.cmake

Lines 77 to 78 in 1347466

	option(${proj}_USE_CUDA "Enable ${proj} CUDA (if cuda libraries are present)" ${USE_CUDA})
	mark_as_advanced(${proj}_USE_CUDA)

If we stick with DISABLE_CUDA I think if it's 'ON' it should be a hard disable that controls the subprojects' behaviour and force all builds to not use CUDA. If it's OFF then the subprojects could be toggled independently, but these should be easier to find and set.

[KrisThielemans]

This logic seems ambiguous,

I agree that that logic was not good. Moreover, both DISABLE_CUDA and USE_CUDA are cached, which can only let to very confusing results, hence my suggestion. I believe it would remove that problem.

Then I spotted the variable and set Disable_CUDA=ON and reconfigured. However at this point all the sub projects {proj}_USE_CUDA are not changed,

yes. This will trip up many, but it is an independent problem of the "logic". We have the same problem with DEVEL_BUILD=ON. It doesn't really work once the cache has been established. I agree that this is undesirable.

If we stick with DISABLE_CUDA I think if it's 'ON' it should be a hard disable that controls the subprojects' behaviour and force all builds to not use CUDA. If it's OFF then the subprojects could be toggled independently, but these should be easier to find and set.

I agree that this suggestion is another (semantically correct) interpretation of DISABLE. and might make sense. The trouble is that I don't know how to do that. CMake allows the developer to override a cached value

(set VAR VALUE CACHE <type> <docstring> FORCE)

However, the SuperBuild cannot do that for the "external" projects, as we have no control over their CMake files, and hence no immediate control over their cached variables. It's possible this can be done with ExternalProject_Add , but I don't know how..

However, if you use that strategy, would you want DEVEL_BUILD=OFF force to use the "default" tags, with "DEVEL_BUILD=ON" let the user select them? I don't think that works, as in most cases, you 'd want them to be set to the "devel" tags...

Final remark, we have far too many variables now already. Adding even more (by making them not-advanced) could easily be non-productive...

Summary: I agree it is not ideal, but I don't know how to do it better...

[paskino]

Shall we close this?

[KrisThielemans]

I'm fine with closing this, but I would remove USE_CUDA from the cache to avoid having 2 cached variables, as I stated in #511 (review). Again, to me this is quite clear. We default to finding and then using CUDA. If the user DISABLE it, we try to disable it for any project where we can (for many we have no control, but for SIRF and STIR we do). We can state that in the doc.

That could be a different PR or this one.

[KrisThielemans]

outdated and not agreed on the strategy