Fix double-conversion issue in ITK#5115
Conversation
|
@dzenanz, thanks for contributing to vcpkg! Here is failed triplet(x64-uwp) and ports with your PR. |
|
I cannot see the details of Also, ITK pretty much has nothing to do with |
|
@dzenanz, the logs here may be incorrect, could you also update CONTROL revision in See more information in https://github.com/Microsoft/vcpkg/blob/master/docs/maintainers/control-files.md |
|
try #5574 for and updated version of HDF5 |
|
If I rebase on top of that, I still get: |
|
@dzenanz: Looking at the version check of ITK: concerning: So lets see what ITK does. First: next: Next: Lets see what FindHDF5 does with that (from cmake): so what das libminc do: and that is set to the shared variables are a empty the static are filled but before that: This code is doing some magic: It is actually pulling in HDF5 with a find_package call what the code is not intending to do. It wants to set ITKHDF5_EXPORT_CODE_INSTALL and ITKHDF5_EXPORT_CODE_BUILD but is actually also doing the find_package call to HDF5. Before it none of Lets conclude: That is a holy cmake mess.... How should one maintain such a monster? My Branch VTK_8.2.0 does not include commit aa560f1 |
|
Oh, I needed to run
ITK's internal Eigen3 was also a somewhat recent addition, and was probably not tested as part of vcpkg before. Perhaps @phcerdan can provide advice or solution regarding that. |
|
@dzenanz dont use Eigen3 and the VC compiler please .... VS inefficient code with Eigen3 so now it is just fixing the portfile? |
That could be. But I would still like @phcerdan and @hjmjohnson to give opinion about the two issues. These warnings might indicate a real problem in ITK's internal Eigen and VXL. @hjmjohnson vcpkg should be working on Linux now, so you might give it a try. |
|
The Eigen3 installation folder was changed recently: InsightSoftwareConsortium/ITK#567 So probably that's related to that change, and should be good? Not familiar with the portfile. I didn't know the issues in the VC compiler with Eigen3 @Neumann-A ... maybe extra action has to be taken in ITK to avoid using Eigen3 in Windows. |
It is just the Visual Studio Compiler, clang-cl would work fine. You could try #4609 in these cases |
|
@phcerdan This is with yesterday's ITK, so that fix is already included. |
|
just add -DITK_USE_SYSTEM_EIGEN=ON in the portfile and put eigen into the control file as a dependency. should solve the eigen errors |
|
still installing targets: you probably dont want to install the normal eigen targets when ITK_USE_SYSTEM_EIGEN is used. All Other Modules can be found in share/itk/Modules. Probably dont use Eigen3_DIR_INSTALL when ITK_USE_SYSTEM_EIGEN is set seems like an itk issue |
|
It seems indeed, I own the blame! I will have a look tomorrow. Thanks @Neumann-A |
Also added documentation about the logic having two Eigen3 targets, and a minimal CMake example for usage in external modules. Raised in: microsoft/vcpkg#5115 (comment)
|
Hopefully solved in InsightSoftwareConsortium/ITK#580. I removed the installation of any targets when ITK_USE_SYSTEM_EIGEN is ON. |
Also added documentation about the logic having two Eigen3 targets, and a minimal CMake example for usage in external modules. Raised in: microsoft/vcpkg#5115 (comment)
Also added documentation about the logic having two Eigen3 targets, and a minimal CMake example for usage in external modules. Raised in: microsoft/vcpkg#5115 (comment)
|
@phcerdan 580 does not help. I still get: I guess |
c077afd to
956dd66
Compare
|
If you set ITK_USE_SYSTEM_EIGEN it should work. |
|
Depending on |
|
As an update, this is the current status as of 4db83e4:
arm64-windows had no regressions x86-windows had no regressions
x64-uwp had no regressions arm-uwp had no regressions I believe the netcdf issue is flakiness due to it writing to the source directory during configure; I've pushed d101837 to fix that. I've reproduced the hdf5 issue locally and it appears to be caused by CRLF line endings in the .zip file (trying out the .tar.gz to see if that fixes the problem). |
|
@ras0219-msft: Just rebase from master. HDF5 has been switched to tar.gz in 49f59fd because I also had problems with strange line breaks in the zip. sry for wrong mention before |
|
@ras0219-msft: and looking at the changes this pull request should either be closed or resubmitted with actually adressing the original issues. (Probably just updating the portfile of ITK and nothing else) |
|
Rebasing just the ITK-related changes on top of current master sounds good, as it has HDF5-1.10.5. Do you want me to do it, or will somebody else do it? |
|
I'd like to keep the netcdf-c changes as well, but if you'd like to rebase it that would be great! |
…Switch to tar.gz to prevent CRLF line endings.
|
Awesome -- thanks for the PR! |
Fixed by InsightSoftwareConsortium/ITK/pull/366. Closes google/double-conversion#85. Closes #4334. ITK is still not properly compiling due to an out-of-date version of HDF5.