Fix modern itk and vtk conformance #143

hjmjohnson · 2022-02-14T21:09:10Z

Update to include conformance with newer ITK and VTK enforced constraints.

ITK 5.3 and VTK 9.1

These changes should be backward compatible.

tashrifbillah · 2022-02-14T21:56:00Z

Hi Steve (@pieper), will Hans' changes be complatible with Slicer?

pieper · 2022-02-14T22:09:38Z

Hi Steve (@pieper), will Hans' changes be complatible with Slicer?

Hi @tashrifbillah - I think yes, they look minor and Slicer is pretty up to date with the latest VTK and ITK versions inn prep for Slicer5 release.

tashrifbillah · 2022-02-14T22:11:30Z

Hello @hjmjohnson , did you test as:

mkdir build
cd build
cmake ..
make
UKFTractography-build/UKFTractography/bin/UKFTractography --help

hjmjohnson · 2022-02-15T14:38:23Z

@tashrifbillah UKF has some tests for building for 8 year old versions of Mac. Slicer now has a minimum requirement of 10.13 or greater for mac. Is it OK to update UKF to also have the same minimum requirements as slicer?

PS: I'm running the build test suggested above, but I am 99% sure it will succeed.

tashrifbillah · 2022-02-15T14:53:45Z

Is it OK to update UKF to also have the same minimum requirements as slicer?

@pieper , can you answer this quickly? Otherwise, I'll have to do my research on it.

@hjmjohnson , if you are testing on MAC, I can test your PR on CentOS 7 Linux.

@yrathi , do we need support of CentOS 6 anymore?

hjmjohnson · 2022-02-15T14:56:09Z

@tashrifbillah The mac build is failing due to known problems with the old version of Eigen. If I update Eigen as well, then the build succeeds.

yrathi · 2022-02-15T15:10:53Z

Thanks @hjmjohnson and @tashrifbillah. We dont need support for CentOS 6. And Yes, we can update UKF to have the minimum requirement as Slicer.

hjmjohnson · 2022-02-16T01:41:57Z

This now builds on mac 10.15 again. super build packages brought up-to-date with slicer versions of packages.

hjmjohnson · 2022-02-16T14:10:28Z

@yrathi

I have a series of code maintenance and C++14 performance updates that I would like to apply. Is there support/interest in adopting coding styles and performance improvements as vetted and incorporated in the ITK and VTK communities for UKF?

The proposed code transformations have been very safe as applied to many ITK-derived projects so far.

tashrifbillah · 2022-02-16T14:10:46Z

Your initial PR failed to build in my CentOS 7 ...
I can test the modification again.

hjmjohnson · 2022-02-16T14:37:01Z

@tashrifbillah NOTE: Centos7 has a default compiler install of "GCC 4.8.5" which does not fully support C++11. Centos7 was first released in 2009.

We have a large cluster of computers still running and performing well with Centos7, but they have provided alternative compilers to keep up with code bases that require C++11.

hjmjohnson · 2022-02-16T14:39:49Z

@tashrifbillah Can you let me know what compiler toolchain you were attempting to use on Centos7, and what the error you came across was?

tashrifbillah · 2022-02-16T14:58:28Z

What GCC did you use?

hjmjohnson · 2022-02-16T15:17:35Z

@tashrifbillah my cluster has 8.3.0 available. I am trying to find the build errors with 4.8.5 now, I expect this build to fail. I will then attempt a gcc 8.3.0 build.

Hans

tashrifbillah · 2022-02-16T15:29:55Z

I have other GCCs I can try. I just wanted to know what you used. No need to try with CentOS 7 default.

yrathi · 2022-02-16T15:39:10Z

@tashrifbillah -- We should be sure that UKF builds on both the PNL network and the cluster (without having to use any containers etc.). If it doesn't, then we need to think if this update should be applied or not.

hjmjohnson · 2022-02-16T16:33:00Z

@tashrifbillah I messed up the VTK clone command in a previous patch fix. The error was not identified until I did a clean build. updated PR coming soon.

hjmjohnson · 2022-02-16T17:11:40Z

@tashrifbillah NOTE: Minimum compiler version supported by ITK for C++14 is claimed to be gcc 5.1. I've added a file to test known minimum compiler versions. Doing a clean build with gcc8.3.0 ... SUCCEEDED!

hjmjohnson · 2022-02-25T22:22:07Z

@tashrifbillah Do you have any concerns with this PR? I would love to get this PR included.

Hans

tashrifbillah · 2022-02-25T22:23:25Z

Sorry Hans, we didn't get time to work on it again yet. I shall get to it soon.

