-
Notifications
You must be signed in to change notification settings - Fork 596
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
deps: Add build recipe for PNG #4423
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Required for building redistributable python wheels on Windows Wheels runners. Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
…dation#4429) This test was running very slowly, taking too long in all circumstances, but shockly causing GHA CI Mac runners to hit the test timeout sometimes. It turns out we were doing a lot of redundant work. There was no need to make separate pattern and bayer images for every data type. Instead, hoist the test image generation out of the loop and just make one (float), then use `-i:type=<type>` to read it into an ImageBuf of the appropriate data type. This cuts the time for this test to run by about 3x. Signed-off-by: Larry Gritz <lg@larrygritz.com>
MacOS builds require that PNG_FRAMEWORK=OFF in order to find and install dynamic libraries... ...but Windows still has trouble finding and installing dynamic libraries. Only building static libraries seems to work, as far as building wheels is concerned; but I fear older dynamic libpngs found on the system will be preferred over newer static libpngs freshly-built by OIIO when linking. Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Looks like we're still failing CI here |
indeed... it's the strangest thing. Still trying to get to the bottom of this. I think what's happening is, for systems that have a "too old" version of dynamic PNG libraries installed, now that there's a build recipe available, even if we build newer static PNG libs, the build system is still preferring to link the older shared libraries (even though they're too old). I dunno if this is a clue or a red herring, but it seems the build system might be getting confused about which version of PNG it's found versus the version it's trying to build (at least nominally):
so... in the second line in the above log snippet, it claims to be "Building package PNG 1.5.13 locally", even though it's building 1.6.44. I'm not sure what, if anything, that has to do with the more serious problems:
A lesser problem, but one worth figuring out -- currently, I'm forcing the recipe to only build static libraries, cuz Windows has trouble figuring out what to do with / how to install the dynamic PNG library it just built. (I was having similar trouble on macOS until I set Ideally, we'd only link the static libraries, since that seems to behave properly across platforms, given the opportunity... |
I'm kind of baffled by the CI build failures... I can't seem to pinpoint anything in the logs or the configs that would explain why or how the failures occur on some runs, but not others... The problematic runs are:
And they all fail the same way:
But... considering the rest of the checks pass, it doesn't seem to be an issue with the compiler or compiler version, the VFX202X build image, the version of Python, the version of EXR, the version of OCIO, or whether sse instructions are used. And I can't seem to ascertain a particular constellation of settings that produces a failure. The only thing I could identify that looked suspicious to me is the ordering of paths in the The logs aren't verbose enough for successful builds for me to tell if this is the difference that explains the failures of unsuccessful runs. |
Attempt to force the build system to always find the locally-built PNG before any system-installed PNGs Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
745c6e0
to
aa2565d
Compare
*Now* it should use PNG's CMake Config.... Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
4494a73
to
36fda80
Compare
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
36fda80
to
4802555
Compare
To keep the build system from getting confused and trying to link found-but-too-old libraries if it can't find the recipe's libpng. So, if our libpng can't be found for whatever reason, this should prevent the build system from silently failing over to linking the found-but-too-old libraries. Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
once more, with feeling. Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
8dd92ae
to
0c95b1d
Compare
Okay, this doesn't seem to be working properly -- CMake is having trouble finding the correct PNG library, and my understanding is, the issue stems from PNG's own build system idiosyncracies. The failures above occur on systems with libpng v1.5 installed, which are initially detected as too-old; but are later "refound" after find_package fails to find the libraries under $PNG_ROOT. This seems to be a problem with either kitware's FindPNG.cmake, PNG's CMake config, or both. This said, it looks like the libpng team has been very actively working on improving and modernizing their CMake-based build system recently, seemingly in anticipation of an imminent libpng 1.8 release. Importantly, two weeks ago, libpng v1.6.44 made some improvements and changes to the CMake build system (see pnggroup/libpng#545 among others) -- which is great -- but the difference between 1.6.43 and 1.6.44 seems to have serious implications for how find_package behaves (or fails to behave) and where targets can be found -- namely, we need find_package to consider the CONFIG introduced in 1.6.44, and likewise set PNG_DIR. However, sometimes, the find_package still fails to (re)find the correct libpng library. Locally, I can produce conditions with the VFX2021 image where find_package is able to find the proper self-built libraries... but only with incremental builds that reuse a build directory, and only after a first build attempt that manages to self-build PNG, but still proceeds to link the too-old system dynamic PNG, as described above. We do have other means for finding PNG -- for one, PkgConfig / png_check_module seems to find our PNG without difficulty -- but I understand this might be problematic on Windows. Alternatively, we can use the output of the libpng-config cli tool for setting PNG_LIBRARIES and PNG_INCLUDE_DIRS (and PNG_FOUND)... but setting those values doesn't seem to sufficiently inform the rest of the build process. And I don't really know enough about CMake or building and linking stuff in general to know what exactly to do next. I feel the best solution involves coercing CMake's own Find mechanism to do the right thing. I'm not sure if that means creating a custom FindPNG.cmake module or patching PNG's CMake config, or what; but I suspect the path of least resistance is contingent on the build system improvements the libpng team is currently working on. This PR was initially motivated by Wheels workflow tasks failing on vanilla macos-14 runners; and while this build_PNG.cmake recipe does seem to help with building the macos-arm64 wheels, it obviously introduces problems under other conditions. So, for the time being, we're bypassing the PNG plugin entirely for the Wheels / Build MacOS ARM64 tasks until we can figure out a more robust solution for PNG. |
@zachlewis Any update on this one? |
I'm afraid not. I'll have another look, but I think I might be stuck... The failures occur when the build system detects a too-old version of libpng:
I'm trying to find a way to force the build system to always find and link "our" PNG; but Also, libpng 1.6.43 and 1.6.44 behave very differently. |
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
As requested in #4387.