Update buildsystem to support customapp bundling

[jcfr]

No description provided.

[jcfr]

@yrathi Once this is integrated, we will rebase & consolidate change associated with #143 and #146

cc: @pieper @LucasGandel

[pieper]

Thank you @jcfr 🙏

@yrathi it's hard to review these changes but I think we should accept to keep the code current or it will fall far behind. Old code and binaries will still be available in the case that something unusual is introduced.

[jcfr]

The white space changes can be excluded from the review appending ?w=1 to the URL.

For example, see https://github.com/pnlbwh/ukftractography/pull/149/files?w=1

Or by explicitly toggling the option:

See https://github.blog/changelog/2021-10-14-hiding-whitespace-is-now-remembered-for-each-pull-request/

[tashrifbillah]

Please wait ... we are dealing with failed storage at our office now.

[LucasGandel]

Tested on Windows with VS2019. Works like a charm after adding the following changes:
UKF_diff.txt

cc: @jcfr

[tashrifbillah]

What GCC did you use to test it on CentOS 7? Are you able to build it using the native 4.8.5?

[jcfr]

Can we move forward with the integration ?

To help move forward, I can split the style commits (STYLE: Strip trailing spaces and STYLE: Convert tabs to spaces) into their own pull request, let me know how you would like to move forward.

[tashrifbillah]

#149 (comment)

comment unaddressed.

[jcfr]

What GCC did you use to test it on CentOS 7?

Testing was done using GCC 7, see latest environment described at https://github.com/Slicer/SlicerBuildEnvironment#supported-build-environments (itself based of https://github.com/dockbuild/dockbuild#compiling-environments)

Are you able to build it using the native 4.8.5?

Instead of using the outdated 4.8.5 compiler on Centos 7, we recommend upgrading your build environment leveraging the RedHat developer toolset.

Doing so allows to use modern compiler while being being compatible with older GlibC.

[tashrifbillah]

we recommend upgrading your build environment leveraging the RedHat developer toolset.

Hard to follow this recommendation in our workstation and cluster environment. They are all CentOS 7 and GCC 4.8.5 based. We shall discuss it internally.

[tashrifbillah]

In any case, does your PR supersede #143?
@hjmjohnson

[jcfr]

As described in #149 (comment)

@yrathi Once this is integrated, we will rebase & consolidate change associated with #143 and #146

[tashrifbillah]

I think I can run the extra mile to install GCC-7 in our environments and see if it builds. Did you add any new tests that would confirm that your builds did not break the underlying algebra/arithmetic?

[jcfr]

Did you add any new tests that would confirm that your builds did not break the underlying algebra/arithmetic?

Since these set of changes only focus on updating the build-system so that it is more consistent and maintainable, there were no tests added (All commits are labelled COMP or STYLE)

[jcfr]

Once this PR is integrated, we will take care of re-basing and addressing issues associated with the other ones.

[tashrifbillah]

Building in progress ...
cc @dheshanm

[dheshanm]

Building the current codebase as-is with cmake-3.23.0-rc3-linux-x86_64 and gcc-9.3.0 results in the following cmake errors:

-- Setting C++ standard
-- Setting C++ standard - C++17
CMake Error at CMake/ExternalProjectDependency.cmake:75 (message):
  Failed to initialize variable SUPERBUILD_TOPLEVEL_PROJECT.  Variable
  CMAKE_PROJECT_NAME is not defined.
Call Stack (most recent call first):
  Common.cmake:2 (include)
  CMakeLists.txt:97 (include)


-- Configuring incomplete, errors occurred!

Steps to reproduce:

1. git clone [email protected]:jcfr/ukftractography.git -b update-buildsystem-to-support-customapp-bundling
2. cd ukftractography
3. mkdir build && cd build
4. cmake ../

I can further confirm that the same configuration, with the same steps builds master branch at pnlbwh/ukftractography without issue.

[jcfr]

