AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.74k stars 430 forks source link

build(windows): Do not try to build minizip before zlib. #1965

Closed SlawekNowy closed 3 weeks ago

doug-walker commented 2 months ago

@SlawekNowy , thanks for the PR. Would you please provide more detail about what scenarios require this fix? In other words, most Windows builds succeed already without this, so it must be something more specific about the way you are building.

SlawekNowy commented 2 months ago

This is the local windows build. Fetched the repository and built it using Visual Studio generator. Both minizip and zlib are managed by opencolorio. Visual studio 2022 Community x64.

SlawekNowy commented 2 months ago

Oh. I forgot to mention I run --parallel at cmake build step.

Also it seems that did not fix my problems?

SlawekNowy commented 2 months ago

The build is done using cmake .. from build directory (from ocio dir), and then building generated project.

SlawekNowy commented 2 months ago

I'll admit this is not standard setup, but this repo's CMakeLists does fetch deps... And besides that is standard setup on Linux...

SlawekNowy commented 2 months ago

From parent project's discord:

ok cmake. You get a race condition without the deps' root set, but you don't if I do set the root directories of deps? what?

stephen-yee-adsk commented 1 month ago

Hi Slawek,

I encountered the same problem on Windows. It seems that minizip-ng sometimes builds before zlib, which causes a build error, because minizip-ng depends on zlib. I was able to fix it locally by adding a dependency on Zlib in the ExternalProject_add call in Installminizip-ng.cmake.

i.e.

    ExternalProject_Add(minizip-ng_install
        GIT_REPOSITORY "https://github.com/zlib-ng/minizip-ng.git"
        GIT_TAG "${minizip-ng_VERSION}"
        GIT_CONFIG advice.detachedHead=false
        GIT_SHALLOW TRUE
        PREFIX "${_EXT_BUILD_ROOT}/libminizip-ng"
        BUILD_BYPRODUCTS ${minizip-ng_LIBRARY}
        CMAKE_ARGS ${minizip-ng_CMAKE_ARGS}
        EXCLUDE_FROM_ALL TRUE
        BUILD_COMMAND ""
        INSTALL_COMMAND
            ${CMAKE_COMMAND} --build .
                            --config ${CMAKE_BUILD_TYPE}
                            --target install
                            --parallel
        DEPENDS ZLIB::ZLIB        # minizip-ng depends on zlib
    )

Could you try this and see if it fixes your build?

SlawekNowy commented 1 month ago

@stephen-yee-adsk Confirmed fixed, by project head.

doug-walker commented 3 weeks ago

@SlawekNowy , thanks for the update, the code looks perfect now, but unfortunately it looks like you did not sign your latest commit? The DCO sign-off check needs to pass before we're able to merge it. Usually "git commit --amend -s" and then re-pushing will fix it.

SlawekNowy commented 3 weeks ago

Closing in favor of #1986.