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

MAINT: Require VTK 9+ #1329

Merged
merged 15 commits into from
Dec 19, 2024
Merged

MAINT: Require VTK 9+ #1329

merged 15 commits into from
Dec 19, 2024

Conversation

larsoner
Copy link
Collaborator

@larsoner larsoner commented Dec 17, 2024

Workaround for https://gitlab.kitware.com/vtk/vtk/-/issues/19562, but also pin fixes to a max version so that over time hopefully the shims die.

Speaking of which, it might be worth adding a min pin on VTK. Some code could be cleaned up by depending on vtk 9+, which is four years old now. Same thing with dropping Python 3.9, which most projects have done nowadays. Happy to add those here if desired.

EDIT: Also restructures CIs explicitly to use latest VTK + NumPy + Qt bindings (preferring official PySide6) available, and comments about old ones.

@larsoner
Copy link
Collaborator Author

Yikes unhashable type issues 😱

I'll have to figure out how they used to be hashed

* upstream/master:
  More fixes to get builds working.
  MAINT: Fix various VTK-9.4 related issues.
  BUG: Fix issue with vtkGenericCell.GetCellFaces.
@larsoner
Copy link
Collaborator Author

@prabhuramachandran okay to make the minimum VTK 9+? It is over 4 years old now, and the last 8.x release on PyPI only has wheels up through 3.7, which we don't even support anymore.

@larsoner larsoner changed the title MAINT: Compat for vtk 9.4 MAINT: Require VTK 9+ Dec 18, 2024
@larsoner
Copy link
Collaborator Author

larsoner commented Dec 18, 2024

We would get even more cleanups if we required Python 3.10 and VTK 9.2 (2 years old) or at least VTK 9.1 (3 years old), but for now we at least get some by requiring VTK 9+ (4 years old).

@larsoner
Copy link
Collaborator Author

groups:
actions:
patterns:
- "*"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noticed this repo didn't have one of these but it's very convenient to have. It'll tend to run CIs once a month or every couple of months which is nice to have to check in. Not to mention actually having the actions stay up-to-date :)

@larsoner
Copy link
Collaborator Author

Okay I think this one should be ready @prabhuramachandran !

@prabhuramachandran
Copy link
Member

We would get even more cleanups if we required Python 3.10 and VTK 9.2 (2 years old) or at least VTK 9.1 (3 years old), but for now we at least get some by requiring VTK 9+ (4 years old).

I am not sure if there are any Enthought internal projects using older VTK. @dpinte, @mdickinson -- what do you think? Can we start requiring VTK 9+ and also Python 3.10? Thanks.

Copy link
Member

@prabhuramachandran prabhuramachandran left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@prabhuramachandran
Copy link
Member

I think @dpinte is on leave but he did tell me that it is OK to move ahead. In any case for older versions, there are older releases of Mayavi.

@prabhuramachandran
Copy link
Member

@larsoner -- should I disable the required statuses before merging here?

@prabhuramachandran
Copy link
Member

@larsoner -- I will merge this for now, perhaps you can adjust the required tests if you have the permissions or let me know which ones you recommend that we keep.

@prabhuramachandran prabhuramachandran merged commit 8aa0e7e into enthought:master Dec 19, 2024
16 checks passed
@larsoner larsoner deleted the vtk branch December 19, 2024 18:08
@larsoner
Copy link
Collaborator Author

@prabhuramachandran I don't have permissions to change the branch protection rules, but I would just set them to the set of runs that were in this PR when it was merged

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