openexr icon indicating copy to clipboard operation
openexr copied to clipboard

CMake: use EXCLUDE_FROM_ALL when declaring Fetch for deflate library

Open mattyjams opened this issue 1 year ago • 8 comments

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.

Testing this on Mac, I ran a build of the current tip of the main branch (3d34ea0) using these commands:

cd /Users/matt.johnson/Developer/openexr
mkdir build && cd build
cmake -DCMAKE_INSTALL_PREFIX="/Users/matt.johnson/Developer/openexr_inst" -DCMAKE_PREFIX_PATH="/Users/matt.johnson/Developer/openexr_inst" -DCMAKE_BUILD_TYPE=Release -DCMAKE_MACOSX_RPATH=ON -G "Xcode" -DOPENEXR_BUILD_TOOLS=OFF -DOPENEXR_BUILD_EXAMPLES=OFF -DOPENEXR_INSTALL_PKG_CONFIG=OFF -DBUILD_TESTING=OFF -DCMAKE_XCODE_ATTRIBUTE_ONLY_ACTIVE_ARCH=YES -DCMAKE_OSX_ARCHITECTURES=arm64 "/Users/matt.johnson/Developer/openexr"
cmake --build . --config Release --target install -- -j 10

And looking at the build products in the include and lib directories, I see this:

% ls -l /Users/matt.johnson/Developer/openexr_inst/include 
total 32
drwxr-xr-x@  37 matt.johnson  staff   1184 Oct 11 09:48 Imath
drwxr-xr-x@ 148 matt.johnson  staff   4736 Oct 11 09:48 OpenEXR
-rw-r--r--@   1 matt.johnson  staff  15748 Oct 11 09:47 libdeflate.h
% ls -l /Users/matt.johnson/Developer/openexr_inst/lib    
total 10560
drwxr-xr-x@ 5 matt.johnson  staff      160 Oct 11 09:48 cmake
-rwxr-xr-x@ 1 matt.johnson  staff   963664 Oct 11 09:48 libIex-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       25 Oct 11 09:48 libIex-3_4.99.dylib -> libIex-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       19 Oct 11 09:48 libIex-3_4.dylib -> libIex-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       16 Oct 11 09:48 libIex.dylib -> libIex-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff    85088 Oct 11 09:48 libIlmThread-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       31 Oct 11 09:48 libIlmThread-3_4.99.dylib -> libIlmThread-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       25 Oct 11 09:48 libIlmThread-3_4.dylib -> libIlmThread-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       22 Oct 11 09:48 libIlmThread.dylib -> libIlmThread-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff   374624 Oct 11 09:48 libImath-3_2.30.3.2.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       27 Oct 11 09:48 libImath-3_2.30.dylib -> libImath-3_2.30.3.2.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       21 Oct 11 09:48 libImath-3_2.dylib -> libImath-3_2.30.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       18 Oct 11 09:48 libImath.dylib -> libImath-3_2.dylib
-rwxr-xr-x@ 1 matt.johnson  staff  1154256 Oct 11 09:48 libOpenEXR-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       29 Oct 11 09:48 libOpenEXR-3_4.99.dylib -> libOpenEXR-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       23 Oct 11 09:48 libOpenEXR-3_4.dylib -> libOpenEXR-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       20 Oct 11 09:48 libOpenEXR.dylib -> libOpenEXR-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff  2388240 Oct 11 09:48 libOpenEXRCore-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       33 Oct 11 09:48 libOpenEXRCore-3_4.99.dylib -> libOpenEXRCore-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       27 Oct 11 09:48 libOpenEXRCore-3_4.dylib -> libOpenEXRCore-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       24 Oct 11 09:48 libOpenEXRCore.dylib -> libOpenEXRCore-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff   231248 Oct 11 09:48 libOpenEXRUtil-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       33 Oct 11 09:48 libOpenEXRUtil-3_4.99.dylib -> libOpenEXRUtil-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       27 Oct 11 09:48 libOpenEXRUtil-3_4.dylib -> libOpenEXRUtil-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       24 Oct 11 09:48 libOpenEXRUtil.dylib -> libOpenEXRUtil-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff   120928 Oct 11 09:48 libdeflate.0.dylib
-rw-r--r--@ 1 matt.johnson  staff    70496 Oct 11 09:48 libdeflate.a
lrwxr-xr-x@ 1 matt.johnson  staff       18 Oct 11 09:48 libdeflate.dylib -> libdeflate.0.dylib
drwxr-xr-x@ 4 matt.johnson  staff      128 Oct 11 09:48 pkgconfig

In particular, include contains libdeflate.h, and lib contains libdeflate.* libraries.

Applying the change in this PR and re-running the same build, those are no longer being produced and installed:

