search

ampas / CTL

The Color Transformation Language (CTL)
Other
216 stars 77 forks source link

Support for OpenEXR-3 / Imath? #100

Open waebbl opened 1 year ago

waebbl commented 1 year ago

Hi,

is there a chance, the library will support OpenEXR-3 / Imath in the foreseeable future?

The package is one of very few in Gentoo Linux, which still needs the old OpenEXR-2.5 / IlmBase packages. As a maintainer of these packages, I'm currently checking removal of these versions from the repository.

TIA

lgritz commented 1 year ago

Does anything else in Gentoo use CTL as a dependency? It's a bit of a specialized thing, I'm a little curious how it got swept into a major Linux distro in the first place.

Note: there doesn't appear to have been a tagged release of CTL since 2014?

waebbl commented 1 year ago

No package in Gentoo depends on CTL. It has been in Gentoo for many years and got moved from CVS when we migrated to Git back in 2015. Eventually someone adapted it from BDS ports, on which the Gentoo package manager is based off, but it's origin is currently unknown to me. It's on a few other major distros as well[1][2].

As long as it builds, no major bugs are reported and the package is still maintained, it's age isn't very important.

[1] https://repology.org/project/ctl/versions [2] https://repology.org/project/ampasctl/versions

michaeldsmith commented 1 year ago

Hi - I volunteered to be the maintainer of CTL earlier this year. So far I've focused on fixing the bugs that impacted my own usage of CTL and ctlrender in my work. I also added CI workflows including valgrind and addressanitizer, and I fixed the bugs that were found by those tools.

@waebbl we could work on supporting OpenEXR-3/Imath if you think its important, is your goal to completely remove OpenEXR 2.5.x ? It appears that the OpenEXR 2.5.x branch is still under active maintenance (last release was March 2022).

@lgritz would it help if we made a new 1.5.3 tag release of CTL? Do you think it would need to include anything more than the bug fixes I've already committed to master branch?

lgritz commented 1 year ago

I think we should take a hard look at whether anybody actually needs the Gentoo pre-built CTL package. If not, they should drop it from their distro in an effort to clear the way for them to upgrade to OpenEXR/Imath 3.1 for the rest of their packages that need OpenEXR/Imath.

OpenEXR 2.5 is definitely past its support window. In March 2022 we tagged a release that contained two very minor build-related fixes, probably as a favor for somebody who specifically requested it. The previous 2.5 patch was almost a year earlier and only contained fixes to critical security bugs. Currently, we're working toward 3.2 and I think I can speak for the OpenEXR dev team and say that 2.x should not expect any further updates and it's our position that everybody should be moving to 3.x ASAP.

waebbl commented 1 year ago

@michaeldsmith Sadly I can not say how important the package is. We don't have reliable numbers on how often a package is installed by users. It doesn't hurt to keep IlmBase / OpenEXR-2.5 around for a while longer, as long as the packages build and security issues are getting fixed. If there's a decision to not upgrade CTL to use Imath / OpenEXR-3, we might eventually have to drop CTL from the repo at some point, if IlmBase / OpenEXR-2 don't get any more security updates.

@lgritz We don't have pre-built CTL packages. Gentoo is a source-base rolling release distro. The package also doesn't block upgrades for packages using OpenEXR-3 / Imath. We currently have packages for v3.1 and v2.5, but they can't be installed at the same time. So if someone wants to install CTL it will in fact block installing the many packages which already use OpenEXR-3 / Imath, including packages like blender, krita, osl or oiio. At the beginning of v3 we had a way to install both, v3 and v2.x in parallel, but this was very fragile and we needed to manually change a few things in the build system for either v3 or v2.x, so we dropped this.

A list of packages depending on IlmBase can be seen at [1]. [B] means blocking, i.e. the package can't be installed at the same time with IlmBase. I checked this list and all packages, but CTL also have newer version in the repo which use OpenEXR-3 / Imath, so all of these could be dropped. One other package YafaRay is using OpenEXR-2, but we can disable it by dropping optional support for EXR format until they come up with a newer release.

[1] https://qa-reports.gentoo.org/output/genrdeps/rindex/media-libs/ilmbase

michaeldsmith commented 1 year ago

@waebbl I have merged pull request #103 into CTL master branch which enables building CTL and ctlrender without the OpenEXR v2 library. The IlmBase library is still required. Does that help address your original issue and question? It seems IlmBase is still available from modern ubuntu package mangers. I'm not sure if Gentoo will continue to support IlmBase. Also, I don't know if its possible to keep a legacy IlmBase v2 and a newer OpenEXR v3 on the same system.

@lgritz do you have any interest in submitting a pull request to CTL that would update it to use the latest OpenEXR v3 libraries instead of IlmBase ?

waebbl commented 1 year ago

