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

Adsk contrib - Fix issue with minizip build #1725

Conversation

cedrik-fuoco-adsk
Copy link
Contributor

This is the PR for Impossible to build against system/external minizip-ng.

I removed the minizip-ng subfolder part when including minizip-ng's headers. This fixes the issue with external builds that don't use that subfolder.

For internal builds, a subfolder is created like the other modules that OCIO is able to download and install manually.

I've decided that having Findminizip-ng.cmake and Findminizip.cmake is less confusing than having only a Findminizip.cmake since "minizip" is not the actual name of the library (minizip-ng without MZ_COMPAT=ON).

Therefore, the build system does the following:

  1. Search for minizip-ng (libminizip-ng) (without MZ_COMPAT=ON)
  2. If minizip-ng is not found, Search minizip-ng with MZ_COMPAT=ON (which is libminizip).
  3. If steps 1 and 2 are not found, download and install minizip-ng with MZ_COMPAT=OFF.
    It is set as OFF because we don't need the compatibility part. We are using the "new" API from minizip-ng.

…arch for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.

- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
@remia remia linked an issue Nov 14, 2022 that may be closed by this pull request
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>
…sing the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>
@cedrik-fuoco-adsk
Copy link
Contributor Author

Note that the analysis workflow is failing, but it was failing before already. The PR does not introduce any new problems with the analysis workflow.

@doug-walker
Copy link
Collaborator

@remia @michdolan , could one of you review this? Merging this would simplify an upcoming PR for us. TIA!

@remia
Copy link
Collaborator

remia commented Dec 3, 2022

Looks good to me, thanks for updating the Analysis workflow! One thing I noticed while testing on macOS, the build with install ALL externals deps works perfectly but when using the minizip-ng from brew, it seems the mz_strm_zlib.h header is missing from /usr/local/include (all the other minizip include are present though). Does anyone else have this issue?

@cedrik-fuoco-adsk
Copy link
Contributor Author

cedrik-fuoco-adsk commented Dec 5, 2022

Looks good to me, thanks for updating the Analysis workflow! One thing I noticed while testing on macOS, the build with install ALL externals deps works perfectly but when using the minizip-ng from brew, it seems the mz_strm_zlib.h header is missing from /usr/local/include (all the other minizip include are present though). Does anyone else have this issue?

