elastix icon indicating copy to clipboard operation
elastix copied to clipboard

WIP: Use vcpkg to get ITK

Open N-Dekker opened this issue 4 years ago • 16 comments

See also https://discourse.itk.org/t/how-to-avoid-rebuilding-itk-by-azure-ci-at-the-github-of-another-project-elastix/2002

N-Dekker avatar Jul 09 '19 20:07 N-Dekker

Compilation errors at https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=467 for example:

itkAccumulateDerivativesParallellizationTest.cxx(62): error C2039: 'ThreadInfoStruct': is not a member of 'itk::PlatformMultiThreader'

because of ITK_LEGACY_REMOVE:

https://github.com/microsoft/vcpkg/blob/47d206e149e88201333b5ca8fe55c30e2b1a8177/ports/itk/portfile.cmake#L38

N-Dekker avatar Jul 11 '19 13:07 N-Dekker

The missing ITK legacy support should be added by https://github.com/microsoft/vcpkg/pull/7241

N-Dekker avatar Jul 16 '19 22:07 N-Dekker

Interesting compile errors at https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=505:

elxAdaptiveStochasticLBFGS.hxx(1573): error C2440: 'initializing': cannot convert from 'const itk::AdvancedTransform<double,2,2> *' to 'itk::SmartPointer<itk::AdvancedTransform<double,2,2>>

elxAdaptiveStochasticLBFGS.hxx(970): error C2662: 'void itk::Transform<TParametersValueType,2,2>::SetParameters(const itk::OptimizerParameters<TParametersValueType> &)': cannot convert 'this' pointer from 'const itk::AdvancedTransform<double,2,2>' to 'itk::Transform<TParametersValueType,2,2> &'

elxAdaptiveStochasticLBFGS.hxx(988): error C2664: 'void itk::ComputeJacobianTerms<itk::Image<float,2>,itk::AdvancedTransform<double,2,2>>::SetTransform(itk::AdvancedTransform<double,2,2> *)': cannot convert argument 1 from 'const itk::AdvancedTransform<double,2,2> *' to 'itk::AdvancedTransform<double,2,2> *'

And also link errors:

LINK : fatal error LNK1181: cannot open input file '\lib\double-conversion.lib'

N-Dekker avatar Aug 08 '19 09:08 N-Dekker

@dzenanz Dženan I'm still trying to build elastix with ITK from vcpkg, but we're getting link errors, at https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=516:

LINK : fatal error LNK1181: cannot open input file '\lib\double-conversion.lib'

Could this be because ITK_USE_SYSTEM_DOUBLECONVERSION is explicitly switched ON, in the vcpkg ITK portfile.cmake? https://github.com/microsoft/vcpkg/blob/743e168ef5c7705e44d1d5cab5b9cca22328345e/ports/itk/portfile.cmake#L41

Do you have a suggestion how to avoid these link errors?

N-Dekker avatar Aug 09 '19 16:08 N-Dekker

There was some work regarding ITK_USE_SYSTEM_DOUBLECONVERSION between 5.0.0 and 5.0.1. Can you try with vcpkg 7352fab or newer?

dzenanz avatar Aug 09 '19 17:08 dzenanz

There was some work regarding ITK_USE_SYSTEM_DOUBLECONVERSION between 5.0.0 and 5.0.1. Can you try with vcpkg 7352fab or newer?

Thanks @dzenanz Unfortunately the virtual machine I'm using at azure, 'vs2017-win2016', still has a vcpkg git repository of July 16: https://github.com/microsoft/vcpkg/commit/ac00ef2070be876479bcf667ac2f41c0a888bdb8 I guess I just have to wait.

Did you also get those double-conversion.lib link errors from ITK 5.0.0 at vcpkg?

N-Dekker avatar Aug 09 '19 17:08 N-Dekker

Those errors do look familiar. You can look at https://github.com/InsightSoftwareConsortium/ITK/issues/579 and stuff linked from there.