@michaeldsmith Thanks for your effort in updating the library. It will not help with my original issue though. We still have OpenEXR-2.5 and IlmBase packages in Gentoo, but they are both definitely bound for EOL. There's no set date for this however, and if there's effort to update to OpenEXR-3 I think we can keep them some more time. On Gentoo it's currently not possible to have OpenEXR-3 and IlmBase-2 packages installed on the same system at the same time. AFAIR this will lead to file collisions for some header files which are in both, OpenEXR-3 and IlmBase, like for example Iex.h. If I read your patch in #103 correctly, OpenEXR is only needed for one test suite. This gives me the opportunity to restrict the specific test for the test suite, and build with only IlmBase support. I'm trying to use your patch in my PR https://github.com/gentoo/gentoo/pull/28278 and remove the dependency on OpenEXR.

lgritz commented 1 year ago

Keeping an old version of the "IlmBase" portions of OpenEXR 2.5 in an otherwise Imath-3.x world will cause all sorts of subtle problems with conflicting header files and symbols. Plus, those 2.x versions no longer get updates, even for critical security bugs, which puts any project that uses them in danger of having vulnerabilities discovered that could prevent them from being used in commercial products or being included in OS distributions. CTL really needs to be converted to a modern, maintained version of Imath.

I'm not a good candidate for writing a PR for CTL, both because my time is already wildly oversubscribed, and also I have never built or used CTL and am not really familiar with its codebase -- I'd be starting from scratch. Surely there is somebody better suited than me to do this work.

I did write a porting guide with the specific goal of making it so I wouldn't have to do the conversion myself in other code bases. That was actually done for the 3.0 release when we thought people would need to simultaneously support OpenEXR/Imath 2.x and 3.x for some time. Now that it's a few years later, if you can take the leap and require 3.0 as a minimum, it's even simpler because you won't need all the #if nonsense that handles either version.

michaeldsmith commented 1 year ago

@waebbl I just merged #105 that adds support for OpenEXR-3 and Imath. Can you let me know if this resolves your issue? Do you have any other feedback?

@lgritz thanks for your suggestions and mentioning your porting guide, it was helpful. I would like to maintain support for both OpenEXR2 and OpenEXR3 until all the package managers pick-up OpenEXR3. Please let me know if you have any feedback.

waebbl commented 1 year ago

@michaeldsmith I can test next week. Left a comment on a missing use of the GNUInstallDirs variable.

waebbl commented 1 year ago

@michaeldsmith I can successfully build the package at commit https://github.com/ampas/CTL/commit/3fc4ae7a8af35d380654e573d895216fd5ba407e and the tests run successfully using Imath-3.1.6 and OpenEXR-3.1.5.

Some feedback for further improvements on the current build files:

waebbl commented 1 year ago

You can pick a patch for the above mentioned changes from https://github.com/gentoo/gentoo/pull/28906/files#diff-d01076278b117f6aad20b60e7cce7f7712310822e8d401e376b1539051fd2d46 if you like. I you prefer, I can also open a pull request with the changes.

michaeldsmith commented 1 year ago

@waebbl thanks for the additional review. Can you please submit a pull request that fixes the issues you have found? I would also be interested to hear if you have any additional ideas about cleaning up the build and configuration files.

Also, it could help me test with Gentoo if you could post a Gentoo Dockerfile to this issue that can be used for testing CTL with Gentoo using Docker. I could also try adding a Gentoo Dockerfile to the Github workflow CI.

waebbl commented 1 year ago

@michaeldsmith I submit a PR in the next few days and notice you if I have any ideas on cleaning up the build.

It's been an extended time, since I last used docker, but I see what I can do to provide a Dockerfile for you to test the build. I'm not sure though if it's meaningful to add it to the GitHub CI workflow, but it's up to you. Gentoo isn't a mainstream distribution, and not widely used by the average user. Common Use Cases for Gentoo include people who like to build the system on their machine, in contrast to installing pre-built packages, and people who like to have an individual setup in terms of compiler configuration and needed / installed dependencies for packages.

waebbl commented 1 year ago

@michaeldsmith The search for IlmBase / Imath and OpenEXR seems to be overcomplex. Currently there are several checks if Imath or IlmBase are found, as well if we have =OpenEXR-3 available.

From my test, this could be simplified to only check for OpenEXR and provide a base version to the find_package call. OpenEXR itself, depends on either Imath (>=OpenEXR-3) or IlmBase (<OpenEXR-3) and checks for the availability of the respective package. So I think, it's enough to only look for OpenEXR and leave the details regarding Imath / IlmBase to that package. I've done some patching in my PR. Let me know, what you're thinking about it. I can do some rework of this, if needed and am happy for feedback on this.

michaeldsmith commented 1 year ago

@waebbl I think we have addressed this issue with our recent pull requests #105 and #115, if you agree, please go ahead and close your issue. If your issue is still not addressed, please describe what remains to be fixed.

waebbl commented 1 year ago

The issue is fixed. I left it open for me as a reminder for the yet to do Dockerfile you've asked for.

michaeldsmith commented 1 year ago

OK sounds good - I added other Dockerfiles in the repo here https://github.com/ampas/CTL/tree/master/docker and the docker github CI workflow file is here https://github.com/ampas/CTL/blob/master/.github/workflows/docker_linuxes.yml