Reduce unnecessary nesting in "CMakeLists.txt"

[SunBlack]

Reduce unnecessary nesting in CMakeLists.txt in case global deactivation parameter exists.

Example:
Old:

if(build)
  // Whole remaining CMake code
endif() //endif at end of file

new:

if(NOT build)
  return()
endif()
// Whole remaining CMake code

PS @whoever is reviewing this: I hope you are not using GitHub diff view to check this PR. A diff view where you can ignore whitespace changes is much better here ;)

[taketwo]

I've restarted the Ubuntu job two times, but it always failed. This does not look like a compilation problem, rather like the agent is running out of memory and loses communication. Question is, what is it in this pull request that affected the build?

[SunBlack]

Mhm, there are only two changed which could have an effect:

• Rename from BUILD_tools to build, but this does not changed anything as explained in conversation above.

• I change this on time from

if(UNIX OR APPLE)
  set(CUDA_NVCC_FLAGS ${CUDA_NVCC_FLAGS}  "-Xcompiler;-fPIC;")
endif()
if(NOT UNIX OR APPLE)
  add_definitions(-DPCLAPI_EXPORTS)
endif()

to

if(UNIX OR APPLE)
  set(CUDA_NVCC_FLAGS ${CUDA_NVCC_FLAGS}  "-Xcompiler;-fPIC;")
else
  add_definitions(-DPCLAPI_EXPORTS)
endif()

But I now believe this adjustment was not correct, even it should not affect Ubuntu build. I believed if(NOT UNIX OR APPLE) is equal to if(NOT (UNIX OR APPLE)) bot now I think it is equal to if((NOT UNIX) OR APPLE) (not tested yet if OR or NOT are stronger in CMake), so I believe correct would be:

if(UNIX OR APPLE)
  set(CUDA_NVCC_FLAGS ${CUDA_NVCC_FLAGS}  "-Xcompiler;-fPIC;")
endif()
if(NOT UNIX) # Removed APPLE, because if NOT UNIX is already true, value of APPLE, WIN32, ... doesn't matter
  add_definitions(-DPCLAPI_EXPORTS)
endif()

[taketwo]

So I believe correct would be:

if(UNIX OR APPLE)
  set(CUDA_NVCC_FLAGS ${CUDA_NVCC_FLAGS}  "-Xcompiler;-fPIC;")
endif()
if(NOT UNIX) # Removed APPLE, because if NOT UNIX is already true, value of APPLE, WIN32, ... doesn't matter
  add_definitions(-DPCLAPI_EXPORTS)
endif()

Are you sure that APPLE is not counted as UNIX? I can imagine that both variables are true on MacOS, though I'm not a MacOS user, so can not confirm.

[taketwo]

Generally, I don't have a good feeling about this PR in it's current state. We need to have absolutely clear separation between commits that only change indentation, commits that change variable names, and commits that (potentially) alter the logic. Otherwise, if a problem is discovered later on, we will need to skim through 5K lines trying to identify if we have some "innocent fix" that was smuggled together with indentation change.

[SunBlack]

I have reverted logic change to this (see last commit), so this PR should just adjust nesting and indentation (except the one thing with BUILD_tools).

[SunBlack]

Last entries on log before crash:

[ 85%] Building CXX object test/registration/CMakeFiles/test_registration_api.dir/test_registration_api.cpp.o
Scanning dependencies of target test_registration_api
[ 85%] Built target test_keypoints
[ 85%] Linking CXX executable test_keypoints
[ 85%] Building CXX object test/registration/CMakeFiles/test_registration.dir/test_registration.cpp.o

But I don't see any big CMake adjustments in test/registration/CMakeFiles.

Can you rerun build of last successful build (ba0a6a6), so we can see, if it is not any other issue?

[taketwo]

Let's see: https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=499

Edit: maybe we should consider splitting test_registration into several parts.

[SunBlack]

Found an issue by comparing resulting Makefile before and after my change. Now they are equal :)

[taketwo]

Old code:

pcl/test/surface/CMakeLists.txt

Lines 11 to 12 in f3dac94

	if(build)
	if(BUILD_io AND BUILD_features)

New code:

if(NOT (build OR (BUILD_io AND BUILD_features)))
   return()
endif()

This is not equivalent. Where does OR come from?

[SunBlack]

You are right. Fixed.

[SunBlack]

Let's see: https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=499

Failed too, so it seems a issue not related to this PR.

[taketwo]

We need to resolve a conflict and to rebase here as well, then can merge if the pipelines succeed.

[taketwo]

I was going through the PR one last time and noticed this change in NSIS. Please revert it and then I'll merge.