Suggested change

	find_package(VTK COMPONENTS
	find_package(VTK 9 COMPONENTS

This will make thing unambiguous

[dheshanm]

I was able to get the build to succeed by making the following changes:

1. Move include(${CMAKE_CURRENT_SOURCE_DIR}/Common.cmake) after project definition on CMakeLists.txt
2. Reverting changes to VTK on UKFTractography.cmake
3. Using 'git' instead of ${GIT_EXECUTABLE} on ukf/CMakeLists.txt

More Details

1. Move `include(${CMAKE_CURRENT_SOURCE_DIR}/Common.cmake)` after project definition on CMakeLists.txt

ukftractography/CMakeLists.txt

Lines 97 to 108 in 32ec799

	include(${CMAKE_CURRENT_SOURCE_DIR}/Common.cmake) #<-- All feature options for top superbuild and inner product build
	if(${PRIMARY_PROJECT_NAME}_SUPERBUILD)
	project(SuperBuild_${PRIMARY_PROJECT_NAME} LANGUAGES C CXX) # <- NOTE: Project name for pre-requisites is different form main project
	include("${CMAKE_CURRENT_SOURCE_DIR}/SuperBuild.cmake")
	return()
	else()
	# Building against Slicer
	project(${PRIMARY_PROJECT_NAME} LANGUAGES C CXX) # <- NOTE: Here is the main project name setting
	include("${CMAKE_CURRENT_SOURCE_DIR}/${PRIMARY_PROJECT_NAME}.cmake")
	return()
	endif()

Move include(${CMAKE_CURRENT_SOURCE_DIR}/Common.cmake) after project definition.

#-----------------------------------------------------------------------------
# Superbuild script
#-----------------------------------------------------------------------------

if(${PRIMARY_PROJECT_NAME}_SUPERBUILD)
  project(SuperBuild_${PRIMARY_PROJECT_NAME} LANGUAGES C CXX)  # <- NOTE: Project name for pre-requisites is different form main project

 # ===== MOVED HERE BELOW =====
  include(${CMAKE_CURRENT_SOURCE_DIR}/Common.cmake) #<-- All feature options for top superbuild and inner product build
 # ===== MOVED HERE ABOVE =====

  include("${CMAKE_CURRENT_SOURCE_DIR}/SuperBuild.cmake")
  return()
else()
  # Building against Slicer
  project(${PRIMARY_PROJECT_NAME} LANGUAGES C CXX)             # <- NOTE: Here is the main project name setting
  include("${CMAKE_CURRENT_SOURCE_DIR}/${PRIMARY_PROJECT_NAME}.cmake")
  return()
endif()
message(FATAL_ERROR "You should never reach this point !")

2. Reverting changes to VTK on UKFTractography.cmake

ukftractography/UKFTractography.cmake

Lines 20 to 31 in 32ec799

	set(VTK_FOUND OFF)
	find_package(VTK COMPONENTS
	CommonSystem
	CommonCore
	CommonSystem
	CommonMath
	CommonMisc
	CommonTransforms
	IOLegacy
	IOXML
	REQUIRED
	)

• Reverting changes to VTK back to

  set(VTK_FOUND OFF)
  find_package(VTK COMPONENTS
    vtkCommonSystem
    vtkCommonCore
    vtkCommonSystem
    vtkCommonMath
    vtkCommonMisc
    vtkCommonTransforms
    vtkIOLegacy
    vtkIOXML
    REQUIRED)
  if(VTK_USE_FILE)
    include(${VTK_USE_FILE})
  endif()

3. Using 'git' instead of ${GIT_EXECUTABLE} on ukf/CMakeLists.txt

ukftractography/ukf/CMakeLists.txt

Line 14 in 32ec799

COMMAND ${GIT_EXECUTABLE} rev-parse HEAD >> "${UKF_GITVER_TEMP}"

Instead using COMMAND git rev-parse HEAD >> "${UKF_GITVER_TEMP}"

Further

Will try building with the proposed find_package(VTK 9 COMPONENTS again.

[tashrifbillah]

Move include(${CMAKE_CURRENT_SOURCE_DIR}/Common.cmake) after project definition on CMakeLists.txt

@jcfr Jean, I think this is another major problem. Feel free to propose a solution.

[dheshanm]

Tried building with proposed find_package(VTK 9 COMPONENTS results in the following build error:

CMake Error at UKFTractography.cmake:21 (find_package):
  Could not find a configuration file for package "VTK" that is compatible
  with requested version "9".

  The following configuration files were considered but not accepted:

    /build/VTK-build/VTKConfig.cmake, version: 7.1.1

Call Stack (most recent call first):
  CMakeLists.txt:110 (include)

Note: This build used the modifications 1 and 3 as outlined from the above comment, and used the proposed find_package(VTK 9 COMPONENTS for the 2nd issue instead of reverting.

[jcfr]

That makes sense since I initially tried to avoid updating VTK external project .. which is still building VTK 7.

I will follow in the next few days with an update. Thanks for the review

[tashrifbillah]

That makes sense since I initially tried to avoid updating VTK external project .. which is still building VTK 7.

Were you able to successfully build this PR before proposing it? @jcfr

[tashrifbillah]

@jcfr , @LucasGandel , @pieper :

UKFTractography headquarter is moving to RedHat 9 OS. Is this PR still of interest? If yes, we can try to build it. For the record, we tried it in CentOS 7 but did not work.

[pieper]

I'd be fine with whatever builds best on modern systems.

@jcfr is this still useful for you?

[jcfr]

The changes are expected to be general and still relevant, while I was actively working on this, I had started to further improve the PR to address both the "standalone" and "bundled" case but had to pause due to other constraints.

Waiting for when I have time to address those, I will submit smaller PR with stylistic changes and alike.

[tashrifbillah]

That makes sense since I initially tried to avoid updating VTK external project .. which is still building VTK 7.

I will follow in the next few days with an update. Thanks for the review

@jcfr , this PR does not build on RHEL 9 either. I get Dheshan's error: #149 (comment)

He already proposed his solution here: #149 (comment)
Can you follow up with a revision?