conda-forge / vtk-feedstock

A conda-smithy repository for vtk.
BSD 3-Clause "New" or "Revised" License
13 stars 64 forks source link

vtk is incompatible with expat 2.6 #321

Closed minrk closed 3 months ago

minrk commented 6 months ago

expat 2.5 run_exports accepts >=2.5,<3, so expat 2.6 is being installed with current builds. I'll do a repodata patch PR separately.

upstream issue: https://gitlab.kitware.com/vtk/vtk/-/issues/19258

test added from https://gitlab.archlinux.org/archlinux/packaging/packages/paraview/-/issues/4#note_166231

conda-forge-webservices[bot] commented 6 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

conda-forge-webservices[bot] commented 6 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

This has been deprecated in favor of the top-level `test` field.
It is now mapped to `test: native_and_emulated`.
        Failed validating 'deprecated' in schema['properties']['test_on_native_only']:
            {'anyOf': [{'type': 'boolean'}, {'type': 'null'}],
             'default': False,
             'deprecated': True,
             'description': 'This was used for disabling testing for '
                            'cross-compiling.\n'
                            '\n'
                            '```warning\n'
                            'This has been deprecated in favor of the top-level '
                            '`test` field.\n'
                            'It is now mapped to `test: native_and_emulated`.\n'
                            '```',
             'title': 'Test On Native Only'}

        On instance['test_on_native_only']:
            True
conda-forge-webservices[bot] commented 6 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

minrk commented 6 months ago

arg, osmesa has had a major release since the last build, and it breaks with:

024-03-14T09:13:52.9684688Z In file included from /home/conda/feedstock_root/build_artifacts/vtk_1710405337176/work/ThirdParty/glew/vtkglew/src/glew.c:41:
2024-03-14T09:13:52.9685854Z /home/conda/feedstock_root/build_artifacts/vtk_1710405337176/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/include/GL/osmesa.h:125:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'OSMesaCreateContext'
2024-03-14T09:13:52.9686578Z   125 | OSMesaCreateContext( GLenum format, OSMesaContext sharelist );
2

This was reported upstream in https://gitlab.freedesktop.org/mesa/mesa/-/issues/10270 but appears to be fixed and doesn't appear to be supposed to affect linux builds anyhow.

I don't really understand how there's an issue because GLAPIENTRY and APIENTRY are both defined to be empty in GL/gl.h on linux.

I don't know if the right -DAPIENTRY= or something will fix it, or if we should just pin mesa to 23.

minrk commented 6 months ago

Had to pin osmesa to get builds to pass, can revert the pin in a new PR after merge.

adrianinsaval commented 6 months ago

alternatively we could remove expat from dependencies and comile VTK with -DVTK_MODULE_USE_EXTERNAL_VTK_expat=OFF, sounds like that's what they're doing in archlinux

minrk commented 6 months ago

Yeah, in general we shouldn't vendor dependencies on conda-forge, but that's a valid temporary workaround. We'd need to make sure to include the expat license in about.license_files.

If expat is installed and dynamically linked, this would be a problem as it would still conflict just as much with existing expat installs, but if it's statically linked that should be okay temporarily.

minrk commented 5 months ago

fwiw, rereading the arch issue, I think folks have been showing that the bundled expat works, but they don't seem to be proposing to actually do it in the package.

My inclination is to go with this pin for now, and https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5640 if we need it to avoid conflicts across conda-forge, and watch https://gitlab.kitware.com/vtk/vtk/-/issues/19258 for a patch we can backport.

Any opinion from @conda-forge/vtk ?

traversaro commented 4 months ago

A fix was released upstream: https://gitlab.kitware.com/vtk/vtk/-/commit/db8f9efca220c9d16a30958e179abae3379d0011 .

traversaro commented 4 months ago

A fix was released upstream: https://gitlab.kitware.com/vtk/vtk/-/commit/db8f9efca220c9d16a30958e179abae3379d0011 .

At a first glance, that does not seem ABI compatible with existing releases.

minrk commented 4 months ago

OK, so looks like we need to keep pinning until the next vtk release?

traversaro commented 4 months ago

OK, so looks like we need to keep pinning until the next vtk release?

Yes, from what I see that is the case, unless there is some way (that I do not see) to make the fix ABI compatible.

minrk commented 4 months ago

WDYT are the implications of that for https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5640 ? Is vtk's compatibility enough that we need to hold everyone else back on conda-forge?

EDIT: I guess it doesn't really hold anything back, just the build-time version.

traversaro commented 4 months ago

WDYT are the implications of that for conda-forge/conda-forge-pinning-feedstock#5640 ? Is vtk's compatibility enough that we need to hold everyone else back on conda-forge?

EDIT: I guess it doesn't really hold anything back, just the build-time version.

Until someone needs a specific feature available only on expat 2.6, that pin seems fine to me. In case anyone needs explicity expat 2.6, they can always override locally the pin in the recipe.

minrk commented 4 months ago

This PR (or something like it) needs to land before any other PRs to this repo (#324, #322) , which would result in reintroducing broken dependencies. I'll merge it as-is to unblock other things, unless there's an objection

jakirkham commented 3 months ago

Looks like the macOS builds are failing on main. Maybe we need a re-render or something?

xref: https://github.com/conda-forge/vtk-feedstock/issues/326