dzenanz avatar Aug 09 '19 18:08 dzenanz

You can look at InsightSoftwareConsortium/ITK#579

Thanks @dzenanz ! So what would then happen when ITK_USE_SYSTEM_DOUBLECONVERSION = ON and there is no doubleconversion library on the system? I have no idea if doubleconversion is available "out of the box" on the azure vs2017-win2016 virtual machine, what do you think?

N-Dekker avatar Aug 09 '19 20:08 N-Dekker

That should use double-conversion from vcpkg.

dzenanz avatar Aug 09 '19 20:08 dzenanz

That should use double-conversion from vcpkg.

In the azure pipelines yml file of elastix (of this PR), I just did:

vcpkg.exe install itk

See https://github.com/SuperElastix/elastix/blob/ceafb802df2478ca8dd0b1ba5d6de87da73e27d0/Testing/CI/Azure/ci.yml#L13 Is that sufficient, or should it also have vcpkg.exe install double-conversion ?

N-Dekker avatar Aug 09 '19 20:08 N-Dekker

double-conversion is specified as a dependency in the CONTROL file, so vcpkg.exe install itk should be enough.

dzenanz avatar Aug 09 '19 20:08 dzenanz

@dzenanz I finally got elastix building at azure vmImage 'vs2017-win2016', with ITK 5.0.1 from vcpkg (latest ITK portfile.cmake). But I have been cheating! I added link_directories to the root CMakeLists of elastix, so that the projects could link to "./lib/double-conversion.lib" and "openjp2.lib": https://github.com/SuperElastix/elastix/pull/161/commits/58a2900e66d0551ce7626b5ef4cd6508819941d0

link_directories(
  C:/vcpkg/installed/x64-windows # For lib/double-conversion.lib
  C:/vcpkg/installed/x64-windows/lib # For openjp2.lib
)

I did so to avoid link errors. Do you have a suggestion how to avoid such an ugly workaround?

N-Dekker avatar Aug 12 '19 14:08 N-Dekker

Perhaps execute vcpkg integrate install?

dzenanz avatar Aug 13 '19 14:08 dzenanz

Perhaps execute vcpkg integrate install?

Thanks @dzenanz I gave it a try, https://github.com/SuperElastix/elastix/pull/161/commits/97a013f72aa27c9d0b2382e96a68d9a4d44b886a and it looks like your suggestion almost fixes the problem! It adds "C:\vcpkg\installed\x64-windows\lib" to the linker. Which does contain "double-conversion.lib", the file mentioned in the link error:

fatal error LNK1181: cannot open input file '\lib\double-conversion.lib'.

However, the linker still cannot find the lib file, as it is looking for "\lib\double-conversion.lib". If it was just looking for "double-conversion.lib", it would have found it in "C:\vcpkg\installed\x64-windows\lib". Now it still can't find the file: https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=505

Do you have one more suggestion, please?

N-Dekker avatar Aug 13 '19 19:08 N-Dekker

That might be a bug in vcpkg, or something related to https://github.com/InsightSoftwareConsortium/ITK/issues/579 or https://github.com/google/double-conversion/issues/100.

dzenanz avatar Aug 13 '19 19:08 dzenanz

@dzenanz Update: yesterday I got elastix fully building with ITK 5.0.1 from vcpkg. It appeared essential to do vcpkg integrate install, and also to add msbuildArguments: /p:VcpkgEnabled=true, in order to fix most link errors. Unfortunately, it still appeared necessary to add link_directories(C:/vcpkg/installed/x64-windows), as a "hack", to the elastix CMakeLists, in order to link to lib/double-conversion.lib

For the time being, we could leave that hack in there. But I still find the build times too long: more than two hours for an entire elastix build, of which almost an hour is spent building and installing ITK. I hope the new Azure Pipelines CacheBeta task can be of help, but I'm still looking for an example: https://github.com/MicrosoftDocs/vsts-docs/issues/4988

N-Dekker avatar Aug 15 '19 12:08 N-Dekker