% ls -l /Users/matt.johnson/Developer/openexr_inst/include 
total 0
drwxr-xr-x@  37 matt.johnson  staff  1184 Oct 11 09:50 Imath
drwxr-xr-x@ 148 matt.johnson  staff  4736 Oct 11 09:50 OpenEXR
% ls -l /Users/matt.johnson/Developer/openexr_inst/lib    
total 10176
drwxr-xr-x@ 4 matt.johnson  staff      128 Oct 11 09:50 cmake
-rwxr-xr-x@ 1 matt.johnson  staff   963664 Oct 11 09:50 libIex-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       25 Oct 11 09:50 libIex-3_4.99.dylib -> libIex-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       19 Oct 11 09:50 libIex-3_4.dylib -> libIex-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       16 Oct 11 09:50 libIex.dylib -> libIex-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff    85088 Oct 11 09:50 libIlmThread-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       31 Oct 11 09:50 libIlmThread-3_4.99.dylib -> libIlmThread-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       25 Oct 11 09:50 libIlmThread-3_4.dylib -> libIlmThread-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       22 Oct 11 09:50 libIlmThread.dylib -> libIlmThread-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff   374624 Oct 11 09:50 libImath-3_2.30.3.2.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       27 Oct 11 09:50 libImath-3_2.30.dylib -> libImath-3_2.30.3.2.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       21 Oct 11 09:50 libImath-3_2.dylib -> libImath-3_2.30.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       18 Oct 11 09:50 libImath.dylib -> libImath-3_2.dylib
-rwxr-xr-x@ 1 matt.johnson  staff  1154256 Oct 11 09:50 libOpenEXR-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       29 Oct 11 09:50 libOpenEXR-3_4.99.dylib -> libOpenEXR-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       23 Oct 11 09:50 libOpenEXR-3_4.dylib -> libOpenEXR-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       20 Oct 11 09:50 libOpenEXR.dylib -> libOpenEXR-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff  2388240 Oct 11 09:50 libOpenEXRCore-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       33 Oct 11 09:50 libOpenEXRCore-3_4.99.dylib -> libOpenEXRCore-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       27 Oct 11 09:50 libOpenEXRCore-3_4.dylib -> libOpenEXRCore-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       24 Oct 11 09:50 libOpenEXRCore.dylib -> libOpenEXRCore-3_4.dylib
-rwxr-xr-x@ 1 matt.johnson  staff   231248 Oct 11 09:50 libOpenEXRUtil-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       33 Oct 11 09:50 libOpenEXRUtil-3_4.99.dylib -> libOpenEXRUtil-3_4.99.3.4.0.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       27 Oct 11 09:50 libOpenEXRUtil-3_4.dylib -> libOpenEXRUtil-3_4.99.dylib
lrwxr-xr-x@ 1 matt.johnson  staff       24 Oct 11 09:50 libOpenEXRUtil.dylib -> libOpenEXRUtil-3_4.dylib
drwxr-xr-x@ 3 matt.johnson  staff       96 Oct 11 09:50 pkgconfig

mattyjams avatar Oct 11 '24 14:10 mattyjams

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: mattyjams / name: Matt Johnson (fb5e11848ed396d401c1520eb317354ce7d3b61a, 0b6a06d03373ff76098da912891a4ef5f2444619)

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.

cary-ilm avatar Oct 11 '24 21:10 cary-ilm

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 EXCLUDE_FROM_ALL only started having the desired effect in CMake 3.28. I had tested with the latest release (3.30.5), but when I dropped back to a 3.27 version of CMake I started seeing what you did where deflate was still being installed. I ended up addressing that by manually setting the EXCLUDE_FROM_ALL property on the directories that end up getting fetched when CMake is older than 3.28.

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 Deflate_POPULATED should have actually been deflate_POPULATED. I fixed that in a separate commit, and used deflate_... in the new code.

mattyjams avatar Oct 12 '24 09:10 mattyjams

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.

cary-ilm avatar Oct 12 '24 17:10 cary-ilm

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.

Thanks @cary-ilm! I was able to reproduce this even still on Mac when I switched to the Unix Makefiles generator and tested some older releases of CMake.

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 FetchContent_Populate() for CMake versions before 3.30, and FetchContent_MakeAvailable() for versions 3.30 and later. I left a bit of a paper trail in the comment to try to explain how I ended up there.

I tested with these versions of CMake: 3.14.0 3.19.3 3.20.2 3.20.3 3.27.9 3.28.0 3.30.5

And confirmed that deflate was not built with any of them, and the deprecation warning that af9ef0e68f6493dc2053c0489f558955e948c9d9 aimed to address still does not appear.

Let me know what you think now! And thanks for your patience with this one. :)

mattyjams avatar Oct 15 '24 21:10 mattyjams

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 FetchContent_Populate.

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.

cary-ilm avatar Oct 15 '24 21:10 cary-ilm

@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!

cary-ilm avatar Oct 16 '24 15:10 cary-ilm

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 avatar Oct 16 '24 18:10 mattyjams

@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!

cary-ilm avatar Oct 30 '24 20:10 cary-ilm

@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!

mattyjams avatar Oct 31 '24 14:10 mattyjams