Skip to content

feat: install osmesa off-screen renderer on Windows runners #29

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

Merged
merged 5 commits into from
Apr 22, 2025

Conversation

RobPasMue
Copy link
Contributor

Although we were using the installer on Windows, we were not providing the osmesa.dll to the systemwide installation. This new implementation fixes it by adding it to the install_opengl.sh script as an optional feature (in case users want to benefit from it). By default, it is not enabled to avoid breaking changes.

It also defines VTK_DEFAULT_OPENGL_WINDOW=vtkOSOpenGLRenderWindow which is needed to force the usage of osmesa.dll based on https://docs.vtk.org/en/latest/advanced/runtime_settings.html

README has been adapted and new action inputs are created.

@RobPasMue
Copy link
Contributor Author

Ubuntu 20.04 runners are no longer generally available AFAIK.

@RobPasMue
Copy link
Contributor Author

RobPasMue commented Apr 22, 2025

Pinging @banesullivan @larsoner @akaszynski for review (sorry for the ping, I don't know if you would get the PR notification otherwise).

Here is a PR were we are making use of this new approach (temporarily, until the fix is in)
ansys/pyansys-geometry#1927

Copy link
Contributor

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Thanks for adding this changes Roberto

Co-authored-by: Sébastien Morais <[email protected]>
@akaszynski
Copy link
Member

Looks like ubuntu 20.04 is set for deprecation. Let's disable that.

@RobPasMue
Copy link
Contributor Author

Looks like ubuntu 20.04 is set for deprecation. Let's disable that.

Just opened #30

@larsoner
Copy link
Collaborator

I think maybe we should have it default to true and cut a v4. Seems like almost all Windows user would want this, no?

@RobPasMue
Copy link
Contributor Author

RobPasMue commented Apr 22, 2025

I think maybe we should have it default to true and cut a v4. Seems like almost all Windows user would want this, no?

If I was using a Windows self-hosted runner or even a Windows GitHub runner, I would be interested indeed -- but I wanted to avoid changing critical behavior without y'all confirming that it is the right way to go =) that's why I went with the false default at first :)

@SMoraisAnsys
Copy link
Contributor

SMoraisAnsys commented Apr 22, 2025

I think maybe we should have it default to true and cut a v4. Seems like almost all Windows user would want this, no?

If I was using a Windows self-hosted runner or even a Windows GitHub runner, I would be interested indeed -- but I wanted to avoid changing critical behavior without y'all confirming that it is the right way to go =) that's why I went with the false default at first :)

I expect multiple maintainer to leverage pyvista/setup-headless-display-action soon since the other alternative of using vtk-osmesa is no longer available. There are no more wheels provided starting from VTK 9.4 and while

OSMesa and EGL support are included by default in the vtk wheels and can be selected at runtime.
we need to have OSMesa installed now, see https://docs.vtk.org/en/latest/release_details/9.4/support-runtime-opengl-window-selection.html

@akaszynski
Copy link
Member

I think maybe we should have it default to true and cut a v4. Seems like almost all Windows user would want this, no?

Let's plan to bump to v4 once merged. Thanks all!

@akaszynski akaszynski merged commit dbce7c2 into pyvista:main Apr 22, 2025
12 checks passed
@RobPasMue
Copy link
Contributor Author

This will need a follow up release. As it is, a minor (v3.3 and v3) should be fine.

@larsoner
Copy link
Collaborator

As it is, a minor (v3.3 and v3) should be fine.

The motivation for major is that we added a behavior change that might break stuff for people. But if we can consider it safely enough as a bugfix then minor is okay

@RobPasMue
Copy link
Contributor Author

As it is, a minor (v3.3 and v3) should be fine.

The motivation for major is that we added a behavior change that might break stuff for people. But if we can consider it safely enough as a bugfix then minor is okay

Oh, yeah, I agree! If we set the default value to true we should put it as a major bump IMO (i.e. v4). However, the PR was merged as is, with the default set to false. I'll let you both decide on the versioning strategy though (@akaszynski @larsoner)

@larsoner
Copy link
Collaborator

My vote is to set to true and bump to v4, @akaszynski WDYT?

@akaszynski
Copy link
Member

My vote is to set to true and bump to v4, @akaszynski WDYT?

Recommending the same. Set to true and bump.

@RobPasMue
Copy link
Contributor Author

Sounds good, I'll do a follow up PR tomorrow - and adapt the README

@larsoner
Copy link
Collaborator

Already opened a PR and cut a release but maybe the README still needs to be updated

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.

4 participants