search

conda-forge / vtk-feedstock

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

Optionally build with ffmpeg because of patenting issues #282

Closed basnijholt closed 1 year ago

basnijholt commented 1 year ago

Because of patenting issues in FFmpeg, I cannot install VTK for anything work-related. I imagine many others are also potentially having this issue (although likely without knowing this.)

Here I create a variant that creates an additional variant where FFmpeg is not installed.

This PR leaves the build string of the version with FFmpeg unchanged, however, adds a variant *noffmpeg.

cc @jgukelberger

Checklist

conda-forge-webservices[bot] commented 1 year 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.

basnijholt commented 1 year ago

Can anyone review this PR? 😄

jakirkham commented 1 year ago

Are you aware that there already are separate builds of ffmpeg?

cc @conda-forge/ffmpeg

hmaarrfk commented 1 year ago

Can you confirm that the LGPL build of ffmpeg is not enough for you?

hmaarrfk commented 1 year ago

A few comments:

  1. Really hope that you consider creating your own company (just noticed you work at Microsoft, so maybe "subgroup" ;) ) infrastructure to get this package useful to you while the review process goes on at conda-forge.

  2. A few people (myself included) have gone down this licensing / patenting rabbit hole. The truth is, that FFMPEG is "big" and "grabs attention", but maybe you should consider that it is likely that the same patents that you might be infringing on for using FFMPEG (parallelized image processing), could also be at play with VTK?

  3. I took time to look through the logs, and noticed that ffmpeg libraries (libavcodec and the like) are linked to a single file:

2023-01-09T20:10:29.7256834Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libavformat.so.59 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105
2023-01-09T20:10:29.7339925Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libavcodec.so.59 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105
2023-01-09T20:10:29.7422791Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libavutil.so.57 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105
2023-01-09T20:10:29.7505743Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libswscale.so.6 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105

For conda-forge's benefit (and this is why I suggest you release your own packages on your own channel), I think that the proliferation of "variants" is really difficult to maintain, and to teach users about. As the author of the lgpl/gpl variants of ffmpeg, I'll justify it due to the "infectiousness" of GPL. Could it be, that for your needs, you create:

  1. A subpackage for the "base" of VTK.
  2. A subpackage for the library (libvtkIOFFMPEG) vtk-io-ffmpeg
  3. Keep the parent package depending on both?

This generally limits the build time, and keeps discoverability high (its easier to find packages than variants given the anaconda.org/conda tools).

Anyway just a few thoughts for you and other VTK maintainers to consider.

hmaarrfk commented 1 year ago

I also meant to provide an example on how to implement the split package idea.

In the following qt PR, I am suggesting creating a package for an optional feature of the qt package that could help a subset of users

https://github.com/conda-forge/qt-main-feedstock/pull/98

basnijholt commented 1 year ago

@jakirkham, yes, I am aware of the GPL and LGPL builds of FFmpeg. Unfortunately, both contain the AV1 and VP9 codec which we cannot install.

@hmaarrfk, thanks a lot for your reply and suggestions. Having thought about the options, it seems like the subpackage idea might be suboptimal. For example, we rely on VTK via PyVista, making the parent VTK package depend on two subpackages (vtk-base and vtk-io-ffmpeg) means I would then have to convince the PyVista feedstock maintainers to only depend on the vtk-base. There are likely also other packages one might want to install that depend on VTK.

I agree with your discoverability concerns, however, having a variant seems to be the correct way of solving this problem.

hmaarrfk commented 1 year ago

having a variant seems to be the correct way of solving this problem.

I've found that variants are needed when there is no way to disentangle the libraries. Here it seems like there is hope. For example, I made the GPL/LGPL since FFMPEG didn't seem to allow that split otherwise.

There have been issues in the past in adding too many "globs" on build strings. For example, i still don't think that https://github.com/conda-forge/abseil-cpp-feedstock/issues/46#issue-1360800935 is resolved.

I tried to check what links to the the libvtkIOFFMPEG-9.2.so and it doesn't seem like any library has a hard dependency on it within VTK. 

ldd *.so > ldd.out.txt

yields ldd.out.txt maybe you can try

rm ${CONDA_PREFIX}/lib/ibvtkIOFFMPEG-9.2.*

on your non-sensitive environments and see how well that works. That should give you information on the viability of the "split" package approach.

I would then have to convince the PyVista feedstock maintainers

This is the challenge of working in a community. This somewhat happened with matplotlib -> matplotlib-base transition (removing the hard qt dependency). It took time, but eventually people got on board. Given you are adept with conda-forge infrastructure, I do think that you can make a successful plea to the package you need in your stack because you can help the maintainers implement the transition.

Truthfully, I don't think conda has a good way to transition packages like debian does

To overcome this in the short term, I strongly recommend making your own channel to move yourself forward on this front. Azure can allow you to build up what you need with little friction from conda-forge forge tooling. Fork -> Add your channel information, and rerender.

Summary

Ultimately, I personally think that using build strings is the wrong approach unless it can be shown that VTK directly depends on the ffmpeg extension once it is compiled it at build time.

The proposed approach permanently doubles the number of builds and maintenance burden.

All said, I really won't stop you and other VTK maintainers from moving this forward if you feel it is important.

PS. I've sat on a similar PR for opencv (https://github.com/conda-forge/opencv-feedstock/pull/206) and (https://github.com/conda-forge/opencv-feedstock/pull/337) for 2-3 years that I just haven't been excited to move on for this reason.

basnijholt commented 1 year ago

@hmaarrfk, thanks a lot for your detailed message.

We have discussed the options and investigated whether the proposed approach will work or not.

Ultimately, I personally think that using build strings is the wrong approach unless it can be shown that VTK directly depends on the ffmpeg extension once it is compiled it at build time.

Unfortunately, building sub-packages (e.g., vtk-io-ffmpeg) won't work because CMake dynamically generates Python code and when compiled with FFmpeg, the Python code will always import it.

Resulting in:

image

AFAIU this means that this can only be solved by compiling with or without the feature (so build strings).

Given the approval 👍 by one of the recipe maintainers (@Tobias-Fischer) but the hesitance 🤔 from @hmaarrfk, we could perhaps democratically solve this.

Could some (one?) of the other @conda-forge/vtk maintainers weigh in here (which would allow us to make a decision)?

hmaarrfk commented 1 year ago

democratically solve this.

I think you all can do what is best for VTK.

won't work because CMake dynamically generates Python code and when compiled with FFmpeg, the Python code will always import it.

Given you've already done the hard work of finding where the python code is generated. I would consider one last option:

with 

from contextlib import suppress

at the top.

I would then work to submit such a patch (or a version of it) upstream explaining the packaging and slicing challenge of VTK (which I think the smart folk at kitware are well aware of). But given the uniqueness of the conda-forge packaging

tbh, if I thought such a solution were possible for opencv, I would patch it an merge today. You've done great work exploring the limitations of the current VTK build system!

This one liner i learned today from: https://stackoverflow.com/a/52020518

hmaarrfk commented 1 year ago

So for what its worth, i tried to "debug" this recipe with conda debug and i couldn't even create the environment locally. it seemed to look for *=cuda.

I guess that is as far as I have time to go. I understand if it is also as far as you are willing to go too.

jakirkham commented 1 year ago

Sounds vaguely similar to issue ( https://github.com/conda-forge/nvidia-apex-feedstock/issues/29 ). Given Mamba doesn't have the same problem ( https://github.com/conda-forge/admin-requests/pull/674#issuecomment-1423419582 ), wonder if Conda has a bug

hmaarrfk commented 1 year ago

So I did a little more digging this morning, and it seems that the VTK maintainers are likely open to a try/except patch.

See: https://gitlab.kitware.com/vtk/vtk/-/issues/18365#note_1079278

Just wanted to point that out.

basnijholt commented 1 year ago

@conda-forge-admin, please rerender

conda-forge-webservices[bot] commented 1 year ago

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

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

basnijholt commented 1 year ago

superseded by https://github.com/conda-forge/vtk-feedstock/pull/287

hmaarrfk commented 1 year ago

Thanks for working through all the challenges