tashrifbillah · 2022-02-28T14:16:20Z

Hans, what is your cmake version?

$ cmake ..
CMake Error at CMakeLists.txt:15 (cmake_minimum_required):
  CMake 3.16.3 or higher is required.  You are running version 3.14.2


-- Configuring incomplete, errors occurred!

I want to understand if you have mandated the above or GCC 7.3.0 I am using has.

tashrifbillah · 2022-03-08T18:10:35Z

Ping @hjmjohnson

hjmjohnson · 2022-03-08T22:22:30Z

@tashrifbillah ITK requires cmake version 3.16.3 to compile.

tashrifbillah · 2022-03-11T20:22:24Z

Testing update:

builds on CentOS 7 cluster with cmake-3.19.2 and GCC-7.3.0
builds on CentOS 7 workstation with 3.23.0-rc3 and GCC-7.3.0

CMakeLists.txt

tashrifbillah · 2022-03-11T21:21:45Z

Now that we are done with building, I shall review your proposals on Eigen, Boost, Apple, and Win32 later.

error "Do not include itkExceptionObject.h directly, include itkMacro.h instead."

[8/14] Building CXX object vtk2mask/CMakeFiles/vtk2maskLib.dir/Converter.cc.o UKF/vtk2mask/Converter.cc: In member function ‘void Converter::WriteOutNrrd(const string&)’: UKF/vtk2mask/Converter.cc:305:31: warning: catching polymorphic type ‘class itk::ExceptionObject’ by value [-Wcatch-value=] 305 | catch( itk::ExceptionObject e ) | ^ UKF/vtk2mask/Converter.cc:322:33: warning: catching polymorphic type ‘class itk::ExceptionObject’ by value [-Wcatch-value=] 322 | catch( itk::ExceptionObject e ) | ^ ninja: build stopped: subcommand failed.

New version of VTK enforces constraint that was previously commented as unsafe usage. The requirement that the returned array is not modified is now enforced by requiring const in the variable declaration. UKF/fibertractdispersion/fiberbundle.cxx: In member function ‘void fiberbundle::ReadFibers(std::string)’: UKF/fibertractdispersion/fiberbundle.cxx:43:43: error: no matching function for call to ‘vtkCellArray::GetNextCell(vtkIdType&, const vtkIdType**)’ 43 | if(curLines->GetNextCell(nPoints, &pts) == 0) | ^ In file included from NAMICExternalProjects-Release-5.8.0/include/vtk-9.1/vtkPolyData.h:68, from UKF/fibertractdispersion/fiberbundle.h:5, from UKF/fibertractdispersion/fiberbundle.cxx:1: NAMICExternalProjects-Release-5.8.0/include/vtk-9.1/vtkCellArray.h:1497:12: note: candidate: ‘int vtkCellArray::GetNextCell(vtkIdType&, const vtkIdType*&)’ 1497 | inline int vtkCellArray::GetNextCell(vtkIdType& npts, vtkIdType const*& pts) VTK_SIZEHINT(pts, npts) | ^~~~~~~~~~~~ NAMICExternalProjects-Release-5.8.0/include/vtk-9.1/vtkCellArray.h:1497:73: note: no known conversion for argument 2 from ‘const vtkIdType**’ {aka ‘const long long int**’} to ‘const vtkIdType*&’ {aka ‘const long long int*&’} 1497 | inline int vtkCellArray::GetNextCell(vtkIdType& npts, vtkIdType const*& pts) VTK_SIZEHINT(pts, npts) | ~~~~~~~~~~~~~~~~~~^~~ NAMICExternalProjects-Release-5.8.0/include/vtk-9.1/vtkCellArray.h:1512:12: note: candidate: ‘int vtkCellArray::GetNextCell(vtkIdList*)’ 1512 | inline int vtkCellArray::GetNextCell(vtkIdList* pts) | ^~~~~~~~~~~~

Only support the same OSX deployments as Slicer.

Simplify the conditional statements for building on mac now that older versions of the MacOSX are not supported.

Retrieving a shallow copy of the ITK git repository is failing.

For compilers that are known to fail to build ITK the failure should occur quickly in the build process.

Bring superbuild packages in line with Slicer. Slicer is in the process of updating its minimum required compiler standard to c++14. This PR updates the external packages to be inline with the Slicer versions of the packages. Update External_Boost to modern version.

hjmjohnson · 2022-03-30T13:15:34Z

@tashrifbillah I rebased on the latest master branch. Is there something I can do to help push this set of changes through?

hjmjohnson · 2022-06-02T15:49:00Z

@tashrifbillah Are there outstanding concerns with this update?

tashrifbillah · 2022-06-06T21:29:05Z

Sorry we kind of forgot about it. I shall take one final look at it tomorrow and reach out with questions should I have any. Thank you Hans.

hjmjohnson · 2022-06-16T16:08:52Z

@tashrifbillah Any concerns found?

hjmjohnson · 2022-07-26T14:11:26Z

@yrathi Are there outstanding concerns with this PR? I'm trying to clear my summer todo list.

yrathi · 2022-07-26T14:12:58Z

@hjmjohnson -- I dont think @tashrifbillah got a chance to double check things. I will wait for his response.

hjmjohnson · 2023-04-06T15:41:30Z

@yrathi Can this be merged yet?

tashrifbillah · 2023-04-06T15:47:44Z

@tashrifbillah -- We should be sure that UKF builds on both the PNL network and the cluster (without having to use any containers etc.). If it doesn't, then we need to think if this update should be applied or not.

Hans, I see that Jean added more commits. So I need to test this PR again in both places as Yogesh noted.

tashrifbillah · 2023-04-06T16:38:47Z

Testing update:

* builds on CentOS 7 cluster with `cmake-3.19.2` and `GCC-7.3.0`

* builds on CentOS 7 workstation with `3.23.0-rc3` and `GCC-7.3.0`

To make GCC-7.3.0 discoverable to cmake in workstation:

cd ukftractography
mkdir build
cd build

export LD_LIBRARY_PATH=/path/to/gcc-7.3.0/gccbuild/lib/gcc/x86_64-pc-linux-gnu/7.3.0:/path/to/gcc-7.3.0/gccbuild/lib/gcc/x86_64-pc-linux-gnu/7.3.0/lib:/path/to/gcc-7.3.0/gccbuild/lib/gcc/x86_64-pc-linux-gnu/7.3.0/lib64
export PATH=/path/to/gcc-7.3.0/gccbuild/bin:$PATH

CC=/path/to/gcc-7.3.0/gccbuild/bin/gcc CXX=/path/to/gcc-7.3.0/gccbuild/bin/g++ cmake .
# https://stackoverflow.com/questions/17275348/how-to-specify-new-gcc-path-for-cmake

Note: CC and CXX settings were discovered later. Maybe PATH and LD_LIBRARY_PATH settings are not required. Just PATH and LD_LIBRARY_PATH settings do not make it discoverable.

tashrifbillah · 2023-04-06T16:42:21Z

Jean's update results in the following error. It could not be built in our cluster environment:

-- Searching 16 bit integer - Using unsigned short
-- Check if the system is big endian - little endian
-- Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY OPENGL_glx_LIBRARY OPENGL_INCLUDE_DIR OpenGL) 
CMake Error at CMake/vtkModule.cmake:4247 (message):
  Could not find the OpenGL external dependency.
Call Stack (most recent call first):
  CMake/vtkModule.cmake:4799 (vtk_module_find_package)
  Utilities/OpenGL/CMakeLists.txt:58 (vtk_module_third_party_external)

Hans, note that your updates had built fine at that time. I waited to read your PR line by line but never got a chance.

tashrifbillah · 2023-04-06T16:46:55Z

In workstation, it fails with a different error:

> cp b2 bjam
tools/build/src/engine/b2
Detecting Python version... 2.7
Detecting Python root... /usr
Unicode/ICU support for Boost.Regex?... /usr
Generating B2 configuration in project-config.jam for gcc...

Bootstrapping is done. To build, run:

    ./b2
    
To generate header files, run:

    ./b2 headers

The configuration generated uses gcc to build by default. If that is
unintended either use the --with-toolset option or adjust configuration, by
editing 'project-config.jam'.

Further information:

   - Command line help:
     ./b2 --help
     
   - Getting started guide: 
     http://www.boost.org/more/getting_started/unix-variants.html
     
   - B2 documentation:
     http://www.boost.org/build/

[  8%] Performing build step for 'Boost'
./b2: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.20' not found (required by ./b2)
./b2: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by ./b2)
make[2]: *** [Boost-prefix/src/Boost-stamp/Boost-build] Error 1
make[1]: *** [CMakeFiles/Boost.dir/all] Error 2
make: *** [all] Error 2

hjmjohnson · 2023-04-10T14:14:22Z

@tashrifbillah I don't know how to interpret your last messages. Are you rejecting these changes? Is the bug related to Jean's changes? None of the recent messages seem to be related to the PR.

tashrifbillah · 2023-04-10T14:36:38Z

Is the bug related to Jean's changes?

Yes.

