Rwindows

[muschellij2]

For building ITK with R, the following changes are required. Some changes are Windows-related.

Eigen3 changes

The changes to Eigen3 are required as ITK is built using R CMD check, which creates a temporary folder to check the package and build the binary: https://travis-ci.org/muschellij2/ITKR/jobs/500018126#L2538. The change ensures that ITKEigen3.cmake in the output has a relative path as currently it is using ${CMAKE_INSTALL_PREFIX}, which makes it less robust, especially if the path is moved. This causes issues when a package wishes to build on ITK, such as ANTs, which is built in R using ANTsRCore and the binary ITKR which was built above: https://travis-ci.org/muschellij2/ANTsRCore/jobs/500018491#L6596. The error here relates to the absolute path Eigen3 is making in the cmake file.

H5wins changes

For building ITKR on Windows (https://ci.appveyor.com/project/muschellij2/itkr), this uses Rtools (https://cran.r-project.org/bin/windows/Rtools/), which is an MSYS2/MINGW build (https://github.com/r-windows/rtools-packages). Here, the builds were failing with

In function 'H5D__efl_read':
C:\run\ITKR.Rcheck\00_pkg_src\ITKR\src\itks\Modules\ThirdParty\HDF5\src\itkhdf5\src\H5win32defs.h:57:66: error: expected expression before ')' token

https://ci.appveyor.com/project/muschellij2/itkr/builds/19044355/job/pvyblx15dws7x66d#L4280. We found this switch to work well. This patch was suggested in #66 but seemed abandoned https://review.source.kitware.com/#/c/23773/ and I don't believe #397 fixes this.

VNL

The VNL fix is the most dubious out of all of these. The issue in https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/VNL/src/vxl/vcl/CMakeLists.txt#L18 is that this causes an incorrect absolute path on Windows as seen in :
https://ci.appveyor.com/project/muschellij2/itkr/builds/21974640/job/v6xp383af3jfp1j8#L6815. I'm not sure of the fix, so I'm fine if this is not incorporated, but I'm unsure on how to resolve this other than removing the compiler header.

[dzenanz]

I am looking into the VNL absolute path issue.

[hjmjohnson]

@dzenanz I think that the absolute path issue was fixed upstream. We need to wait until the windows builds work again before updating VNL.

Is there an update on when windows builds will work again?

[phcerdan]

Does the upstream version of this file fix this issue?

https://github.com/live-clones/hdf5/blob/master/src/H5win32defs.h#L46-L49

[muschellij2]

Yes I believe so.

[muschellij2]

I don't actually believe that this fixes it sufficiently without hte rest of the HDF5 changes as failures happen with H5Defl.c:

C:\run\ITKR.Rcheck\00_pkg_src\ITKR\src\itks\Modules\ThirdParty\HDF5\src\itkhdf5\src\H5Defl.c: In function 'H5D__efl_read':
C:\run\ITKR.Rcheck\00_pkg_src\ITKR\src\itks\Modules\ThirdParty\HDF5\src\itkhdf5\src\H5Defl.c:291:44: error: macro "HDopen" requires 3 arguments, but only 2 given
         if((fd = HDopen(full_name, O_RDONLY)) < 0)
                                            ^
C:\run\ITKR.Rcheck\00_pkg_src\ITKR\src\itks\Modules\ThirdParty\HDF5\src\itkhdf5\src\H5Defl.c:291:18: error: 'HDopen' undeclared (first use in this function)
         if((fd = HDopen(full_name, O_RDONLY)) < 0)
                  ^
C:\run\ITKR.Rcheck\00_pkg_src\ITKR\src\itks\Modules\ThirdParty\HDF5\src\itkhdf5\src\H5Defl.c:291:18: note: each undeclared identifier is reported only once for each function it appears in

https://ci.appveyor.com/project/muschellij2/itkr/builds/23054691

[muschellij2]

This still is an issue, but I think we can bring up in new PR

[dzenanz]

VNL absolute path issue should be fixed by vxl/vxl#604.

[dzenanz]

I squashed the PR into a single commit. This simplifies reviewing.

[dzenanz]

Once VNL fix is integrated, this should be rebased on top with VNL-related changes removed.

[muschellij2]

Thanks for reviews; seems like relative Eigen3 path is the main question fix

On Fri, Mar 1, 2019 at 12:33 PM Dženan Zukić ***@***.***> wrote: Once VNL fix is integrated, this should be rebased on top with VNL-related changes removed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#562 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBnrin64lOcU7wnkyRryIhrWCz2dq0nks5vSWR_gaJpZM4bX7Di> .

-- John

[dzenanz]

Please rebase on current master.

[phcerdan]

Should we open a new PR with just the Eigen fix?

[dzenanz]

That would work. Please add @muschellij2 as a co-author. Then if there are any leftover problems, a new issues or pull request can be created.

[phcerdan]

@muschellij2 @dzenanz I opened #567 (Co-authored-by @muschellij2 ) to fix Eigen3.

[thewtex]

/azp run ITK.Windows

[thewtex]

@muschellij2 @dzenanz @phcerdan can this please be rebased on master to resolve the Eigen conflict?

[dzenanz]

This PR is so outdated now, I will close it. @muschellij2 if there is still some leftover problems, please create a new PR or issue.