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

Remove points #1534

Merged
merged 13 commits into from Apr 8, 2022
Merged

Remove points #1534

merged 13 commits into from Apr 8, 2022

Conversation

danlipsa
Copy link
Collaborator

@danlipsa danlipsa commented Mar 9, 2022

No description provided.

@danlipsa
Copy link
Collaborator Author

danlipsa commented Mar 9, 2022

Needs Kitware/fletch#699

@danlipsa
Copy link
Collaborator Author

danlipsa commented Mar 9, 2022

@mleotta Please review. How do I get a user name for Jenkins to see the issue with the linux build?

@mleotta
Copy link
Member

mleotta commented Mar 9, 2022

I think the linux CI machine might just be down. I'm seeing a similar failure on other PRs. @dstoup or @Cookt2 can you confirm?

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

config/applets/color_mesh.conf Outdated Show resolved Hide resolved
config/applets/color_mesh.conf Outdated Show resolved Hide resolved
config/applets/color_mesh.conf Outdated Show resolved Hide resolved
@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@mleotta
Copy link
Member

mleotta commented Mar 14, 2022

jenkins test this please

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@mleotta
Copy link
Member

mleotta commented Mar 15, 2022

@danlipsa this is failing because VTK 9.1 is not the default on the CI machines. Either I need to get consensus to make it the default or we need to conditionally disable these new features if building with older VTK.

@danlipsa
Copy link
Collaborator Author

I see. Yes, the filter I use is not available for VTK < 9.1

@mleotta
Copy link
Member

mleotta commented Mar 15, 2022

It seems that we need to update this file:
https://github.com/Kitware/kwiver/blob/master/CMake/kwiver-depends-VTK.cmake
to enforce that KWIVER requires at least VTK 9.1. Currently we can build against > 8.0.

The alternative is to find away to disable the new features you added when using older VTK or to use an alternate implementation that does not need VTK 9.1.

@danlipsa
Copy link
Collaborator Author

@mleotta Are the CIs updated?

@danlipsa
Copy link
Collaborator Author

jenkins test this please

@danlipsa
Copy link
Collaborator Author

@mleotta So now I compile color_mesh only if we have VTK >= 9.1. This way we keep visgui working for older versions of VTK. Would that work?

@kwcvrobot
Copy link
Collaborator

Copy link
Member

@mleotta mleotta left a comment

Choose a reason for hiding this comment

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

See inline comments.

I think disabling color-mesh for older VTK is a reasonable compromise. Disabling only the use of vtkRemovePolyData within color-mesh would be better, but also a lot messier.

arrows/vtk/applets/color_mesh.cxx Outdated Show resolved Hide resolved
arrows/vtk/applets/color_mesh.cxx Outdated Show resolved Hide resolved
arrows/vtk/mesh_coloration.cxx Outdated Show resolved Hide resolved
@kwcvrobot
Copy link
Collaborator

@chetnieter
Copy link
Member

Jenkins test this please

@chetnieter
Copy link
Member

All the build failures say 'Build step 'Execute shell' marked build as failure' at the very end despite the build succeeding and the tests passing.

@mleotta
Copy link
Member

mleotta commented Apr 7, 2022

Jenkins test this please

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@chetnieter
Copy link
Member

@mleotta Tests passed so I assume we are ok to merge?

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.

None yet

4 participants