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] fix finding glew issue #7956

Closed
wants to merge 1 commit into from

Conversation

PhoebeHui
Copy link
Contributor

@PhoebeHui PhoebeHui commented Aug 29, 2019

Glew find failed when CMake upgrade to 3.15.2, since FindGLEW.cmake changed in CMake 3.15.2, related issue #7519

VTK has CMake/FindGLEW.cmake, but it removed by 616cfb0ef due to VTK debug was build linked against the release glew library, however this issue fixed in VTK, so I think we should add that back instead of adding others function.

I checked that glew debug library correctly linked in vtk now.

@PhoebeHui PhoebeHui added the info:internal This PR or Issue was filed by the vcpkg team. label Aug 29, 2019
@Neumann-A
Copy link
Contributor

The Problem is the new FindGLEW does not define the variables it promises to define. So the error is on CMake here and the fix in this PR only fixes the VTK issue and no other ports or external cmake projects which still requires those variables. As such I made #7967 to fix the real underlying problem which should work for all dependent projects.

@PhoebeHui
Copy link
Contributor Author

Thanks for the fix!

@PhoebeHui PhoebeHui closed this Aug 29, 2019
@PhoebeHui PhoebeHui reopened this Aug 30, 2019
@PhoebeHui
Copy link
Contributor Author

PhoebeHui commented Aug 30, 2019

@Neumann-A, thanks for fixing the glew issue, I consider CMake/FindGLEW.cmake in vtk still need add back, what do you think?

@Neumann-A
Copy link
Contributor

@PhoebeHui Why do you think it needs to be added back in? I don't see a reason for it. I also checked the current version from here https://github.com/Kitware/VTK/blob/master/CMake/FindGLEW.cmake and cannot see a reason to add it back into vtk. Config+wrapper seems to be superior to whatever VTK tries to accomplish

@PhoebeHui
Copy link
Contributor Author

I just think the original reason to remove cmake/FindGLEW.cmake doesn't exsit now, and if add that back, if vtk has changes, vcpkg change accordingly.

@Neumann-A
Copy link
Contributor

If you add it back in the debug build will link wrongly against the release version of glew (debug version has an extra d suffix). That is the reason why it was removed and this reason did not disappear.

@cbezault cbezault self-assigned this Aug 30, 2019
@PhoebeHui
Copy link
Contributor Author

@Neumann-A, thanks, your're right, I double checked, it still linked wrong dll, it's my fault to check the build results in another machine which build with previous cmake version, very appreciate for pointing out this!

@PhoebeHui PhoebeHui closed this Sep 5, 2019
@PhoebeHui PhoebeHui deleted the dev/Phoebe/fixvtk branch September 5, 2019 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants