CMake: Inconsistent feature control

[dg0yt]

TLDNR: Replace <Pkg>_FOUND with ENABLE_<Feature>.

Describe the bug
In CMakeLists.txt, there are ON/OFF-options for feature control, e.g. ENABLE_GD_FORMATS:

libgd/CMakeLists.txt

Line 12 in 7b0d419

OPTION(ENABLE_GD_FORMATS "Enable GD image formats" 0)

These options are used to control lookup of required dependencies, e.g.

libgd/CMakeLists.txt

Lines 123 to 125 in 7b0d419

	if (ENABLE_GD_FORMATS)
	FIND_PACKAGE(ZLIB REQUIRED)
	endif (ENABLE_GD_FORMATS)

This is fine.
However, in further processing (for macro definitions, selection of programs, export of gdlib.pc), the options are not used directly, but the <Pkg>_FOUND values are used instead:

libgd/CMakeLists.txt

Lines 192 to 196 in 7b0d419

	IF(ZLIB_FOUND)
	INCLUDE_DIRECTORIES(${ZLIB_INCLUDE_DIR})
	SET(HAVE_LIBZ 1)
	LIST(APPEND PKG_REQUIRES_PRIVATES zlib)
	ENDIF(ZLIB_FOUND)

IMO this is wrong and should use the original ENABLE_<Feature> variables. A CMake package might have been found even if the user disabled the corresponding libgd feature. In particular, this happens in vcpkg for static triplets when CMake configs or Find module wrappers chainload additional packages to resolve transitive usage requirements.

• FreeType may use ZLIB.
• TIFF may use ZLIB, JPEG and WebP.

This problem is unlikely to be present with libgd's Find modules in cmake/modules which are unaware of actual static usage requirements. Modifying/replacing these modules is mandatory for static linkage. I still think it is an inconsistency which should be resolved.

As a variant, feature PNG also looks for ZLIB:

libgd/CMakeLists.txt

Lines 127 to 130 in 7b0d419

	if (ENABLE_PNG)
	FIND_PACKAGE(ZLIB REQUIRED)
	FIND_PACKAGE(PNG REQUIRED)
	endif (ENABLE_PNG)

I can imagine that this helps with static linkage. But I don't think it should override the effect of ENABLE_GD_FORMATS.

I could create a PR (based on packaging in vcpkg). I would also merge the second spots (if(<Pkg>_FOUND)) with the first spots (if(<Feature>_ENABLED). This should be possible without source changes which would need adjustments to the autotools build system. But there maybe a need for upfront discussion.

To Reproduce

Build for static triplet in vcpkg with different feature configurations and check build, or do the equivalent directly in CMake.

for I in core core,jpeg core,tools,jpeg ; do 
  for J in "" ,png ,tiff ,freetype ,fontconfig ,webp ; do
    ./vcpkg remove libgd; ./vcpkg install libgd[$I$J] --no-binarycaching
    <Check manually>
  done
done

Or maybe (untested):

cmake ... -DENABLE_GD_FORMATS=0 -DZLIB_FOUND=1

Expected behavior
If a feature is not enabed, it shall never be compiled into libraries, and related executables must not be built.

Actual results
A feature and related executables may be built even if explicit disabled by the user.

Environment (please complete the following information):

• OS: any
• Version: 2.3.3
• Libraries from vcpkg master today.

Additional context
microsoft/vcpkg#27551
which now uses a minimal patch to forward ENABLE_<Feature> to <Pkg>_FOUND to achieve the desired build control.

[vapier]

i don't know why the cmake files prefer xxx_FOUND. it's been written this way since they were introduced, long before i worked on gd directly. maybe as the author, @pierrejoye might remember.

otherwise i agree that we should only be writing out config settings & turning on features based on the knobs the user passed, and not let things get flipped on indirectly.

related, i'd like to see the feature tests in cmake files follow an autodetect-by-default policy rather than off-by-default policy. this is how the autotools build works today:

• default is to probe & use features if found, or turn them off if not
• opting-in to a feature will require it to be found, or fail the build if not
• opting-out of a feature will always turn it off

[dg0yt]

related, i'd like to see the feature tests in cmake files follow an autodetect-by-default policy rather than off-by-default policy. this is how the autotools build works today

Better not, or at least provide an option to change the default for all features. Otherwise it is really hard to avoid unexpected dependencies. The presence of a dependency is only a weak indicator that a depending feature is actually desired.

[vapier]

it isn't hard: disable the options yourself if you don't care. this is how autotools has worked for decades without much hassle.

[dg0yt]

it isn't hard: disable the options yourself if you don't care.

The problem is that with auto-probe, I have to deal with every option because I care about every dependency.

this is how autotools has worked for decades without much hassle.

... within a certain scope of systems and expectations.

In my experience, preferences for default behaviours very much vary with a person's role: core developer, package maintainer, occassional user. I spent quite some time with the (now gone) autotools setup of GDAL... It is easy to miss control (opt-out) for a feature which you don't need (and different users get different results) but it is much less likely to miss control (opt-in) for a feature which you do need.

But I admit that libgd has a fairly small and stable set of options and dependencies.

[vapier]

most people get gd via distros who are well equipped and expected to control every option. autodetect doesn't matter to them because they would never go that route.

random devs who download source releases and build from scratch tend to expect things "just work" out of the box without having to read the manual first. forcing them to specify every option is a pita and not the standard in the larger OSS world.

if you're trying to build and package this up for reuse, then you're expected to set the options yourself.

[dg0yt]

I don't feel comfortable with the strong bias in the responses. I wonder why took the time to read the code of conduct before posting.

[dg0yt]

it isn't hard: disable the options yourself if you don't care. this is how autotools has worked for decades without much hassle.

if you're trying to build and package this up for reuse, then you're expected to set the options yourself.

The key point of this issue that the options do not reliably (enough) control the effects, due to inconsistent use of CMake variables.

[vapier]

The key point of this issue that the options do not reliably (enough) control the effects, due to inconsistent use of CMake variables.

i'm pretty sure i said i agree with you. if the user explicitly enabled/disabled something, then that should be respected.

i think the reason we use the "found" variables is because we want to emulate the autoconf behavior i described wrt autodetect-by-default. we just didn't foresee the indirect leakage via other knobs.

[dg0yt]

It is (fairly) easy to fix the key point of this issue (strict control).
It is a a different thing to implement both proper auto-use and explicit control.

at least provide an option to change the default for all features.

I fail to see if this was accepted as a sufficient compomise.