tashrifbillah · 2023-04-10T14:37:07Z

None of the recent messages seem to be related to the PR.

All of the.

tashrifbillah · 2023-04-10T14:46:18Z

I don't know how to interpret your last messages.

You can undo Jean's changes or have him fix the errors I reported in #143 (comment) and #143 (comment) .

hjmjohnson · 2023-08-08T13:29:48Z

@tashrifbillah I'm going to close this issue as there does not seem to be interest in these updates.

hjmjohnson requested a review from yrathi February 14, 2022 21:09

hjmjohnson force-pushed the fix-modern-itk branch from b680bce to 81d29f1 Compare February 16, 2022 14:27

hjmjohnson force-pushed the fix-modern-itk branch from 5374fc5 to ba5fe31 Compare February 16, 2022 17:13

hjmjohnson requested a review from tashrifbillah February 25, 2022 22:22

hjmjohnson self-assigned this Feb 25, 2022

tashrifbillah reviewed Mar 11, 2022

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

hjmjohnson added 9 commits March 30, 2022 08:11

COMP: Include recommended header for ExceptionObjects

1cf2dc1

error "Do not include itkExceptionObject.h directly, include itkMacro.h instead."

ENH: Use modern Slicer OSX target designations

a0c16ba

Only support the same OSX deployments as Slicer.

COMP: Simplify mac deployment builds

2a94365

Simplify the conditional statements for building on mac now that older versions of the MacOSX are not supported.

ENH: Remove windows end-of-line-marks

1641c01

COMP: Shallow git checkout failing for ITK

1b5911f

Retrieving a shallow copy of the ITK git repository is failing.

ENH: Improve compiler checks to fail early

23597b4

For compilers that are known to fail to build ITK the failure should occur quickly in the build process.

hjmjohnson force-pushed the fix-modern-itk branch from ba5fe31 to 1ede1e2 Compare March 30, 2022 13:14

rmukh mentioned this pull request Jun 9, 2022

CMake build improvements + some extra #146

Closed

jcfr mentioned this pull request Jan 13, 2023

Update buildsystem to support customapp bundling #149

Open

hjmjohnson closed this Aug 8, 2023

Fix modern itk and vtk conformance #143

Fix modern itk and vtk conformance #143

Conversation

hjmjohnson commented Feb 14, 2022

tashrifbillah commented Feb 14, 2022

pieper commented Feb 14, 2022

tashrifbillah commented Feb 14, 2022 • edited Loading

hjmjohnson commented Feb 15, 2022

tashrifbillah commented Feb 15, 2022

hjmjohnson commented Feb 15, 2022

yrathi commented Feb 15, 2022

hjmjohnson commented Feb 16, 2022

hjmjohnson commented Feb 16, 2022

tashrifbillah commented Feb 16, 2022

hjmjohnson commented Feb 16, 2022

hjmjohnson commented Feb 16, 2022

tashrifbillah commented Feb 16, 2022

hjmjohnson commented Feb 16, 2022

tashrifbillah commented Feb 16, 2022

yrathi commented Feb 16, 2022

hjmjohnson commented Feb 16, 2022

hjmjohnson commented Feb 16, 2022

hjmjohnson commented Feb 25, 2022

tashrifbillah commented Feb 25, 2022

tashrifbillah commented Feb 28, 2022 • edited Loading

tashrifbillah commented Mar 8, 2022

hjmjohnson commented Mar 8, 2022

tashrifbillah commented Mar 11, 2022 • edited Loading

tashrifbillah commented Mar 11, 2022

hjmjohnson commented Mar 30, 2022

hjmjohnson commented Jun 2, 2022

tashrifbillah commented Jun 6, 2022

hjmjohnson commented Jun 16, 2022

hjmjohnson commented Jul 26, 2022

yrathi commented Jul 26, 2022

hjmjohnson commented Apr 6, 2023

tashrifbillah commented Apr 6, 2023

tashrifbillah commented Apr 6, 2023 • edited Loading

tashrifbillah commented Apr 6, 2023

tashrifbillah commented Apr 6, 2023

hjmjohnson commented Apr 10, 2023

tashrifbillah commented Apr 10, 2023

tashrifbillah commented Apr 10, 2023

tashrifbillah commented Apr 10, 2023

hjmjohnson commented Aug 8, 2023

tashrifbillah commented Feb 14, 2022 •

edited

Loading

tashrifbillah commented Feb 28, 2022 •

edited

Loading

tashrifbillah commented Mar 11, 2022 •

edited

Loading

tashrifbillah commented Apr 6, 2023 •

edited

Loading