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

[vtk-dicom] Fix cmake path #12839

Closed
wants to merge 1 commit into from

Conversation

JackBoosY
Copy link
Contributor

Fixes #12813.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Aug 10, 2020
@JackBoosY JackBoosY requested a review from PhoebeHui August 10, 2020 06:22
@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Aug 10, 2020
@@ -32,7 +32,7 @@ vcpkg_configure_cmake(
)

vcpkg_install_cmake()
vcpkg_fixup_cmake_targets(CONFIG_PATH lib/cmake)
vcpkg_fixup_cmake_targets(CONFIG_PATH lib/cmake/vtk TARGET_PATH share/DICOM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the right fix. Could it be that the correct find_package call is find_package(VTK COMPONENTS DICOM). Also the port vtk-dicom is up for removal since it is planned to be integrated into VTK

Copy link
Contributor

@ras0219 ras0219 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the output packages:

vtk-dicom:/share/DICOM/DICOM-targets-debug.cmake
vtk-dicom:/share/DICOM/DICOM-targets-release.cmake
vtk-dicom:/share/DICOM/DICOM-targets.cmake
vtk-dicom:/share/DICOM/DICOM-vtk-module-properties.cmake
vtk-dicom:/share/dicom-0.8/Copyright.txt
vtk-dicom:/share/vtk-dicom/copyright
vtk-dicom:/share/vtk-dicom/vcpkg_abi_info.txt

it doesn't look like there is a DICOM-config.cmake, so find_package() won't work. Maybe we need to generate a DICOM/DICOM-config.cmake or maybe we need to use TARGET_PATH share/vtk. Given that the original path is lib/cmake/vtk, I think that TARGET_PATH share/vtk is probably correct and we need to add a usage file for find_package(VTK COMPONENTS DICOM).

@JackBoosY
Copy link
Contributor Author

@ras0219 @ras0219-msft I think what @Neumann-A said is very reasonable, the correct way is to add a usage to inform users how to use this port.

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Aug 11, 2020
@JackBoosY JackBoosY marked this pull request as draft August 11, 2020 09:09
@JackBoosY
Copy link
Contributor Author

According to the official docs, the usage does not work:

find_package(DICOM QUIET)
if(DICOM_FOUND)
  include(${DICOM_USE_FILE})
endif()
set(VTK_DICOM_LIBRARIES vtkDICOM)

It seems that cmake does not provide FindDICOM.cmake, but provides FindDCMTK.cmake.

@qaler
Copy link

qaler commented Aug 16, 2020

I can't include vtk-dicom in my project using neither:

find_package(DICOM QUIET)
if(DICOM_FOUND)
  include(${DICOM_USE_FILE})
endif()
set(VTK_DICOM_LIBRARIES vtkDICOM)

nor :

#recommended usage by vcpkg when installation finished
find_package(vtk CONFIG REQUIRED)
target_link_libraries(main PRIVATE VTK::DICOM)

Could you please point out a reasonable way to include vtk-dicom library?

@JackBoosY
Copy link
Contributor Author

@dgobbi Could you please take a look?

Thanks.

@dgobbi
Copy link

dgobbi commented Aug 18, 2020

@JackBoosY right now vtk-dicom only creates "DICOMConfig.cmake" when built with VTK 8. I will fix it so that it creates one with VTK 9, but it will be a few days before I have time to do so.

@JackBoosY
Copy link
Contributor Author

@dgobbi once your PR merged, I will close this PR.

@QiuKejian
Copy link

Found a temporary solution:
step 1:
cwd: ${VCPKGROOT}\installed\x64-windows\share
copy the content of .\vtk-dicom\vtk into .\vtk
step 2:
modify .\vtk\vtk-config.cmake at about line 127, add the following lines:
include("${CMAKE_CURRENT_LIST_DIR}/DICOM-targets.cmake")
include("${CMAKE_CURRENT_LIST_DIR}/DICOM-vtk-module-properties.cmake")

image

@JackBoosY
Copy link
Contributor Author

Temporary close this PR, will continue this work in the future.

@drescherjm
Copy link

This seems to be still an issue unless I am doing something wrong.

@drescherjm
Copy link

drescherjm commented Mar 9, 2022

Can someone please attempt to fix this bug or get the vtk-dicom module to work inside the vtk port? I tried enabling the module but it fails because a file is overwritten in the download of the external module. I could not figure out how to get around that error. Every time I build vtk I need to do the steps that @QiuKejian mentioned above which does work but is a bit of manual work.

@JackBoosY
Copy link
Contributor Author

@dgobbi Any progress?

@dgobbi
Copy link

dgobbi commented Apr 21, 2022

I've added dicom-config.cmake to the vtk-dicom master branch, please let me know if anything else is needed.

@JackBoosY
Copy link
Contributor Author

Will continue this PR in the next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vtk-dicom] find_package() can not find vtk-dicom
8 participants