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

vtk is incompatible with expat 2.6 #321

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Mar 14, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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:

  •         'Test On Native Only' is deprecated.
    

This was used for disabling testing for cross-compiling.

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
Copy link
Contributor

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
Copy link
Member Author

minrk commented Mar 15, 2024

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.

deal with mesalib 24 later
@minrk
Copy link
Member Author

minrk commented Mar 15, 2024

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

@adrianinsaval
Copy link

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
Copy link
Member Author

minrk commented Mar 20, 2024

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
Copy link
Member Author

minrk commented Mar 26, 2024

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 conda-forge/conda-forge-pinning-feedstock#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
Copy link
Contributor

@traversaro
Copy link
Contributor

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
Copy link
Member Author

minrk commented Apr 29, 2024

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

@traversaro
Copy link
Contributor

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
Copy link
Member Author

minrk commented Apr 29, 2024

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.

@traversaro
Copy link
Contributor

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
Copy link
Member Author

minrk commented May 3, 2024

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

@minrk minrk merged commit 1af7955 into conda-forge:main Jun 6, 2024
42 checks passed
@jakirkham
Copy link
Member

jakirkham commented Jun 6, 2024

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

xref: #326

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