The minizip-ng packages available through brew do not have a dependency on zlib (see https://formulae.brew.sh/formula/minizip-ng#default).

That tells me that particular minizip-ng wasn't built with zlib compression activated. Based on the dependencies list, it was built with only xz and zstd compression activated - which corresponds with the missing mz_strm_zlib.h. In the install steps, minizip-ng installs only the headers for the compression methods that were activated in the build.

@doug-walker
Copy link
Collaborator

Thanks for the review Remi! Regarding the Brew issue, based on Cedrik's reply, it sounds like the Brew minizip-ng cannot be used with OCIO since it was not compiled with zlib support activated. In any case, I think we could merge this one now to unblock Cedrik's other PR. We could come back to the Brew issue later, if that's ok with you.

@remia
Copy link
Collaborator

remia commented Dec 5, 2022

Thanks @cedrik-fuoco-adsk yeah that make sense, I guess I'm surprised that zlib is not used it seemed like an obvious choice (to my naive eyes). Agree @doug-walker sorry for dragging this PR!

@doug-walker
Copy link
Collaborator

Don't worry Remi, you did not drag the PR. You raised an excellent point about Brew. And in fact you sped up the PR by reviewing it, thank you!!

@doug-walker doug-walker merged commit 811902b into AcademySoftwareFoundation:main Dec 5, 2022
@doug-walker doug-walker deleted the adsk_contrib/fix-issue-with-minizip-build branch December 5, 2022 22:33
cedrik-fuoco-adsk added a commit that referenced this pull request Jan 5, 2023
* - Refactoring how OCIO search for minizip-ng. The first step is to search for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.
- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Update comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Improved find_package in Config mode (adding it back)
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding +x permissions for install_minizip_ng and zlib
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing target name to match the one used by minizip-ng library (+ using the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 811902b)
cedrik-fuoco-adsk added a commit that referenced this pull request Jan 5, 2023
* - Refactoring how OCIO search for minizip-ng. The first step is to search for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.
- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Update comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Improved find_package in Config mode (adding it back)
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding +x permissions for install_minizip_ng and zlib
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing target name to match the one used by minizip-ng library (+ using the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 811902b)
cedrik-fuoco-adsk added a commit that referenced this pull request Jan 5, 2023
* - Refactoring how OCIO search for minizip-ng. The first step is to search for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.
- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Update comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Improved find_package in Config mode (adding it back)
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding +x permissions for install_minizip_ng and zlib
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing target name to match the one used by minizip-ng library (+ using the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 811902b)
Signed-off-by: Cédrik Fuoco <[email protected]>
doug-walker added a commit that referenced this pull request Jan 6, 2023
* Changing version release type for 2.2.1 official release.

Signed-off-by: Cédrik Fuoco <[email protected]>

* replace texture2D function with texture for GLSL 1.3 (#1723)

Signed-off-by: Bart Styczen <[email protected]>

Signed-off-by: Bart Styczen <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit d5cedbf)
Signed-off-by: Cédrik Fuoco <[email protected]>

* CheckSupportSSE2: Fix sse flags unexpected propagation (#1721)

Set function will affects all variables in current directory. If sse
flags are added in CheckSupportSSE2.cmake, they will remain in
Findminizip-ng.cmake which will cause liblzma cannot be detected. This
patch keep CMAKE_REQUIRED_FLAGS being changed only in current file
scope. This patch has been tested on riscv64.

Signed-off-by: Letu Ren <[email protected]>

Signed-off-by: Letu Ren <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit b6e40f4)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk contrib - Fix issue with minizip build (#1725)

* - Refactoring how OCIO search for minizip-ng. The first step is to search for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.
- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Update comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Improved find_package in Config mode (adding it back)
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding +x permissions for install_minizip_ng and zlib
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing target name to match the one used by minizip-ng library (+ using the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 811902b)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk contrib - Processor cache does not detect changes in cccid (#1726)

* For Looks that has a FileTransform and for Colorspace with FileTransfrom, add the CCCID to the processor's cache key for that transform.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Removing the workaround in the related unit tests and fixing the issue by adding the environment variable to the context using setStringVar. The processor's cache key is using the context cache ID which has all the context variables taken into account.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Now using addStringVars and creating a new context instead of reusing the one used for the filename.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding cccid to the context when there are no context variable.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding a few unit tests to test that the processor is different when changing the FileTransform's CCCID.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Using setStringVar to set CCNUM context variable in unit test.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding a test in FileTransform to test CollectContextVariables directly.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Minor tweaks for the unit test

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 4fa7750)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk contrib - Configure the OpenColorIO.pc file on Windows (#1720)

* Configure the OpenColorIO.pc file on Windows and fix an issue where CMAKE_INSTALL_PREFIX wasn't included in the cmake build command in ocio.bat.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Removing pkconfig folder since that PC file is not used.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Small tweak in the configuration of OpenColorIO.cmake.in to handle absolute path with CMAKE_INSTALL_LIBDIR or CMAKE_INSTALL_INCLUDEDIR.
Keeping exec_prefix for CMAKE_INSTALL_INCLUDE_DIR since it was changed for a specific issue on Mac (see PR #1120).

Signed-off-by: Cédrik Fuoco <[email protected]>

* Using ${prefix} for includedir

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 332462e)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk contrib - Hiding minizip-ng symbols on Mac (#1729)

* Hiding minizip-ng symbols on Mac

Signed-off-by: Cédrik Fuoco <[email protected]>

* Fixing a linking issue related to the code that tries to hide expat symbols and small refactor to add robustness.

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 907ed3b)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk contrib - Fix issue with is colorspace linear (#1734)

* Throw when the colorspace is undefined for isColorSpaceLinear method. (+ unit test)

Signed-off-by: Cédrik Fuoco <[email protected]>

* Typo

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 6d7c479)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk Contrib - Improve naming of ICC-based displays on Windows (#1742)

* Adding a method to get the Monitor's userFriendlyName if available

Signed-off-by: Cédrik Fuoco <[email protected]>

* Removing the slashes at the start of the display name

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding descriptions and comments.
Adding troubleshooting script that print the monitor display name and ICC profile path.

Signed-off-by: Cédrik Fuoco <[email protected]>

* no message

Signed-off-by: Cédrik Fuoco <[email protected]>

* Comments

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 1d126b5)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Fix minizip-ng CMake args passing (#1741)

Signed-off-by: Rémi Achard <[email protected]>

Signed-off-by: Rémi Achard <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
(cherry picked from commit 051241c)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Fix inverse Lut1D optimization bug (#1743)

Signed-off-by: Doug Walker <[email protected]>

Signed-off-by: Doug Walker <[email protected]>
(cherry picked from commit 5152635)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Update documentation for 2.2 release (#1738)

* Update documentation for 2.2 release

Signed-off-by: Doug Walker <[email protected]>

* Fix typos

Signed-off-by: Doug Walker <[email protected]>

* Remove V2_DOC_README.md

Signed-off-by: Doug Walker <[email protected]>

* Tweaks to 2.2

Signed-off-by: Doug Walker <[email protected]>

* Improve installation section

Signed-off-by: Doug Walker <[email protected]>

* Add link to demo config

Signed-off-by: Doug Walker <[email protected]>

* Fix typos

Signed-off-by: Doug Walker <[email protected]>

* Improve file_rules section

Signed-off-by: Doug Walker <[email protected]>

Signed-off-by: Doug Walker <[email protected]>
(cherry picked from commit 17f5c98)
Signed-off-by: Cédrik Fuoco <[email protected]>

* Adsk Contrib - OCIO cmake improvements (#1736)

* - Refactoring how OCIO search for minizip-ng. The first step is to search for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.
- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Update comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Improved find_package in Config mode (adding it back)
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding +x permissions for install_minizip_ng and zlib
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing target name to match the one used by minizip-ng library (+ using the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>

* First pass for the OpenColorIOConfig.cmake file with the required dependencies only.
A few extra fixes for OpenEXR, ZLIB and Minizip-ng.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding a informative message when building static ocio instead of per module.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding an extra step to test the consumer app with static OpenColorIO for Windows, Linux and MacOS.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Re-using the same test instead of creating a new one by removing the condition on build-shared.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Fixing spacing, typo and adding back the NOT in the condition. (it was removed for debugging purpose)

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing directory where we share OCIO custom find modules (now in <install dir>/share/cmake/modules).
Adding a cmake macro when installing OCIO since it is needed by some custom find modules.
The find modules are only installed when building OCIO as a static library.
The config.cmake.in files now only looks for the dependency when OCIO was built as a static library.
Tentative (ci-workflow): Adding cmake-consumer test for static builds.
Overhaul of the Findzlib module to be more inline with the FindZLIB from cmake and to be more robust.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding missing backward slashes and saving the build path in an existing step instead of creating a new step.
Adding a check for CMake version for a section in Findzlib.cmake.
Re-phrasing some comments.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Prevent the download of the dependencies in the scenario where static OCIO is linked to a consumer project. Since OCIO_INSTALL_EXT_PACKAGES is not defined, our find module tries to download the dependencies, but we don't want that mecanism for consumer project.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing path where OCIO install its own custom find module.
Removing custom Findzlib and making modifications to use CMake FindZLIB.
Created a InstallZLIB module which does the download and install part if OCIO_INSTALL_EXT_PACAKGES is ALL or MISSING.
Tweaked config.cmake.in to use CMake FindZLIB.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding more details in comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Removing an extra "shell" property in CI workflow.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Fix issues discovered with the failing CI workflow:

Cmake-consumer test now prefer the static version of the dependencies.
Fix an issue where yaml-cpp is not found by a consumer app (variable spelled incorectly in Findyaml-cpp).
Fix an issue expat library is not found by a consumer app on Windows.
Adding support for OCIO's  <pkg_name>_STATIC_LIBRARY for ZLIB while supporting ZLIB_USE_STATIC_LIBS (CMake 3.24+) from CMake's FindZLIB.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Removing pystring_STATIC_LIBRARY does not exists in CI workflow.
Detecting if the build is Debug in the find module that OCIO installs since they can't rely on variables set by OCIO CMakefiles.
Fix issue with yaml-cpp library naming in debug.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Fix issues on Windows with expat and minizip-ng library naming when building static OCIO.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Added option for macOS CI job in order to get more info on a failed job. Will be reverted once the issue is found.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Fixing a issue on macOS.
Improving ZLIB usage comments.
Bumping minizip-ng to the latest version - 3.0.7.
Bumping ZLIB to the latest version 2.1.13 to fix a vulnerability (CVE-2022-37434)

Signed-off-by: Cédrik Fuoco <[email protected]>

* Proposing to remove Findminizip module since external minizip-ng build need to be done with the same option as the internal build.
Otherwise, linking and symbols issues are going to happend if the other libraries are not linked in correctly.
It is going to simplify the maintainability of OCIO.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Removing findminizip.cmake from the install since it does not exist anymore.

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
(cherry picked from commit 91761f2)
Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Signed-off-by: Bart Styczen <[email protected]>
Signed-off-by: Letu Ren <[email protected]>
Signed-off-by: Rémi Achard <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: bartstyczen <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
Co-authored-by: FantasqueX <[email protected]>
Co-authored-by: Rémi Achard <[email protected]>
@remia remia mentioned this pull request Mar 7, 2023
cedrik-fuoco-adsk added a commit to autodesk-forks/OpenColorIO that referenced this pull request Mar 24, 2023
…n#1725)

* - Refactoring how OCIO search for minizip-ng. The first step is to search for an external minizip-ng. If not found, search for minizip-ng with MZ_COMPAT=ON (libminizip). If it is not found either, download and install minizip-ng with MZ_COMPAT=OFF.
- Removing the minizip-ng part for the includes for minizip-ng headers.

Signed-off-by: Cédrik Fuoco <[email protected]>

* Update comments

Signed-off-by: Cédrik Fuoco <[email protected]>

* Improved find_package in Config mode (adding it back)
Added missing scripts to install minizip-ng and zlib for the analysis workflow

Signed-off-by: Cédrik Fuoco <[email protected]>

* Adding +x permissions for install_minizip_ng and zlib
Fixing path to find zlib in install_minizip-ng.sh

Signed-off-by: Cédrik Fuoco <[email protected]>

* Changing target name to match the one used by minizip-ng library (+ using the imported target instead of creating a new one when minizip-ng is found)

Signed-off-by: Cédrik Fuoco <[email protected]>

Signed-off-by: Cédrik Fuoco <[email protected]>
Co-authored-by: Doug Walker <[email protected]>
Signed-off-by: Cédrik Fuoco <[email protected]>
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.

Impossible to build against system/external minizip-ng
3 participants