-
Notifications
You must be signed in to change notification settings - Fork 285
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 9.3: Fix test suite failures (VTK API changes) #1290
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me!
@prabhuramachandran would you be up for making me a maintainer so I can at least merge PRs that keep things working for packaging like this and #1291 ? I wouldn't want to cut releases or anything like that but we could at least keep master
green hopefully!
Hi @larsoner -- I am terribly sorry I have not been able to do much. I have a real baby these days. And over the last couple of weeks have been traveling so have had little or no time for this. I would be very happy to have you become a maintainer. Thank you for asking and all your work! |
@larsoner - I am not sure what the correct way of making you a maintainer is but will ping the folks at Enthought. @dpinte , @rahulporuri -- Can you please add @larsoner as a maintainer? Thanks! |
Whatever scope would allow me to merge PRs (like this), change branch protection rules (which need to change with #1291), and cut releases would be good. From the GitHub docs it sounds like this would be admin rather than maintainer at least for the branch protection part 😱 . I promise to be careful, but if that's (understanably) too much power to delegate for now then "maintainer" is already good start! |
@prabhuramachandran Eric has been added as maintainer on the project. @larsoner you now have maintainer rights on the repo! Once you've accepted the invite, we'll be able to tweak the branch protection to allow you to merge the PRs (that won't require you to be an admin, I think). |
@bnavigator okay with you for me to close this one and give you co-author credit on #1291 ? CIs are green over there so it's a bit simpler that way |
@dpinte yes I have a merge button now, which is nice. As expected, though, I can't change the branch protection rules. Can you update the branch protection rules to reflect the ones in #1291 ? FWIW I'd expect these to need updates every 6-12 months based on new Python and VTK versions coming out. There will most likely also be a one-off special update required in the next 6 weeks because the Cythonized file we ship will probably need to be updated (i.e., re-Cythonized with an updated Cython) to be NumPy 2.0 API compliant as it's not backward compatible and that should probably be a separate CI run (though strictly speaking doesn't have to be I suppose). And then I'll also need a review approval to merge #1291 (or you could give me the "bypass branch protections" permissions in my role) as any PR requires 1 approval. This will be the case for any other PRs I open, so could slow things down since the primary problem here seems to be getting other reviewers/maintainers. (If I open a PR I can't be the one to also approve it.) As long as there is someone I can ping for branch protection rule updates and PR reviews things should run smoothly. If there isn't... then I fear we'll end up with PRs getting stuck quite a bit again :( |
@larsoner -- if you need something from me, please feel free to ping me here and I can share other means of contacting me. Last month was difficult but going forward I think at least over the weekends I should be able to make some time for maintenance. |
Superseded by #1291 |
We encounter a few test suite errors in the rpm build for Mayavi since VTK has been updated to 9.3.0 in openSUSE Tumbleweed.
This is by far not complete in terms of deprecations and removals for 9.3 (See #1286 for example), but at least it greens the test suite for us.