CMake: use EXCLUDE_FROM_ALL when declaring Fetch for deflate library #1880
Conversation
|
Oops, I accidentally pushed a commit to your branch, after cloning your PR here. How is that even possible?!? I'm having a hard time reproducing your fix. Even with EXCLUDE_FROM_ALL, my build is still installing the deflate files. The commit I just pushed forces an internal build of libdeflate in the CI so we'll see what it does. It was an oversight that the CI is not validating the behavior of the internal libdeflate. |
mattyjams
force-pushed
the
bugfix/exclude_deflate_from_build
branch
from
cb4307b to
6b69f92
Compare
|
No problem! I just re-pushed the branch, including what I think is a fix for the problem you're seeing. I think it ended up being a CMake version issue. Now that I'm paying closer attention to the documentation, it looks like I wasn't able to test all the way back to CMake 3.14, which appears to be the current minimum, but I also don't see any other notable "New in ..." notes in the documentation that would be of concern. One other oddity I noticed is that these CMake functions are actually setting their variables using a case-lowered version of the name you provide them, so the existing check of |
|
There seems to be yet another problem here, when attempting to build the website source code, to validate that it compiles and links, it's somehow expecting to find libdeflate.a, although I'm still having trouble reproducing that error locally. |
mattyjams
force-pushed
the
bugfix/exclude_deflate_from_build
branch
from
6b69f92 to
d1bb107
Compare
Thanks @cary-ilm! I was able to reproduce this even still on Mac when I switched to the I tried a whole bunch of things to manually set properties on targets and set various policies, and I could manage to get deflate not to build, but I could not find a way around preventing CMake from thinking it needed to install those unbuilt libraries. The slightly unsatisfying, but simplest, solution was simply to keep using I tested with these versions of CMake: And confirmed that deflate was not built with any of them, and the deprecation warning that af9ef0e aimed to address still does not appear. Let me know what you think now! And thanks for your patience with this one. :) |
|
Thanks for the further investigation. That seems entirely reasonable, even if it is a bit messy. I also experimented with several other options but couldn't find anything that worked, so I was starting to come to the same conclusion, to revert to This did spur me to add a validation step to our CI to ensure that we don't inadvertently introduce unexpected files into the install, which would have flagged this prior to release. |
|
@mattyjams, thanks for working this out, we're happy to accept the contribution. Will you be able to sign the CLA? If that's too much of a hassle, I'm happy to replicate the change in a fresh PR myself, let me know which you prefer. If you can get the CLA signed, our project policy is also to require signed commits, as noted in the DCO check. If you amend the commits with the -s option, that will satisfy it. Thanks! |
The documentation for CMake's FetchContent_GetProperties() is not particularly clear, but it follows the same convention of FetchContent_Populate() and FetchContent_MakeAvailable() where the given name is converted to lower case to produce the variable names, so this changes the test for Deflate population to use the correct "deflate_POPULATED" variable name. Signed-off-by: Matt Johnson <[email protected]>
Commit af9ef0e made a change in the CMake setup to switch from using FetchContent_Populate() to FetchContent_MakeAvailable() to bring in libdeflate (and Imath) as dependencies. Using FetchContent_MakeAvailable() has the side effect that libdeflate is now being included as part of the build, causing libraries and headers to be built and installed. The intent with fetching the deflate source is solely to copy select files into OpenEXRCore, so this change adds the EXCLUDE_FROM_ALL option to the call to FetchContent_Declare() to prevent deflate from being included in the build. One wrinkle though is that prior to CMake 3.28, passing the EXCLUDE_FROM_ALL option to FetchContent_Declare() does *not* have the desired effect of excluding the fetched content from the build when FetchContent_MakeAvailable() is called. Ideally we could "manually" set the EXCLUDE_FROM_ALL property on the deflate SOURCE_DIR and BINARY_DIR, but a bug that was only fixed as of CMake 3.20.3 prevents that from properly excluding the directories: https://gitlab.kitware.com/cmake/cmake/-/issues/22234 To support the full range of CMake versions without overly complicating things with cumbersome workarounds, we continue to use Populate for CMake versions before 3.30, and switch to MakeAvailable for CMake 3.30 and later. Signed-off-by: Matt Johnson <[email protected]>
mattyjams
force-pushed
the
bugfix/exclude_deflate_from_build
branch
from
d1bb107 to
0b6a06d
Compare
|
Thanks again, @cary-ilm! I just pushed one more update with those commits signed off. Sorry I missed that earlier! And I've pinged a few folks internally to try to get myself added to our (Epic Games) corporate CLA. I'll try to keep prodding that along. |
|
@mattyjams, any progress on the legal front? If it looks like it's going to take much longer, I'd like to submit a parallel fix so we can move forward with a new patch release soon. Thanks! |
Sorry for the delay @cary-ilm! The CLA stuff should be resolved now! |
…1880) * CMake: use lower case name when checking whether Deflate is populated The documentation for CMake's FetchContent_GetProperties() is not particularly clear, but it follows the same convention of FetchContent_Populate() and FetchContent_MakeAvailable() where the given name is converted to lower case to produce the variable names, so this changes the test for Deflate population to use the correct "deflate_POPULATED" variable name. Signed-off-by: Matt Johnson <[email protected]> * CMake: use EXCLUDE_FROM_ALL when declaring Fetch for deflate library Commit af9ef0e made a change in the CMake setup to switch from using FetchContent_Populate() to FetchContent_MakeAvailable() to bring in libdeflate (and Imath) as dependencies. Using FetchContent_MakeAvailable() has the side effect that libdeflate is now being included as part of the build, causing libraries and headers to be built and installed. The intent with fetching the deflate source is solely to copy select files into OpenEXRCore, so this change adds the EXCLUDE_FROM_ALL option to the call to FetchContent_Declare() to prevent deflate from being included in the build. One wrinkle though is that prior to CMake 3.28, passing the EXCLUDE_FROM_ALL option to FetchContent_Declare() does *not* have the desired effect of excluding the fetched content from the build when FetchContent_MakeAvailable() is called. Ideally we could "manually" set the EXCLUDE_FROM_ALL property on the deflate SOURCE_DIR and BINARY_DIR, but a bug that was only fixed as of CMake 3.20.3 prevents that from properly excluding the directories: https://gitlab.kitware.com/cmake/cmake/-/issues/22234 To support the full range of CMake versions without overly complicating things with cumbersome workarounds, we continue to use Populate for CMake versions before 3.30, and switch to MakeAvailable for CMake 3.30 and later. Signed-off-by: Matt Johnson <[email protected]> --------- Signed-off-by: Matt Johnson <[email protected]>
Commit af9ef0e made a change in the CMake setup to switch from using
FetchContent_Populate()toFetchContent_MakeAvailable()to bring inlibdeflate(andImath) as dependencies. UsingFetchContent_MakeAvailable()has the side effect thatlibdeflateis now being included as part of the build, causing libraries and headers to be built and installed.The intent with fetching the deflate source is solely to copy select files into OpenEXRCore, so this change adds the
EXCLUDE_FROM_ALLoption to the call toFetchContent_Declare()to prevent deflate from being included in the build.Testing this on Mac, I ran a build of the current tip of the main branch (3d34ea0) using these commands:
And looking at the build products in the
includeandlibdirectories, I see this:In particular,
includecontainslibdeflate.h, andlibcontainslibdeflate.*libraries.Applying the change in this PR and re-running the same build, those are no longer being produced and installed: