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

Build: fix over-eager use of targets #3108

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 25, 2021

Background: Old style CMake was to use FindFoo.cmake modules, which
set FOO_INCLUDES and FOO_LIBRARIES variables. Best new cmake practices
are for the upstream packages themselves to export a FooConfig.cmake
and friends, which set up cmake targets. Of course, many packages don't
provide exported configs, so we use the old Find modules for many of
our dependencies.

I've slowly been migrating our Find modules to set up targets in addition
to the variables, and favoring use of the targets over the variables.

But I was wrong about this. It's more than just a choice of style.
When targets are used, use of transitive linkage (especially for static
library versions of dependencies) make the dependency targets show up in
our own exported OpenImageIOConfig.cmake. But to a downstream package,
now our config talks about a target like Libsquish::Libsquish, but it
doesn't know what that is, and it doesn't have the FindLibsquish.cmake
module that we used (internally) when we found that dependency.

This was causing particular problems for Libsquish on Windows with
vcpkg, but I see now that this could be a problem elsewhere as well:
field3d, libheif, openvdb, and webp.

So this patch:

  • Reverts from the ill-advised targets back to the old style
    variables, for the dependencies where we use custom Find modules.
    (We presume that a dependency's exported configs, as well as any
    Find modules that are distributed as part of CMake itself and define
    targets, are done correctly, and will be available to downstream
    packages, and so we continue to use targets in those cases.)

  • Beefs up our FindLibsquish.cmake module to correctly find debug
    versions of the library, to use in debug builds.

  • Alters build_webp.bash to allow WEBP_BUILD_TYPE to override the build
    type, which had previously been hard-coded to Release.

  • Removes search for HDF5, which as far as I can tell was unused
    directly by OIIO.

Background: Old style CMake was to use FindFoo.cmake modules, which
set FOO_INCLUDES and FOO_LIBRARIES variables. Best new cmake practices
are for the upstream packages themselves to export a FooConfig.cmake
and friends, which set up cmake targets. Of course, many packages don't
provide exported configs, so we use the old Find modules for many of
our dependencies.

I've slowly been migrating our Find modules to set up targets in addition
to the variables, and favoring use of the targets over the variables.

But I was wrong about this. It's more than just a choice of style.
When targets are used, use of transitive linkage (especially for static
library versions of dependencies) make the dependency targets show up in
our own exported OpenImageIOConfig.cmake. But to a downstream package,
now our config talks about a target like Libsquish::Libsquish, but it
doesn't know what that is, and it doesn't have the FindLibsquish.cmake
module that we used (internally) when we found that dependency.

This was causing particular problems for Libsquish on Windows with
vcpkg, but I see now that this could be a problem elsewhere as well:
field3d, libheif, openvdb, and webp.

So this patch:

* Reverts from the ill-advised targets back to the old style
  variables, for the dependencies where we use *custom* Find modules.
  (We presume that a dependency's exported configs, as well as any
  Find modules that are distributed as part of CMake itself and define
  targets, are done correctly, and will be available to downstream
  packages, and so we continue to use targets in those cases.)

* Beefs up our FindLibsquish.cmake module to correctly find debug
  versions of the library, to use in debug builds.

* Alters build_webp.bash to allow WEBP_BUILD_TYPE to override the build
  type, which had previously been hard-coded to Release.

* Removes search for HDF5, which as far as I can tell was unused
  directly by OIIO.
@lgritz lgritz merged commit 8de11b8 into AcademySoftwareFoundation:master Sep 28, 2021
@lgritz lgritz deleted the lg-targets branch September 28, 2021 04:53
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Sep 29, 2021
Background: Old style CMake was to use FindFoo.cmake modules, which
set FOO_INCLUDES and FOO_LIBRARIES variables. Best new cmake practices
are for the upstream packages themselves to export a FooConfig.cmake
and friends, which set up cmake targets. Of course, many packages don't
provide exported configs, so we use the old Find modules for many of
our dependencies.

I've slowly been migrating our Find modules to set up targets in addition
to the variables, and favoring use of the targets over the variables.

But I was wrong about this. It's more than just a choice of style.
When targets are used, use of transitive linkage (especially for static
library versions of dependencies) make the dependency targets show up in
our own exported OpenImageIOConfig.cmake. But to a downstream package,
now our config talks about a target like Libsquish::Libsquish, but it
doesn't know what that is, and it doesn't have the FindLibsquish.cmake
module that we used (internally) when we found that dependency.

This was causing particular problems for Libsquish on Windows with
vcpkg, but I see now that this could be a problem elsewhere as well:
field3d, libheif, openvdb, and webp.

So this patch:

* Reverts from the ill-advised targets back to the old style
  variables, for the dependencies where we use *custom* Find modules.
  (We presume that a dependency's exported configs, as well as any
  Find modules that are distributed as part of CMake itself and define
  targets, are done correctly, and will be available to downstream
  packages, and so we continue to use targets in those cases.)

* Beefs up our FindLibsquish.cmake module to correctly find debug
  versions of the library, to use in debug builds.

* Alters build_webp.bash to allow WEBP_BUILD_TYPE to override the build
  type, which had previously been hard-coded to Release.

* Removes search for HDF5, which as far as I can tell was unused
  directly by OIIO.
@JackBoosY
Copy link

Thanks for your fix.

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.

2 participants