Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Use vcpkg to get ITK #161

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

WIP: Use vcpkg to get ITK #161

wants to merge 8 commits into from

Conversation

@N-Dekker
Copy link
Member Author

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
Copy link
Member Author

The missing ITK legacy support should be added by microsoft/vcpkg#7241

@N-Dekker
Copy link
Member Author

N-Dekker commented Aug 8, 2019

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 &)': 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 N-Dekker force-pushed the vcpkg-ITK branch 3 times, most recently from 2b8b064 to da7e61a Compare August 9, 2019 14:09
@N-Dekker
Copy link
Member Author

N-Dekker commented Aug 9, 2019

@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?

@dzenanz
Copy link
Contributor

dzenanz commented Aug 9, 2019

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?

@N-Dekker
Copy link
Member Author

N-Dekker commented Aug 9, 2019

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: microsoft/vcpkg@ac00ef2 I guess I just have to wait.

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

@dzenanz
Copy link
Contributor

dzenanz commented Aug 9, 2019

Those errors do look familiar. You can look at InsightSoftwareConsortium/ITK#579 and stuff linked from there.

@N-Dekker
Copy link
Member Author

N-Dekker commented Aug 9, 2019

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?

@dzenanz
Copy link
Contributor

dzenanz commented Aug 9, 2019

That should use double-conversion from vcpkg.

@N-Dekker
Copy link
Member Author

N-Dekker commented Aug 9, 2019

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

vcpkg.exe install itk

Is that sufficient, or should it also have vcpkg.exe install double-conversion ?

@dzenanz
Copy link
Contributor

dzenanz commented Aug 9, 2019

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

@N-Dekker N-Dekker force-pushed the vcpkg-ITK branch 2 times, most recently from bdaeb33 to 2e0e6a4 Compare August 11, 2019 11:32
@N-Dekker
Copy link
Member Author

N-Dekker commented Aug 12, 2019

@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": 58a2900

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?

@dzenanz
Copy link
Contributor

dzenanz commented Aug 13, 2019

Perhaps execute vcpkg integrate install?

@N-Dekker
Copy link
Member Author

Perhaps execute vcpkg integrate install?

Thanks @dzenanz I gave it a try, 97a013f 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?

@dzenanz
Copy link
Contributor

dzenanz commented Aug 13, 2019

That might be a bug in vcpkg, or something related to InsightSoftwareConsortium/ITK#579 or google/double-conversion#100.

N-Dekker added a commit that referenced this pull request Aug 13, 2019
Fixed by ugly `link_directories` quick-fix.
See also #161 (comment)
@N-Dekker
Copy link
Member Author

@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 added a commit that referenced this pull request Aug 16, 2019
Fixed by ugly `link_directories` quick-fix.
See also #161 (comment)
Hoping to fix:

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

Dženan Zukić @dzenanz informed us that there was some work regarding ITK_USE_SYSTEM_DOUBLECONVERSION between IK 5.0.0 and 5.0.1, which might solve this problem.
Suggested by Dženan Zukić @dzenanz
Fixed by ugly `link_directories` quick-fix.
See also #161 (comment)
Yet another attempt to fix those link errors
Suggested at "Disable automatic integration for a particuar VS project"
microsoft/vcpkg#281
Check if it still compiles with VcpkgEnabled=true
Work around the CMake Error at
https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=971&view=logs&j=8113fa60-6c65-540e-d189-4caa7b0fea60&t=ec6f6503-6d4b-5e80-e979-365ac800d840

> CMake Error at C:/vcpkg/scripts/buildsystems/vcpkg.cmake:493 (_find_package):
>  Could not find a configuration file for package "ITK" that is compatible
>  with requested version "5.1.1".
>
> The following configuration files were considered but not accepted:
>
> C:/vcpkg/installed/x64-windows/share/itk/ITKConfig.cmake, version: 5.1.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants