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.

Sign up for GitHub

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

Jump to bottom

[openimageio] Re-fix the usage #20092

Merged
merged 2 commits into from
Sep 17, 2021
Merged

[openimageio] Re-fix the usage #20092

merged 2 commits into from
Sep 17, 2021

Conversation

JackBoosY
Copy link
Contributor

Merging to master caused all my changes in portfile.cmake to be rolled back.
Re-fix them:

  1. Restore FindOpenEXR.cmake since it calls find_dependency(OpenEXR CONFIG) and it contains the required macro FOUND_OPENEXR_WITH_CONFIG in OpenImageIOConfig.cmake.
  2. Correct FindLibsquish.cmake and vcpkg-cmake-wrapper.cmake installation path.

Related: #19916.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Sep 10, 2021
@JackBoosY
Copy link
Contributor Author

@jmacey Can you please test this PR?

Thanks.

github-actions[bot]
github-actions bot requested changes Sep 10, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated 
git add -u && git commit
git checkout dfcd4e4b30799c4ce02fe3939b62576fec444224 -- versions
./vcpkg x-add-version --all
Diff 
diff --git a/versions/baseline.json b/versions/baseline.json
index 4430ddd..bfbc314 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -4718,7 +4718,7 @@
     },
     "openimageio": {
       "baseline": "2.3.7.2",
-      "port-version": 1
+      "port-version": 2
     },
     "openjpeg": {
       "baseline": "2.3.1",
diff --git a/versions/o-/openimageio.json b/versions/o-/openimageio.json
index 1ebf88c..827012d 100644
--- a/versions/o-/openimageio.json
+++ b/versions/o-/openimageio.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "6084a6398514fc24fd450a7b9290937b1274d7b6",
+      "version": "2.3.7.2",
+      "port-version": 2
+    },
     {
       "git-tree": "799ea36f0486224257ecfea149b429d81e74a879",
       "version": "2.3.7.2",

@jmacey
Copy link

jmacey commented Sep 10, 2021

@jmacey Can you please test this PR?

Thanks.

This now builds fine (Windows) but still fails to copy squish.dll to the target directory.

@JackBoosY
Copy link
Contributor Author

but still fails to copy squish.dll to the target directory.

@jmacey Can you provide me a repro step?

@jmacey
Copy link

jmacey commented Sep 10, 2021 via email

@JackBoosY
Copy link
Contributor Author

@jmacey Did you use the X_VCPKG_APPLOCAL_DEPS_INSTALL feature?

@jmacey
Copy link

jmacey commented Sep 13, 2021 via email

@JackBoosY
Copy link
Contributor Author

I confirmed that squish.dll was copied into tools/openimageio:

PS F:\vcpkg\packages\openimageio_x86-windows\tools\openimageio> ls


    Directory: F:\vcpkg\packages\openimageio_x86-windows\tools\openimageio


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----         9/12/2021  10:56 PM                plugins
-a----         7/19/2021   6:49 PM          94208 boost_filesystem-vc141-mt-x32-1_75.dll
-a----         7/19/2021   6:52 PM          66048 boost_thread-vc141-mt-x32-1_75.dll
-a----        10/19/2020  12:38 AM         135680 brotlicommon.dll
-a----        10/19/2020  12:38 AM          41984 brotlidec.dll
-a----         1/15/2021   1:05 AM          52736 bz2.dll
-a----          2/3/2021  11:18 PM         526848 freetype.dll
-a----          1/6/2021   1:00 AM         274432 Half-2_5.dll
-a----         3/12/2021   8:09 AM         832000 harfbuzz.dll
-a----         8/15/2021   8:15 PM         339968 heif.dll
-a----         9/12/2021  10:55 PM          93696 iconvert.exe
-a----         1/17/2021   6:50 PM       28398592 icudt67.dll
-a----         1/17/2021   6:50 PM        2095104 icuin67.dll
-a----         1/17/2021   6:50 PM        1377792 icuuc67.dll
-a----         9/12/2021  10:55 PM          41984 idiff.exe
-a----          1/6/2021   1:00 AM         181760 Iex-2_5.dll
-a----         9/12/2021  10:55 PM          53760 igrep.exe
-a----         9/12/2021  10:55 PM         119296 iinfo.exe
-a----          1/6/2021   1:00 AM        2668544 IlmImf-2_5.dll
-a----          1/6/2021   1:00 AM          35840 IlmThread-2_5.dll
-a----          1/6/2021   1:00 AM          84992 Imath-2_5.dll
-a----         9/12/2021  10:55 PM         238592 iv.exe
-a----        10/14/2020  10:54 PM         528896 jpeg62.dll
-a----          1/3/2021   9:56 PM         417792 libde265.dll
-a----          2/6/2021   6:42 PM         162304 libpng16.dll
-a----         5/31/2021   3:01 AM        2905600 libx265.dll
-a----         2/18/2021   1:49 AM         132608 lzma.dll
-a----         9/12/2021  10:55 PM          94208 maketx.exe
-a----         9/12/2021  10:55 PM         564736 oiiotool.exe
-a----         9/12/2021  10:55 PM        6078464 OpenImageIO.dll
-a----         9/12/2021  10:54 PM         581120 OpenImageIO_Util.dll
-a----        10/19/2020   1:02 AM         409088 pcre2-16.dll
-a----         9/12/2021  10:56 PM              9 qt.conf
-a----         8/15/2021   8:42 PM        4476416 Qt5Core.dll
-a----         8/15/2021   8:45 PM        5555712 Qt5Gui.dll
-a----         8/15/2021   8:47 PM        4606976 Qt5Widgets.dll
-a----          9/1/2021  11:05 PM          41472 squish.dll
-a----         2/18/2021   1:50 AM         380928 tiff.dll
-a----        10/14/2020  10:55 PM          73216 zlib1.dll
-a----        12/23/2020   1:38 AM         460800 zstd.dll

@jmacey
Copy link

jmacey commented Sep 13, 2021 via email

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Sep 13, 2021 •
edited by PhoebeHui
Loading

After reading the source code, oiio requires squish, but it wasn't exported in the oiio cmake config files. That cause the usuage issue. I think that's a oiio bug, @lgritz, could you please help comfirm this?

All squish related codes here:
In src/dds.imageio/CMakeLists.txt:

if (Libsquish_FOUND)
    # External libsquish was found -- use it
    add_oiio_plugin (ddsinput.cpp
                     LINK_LIBRARIES Libsquish::Libsquish
                     )
...

In src/cmake/add_oiio_plugins.cmake:

macro (add_oiio_plugin)
    cmake_parse_arguments (_plugin "" "NAME" "SRC;INCLUDE_DIRS;LINK_LIBRARIES;DEFINITIONS" ${ARGN})
...
            set (format_plugin_libs ${format_plugin_libs} ${_plugin_LINK_LIBRARIES} PARENT_SCOPE)
...

In src/libOpenImageIO/CMakeLists.txt:

...
target_link_libraries (OpenImageIO
...
        PRIVATE
...
            ${format_plugin_libs} # Add all the target link libraries from the plugins
...

@jmacey
Copy link

jmacey commented Sep 13, 2021 via email

@JackBoosY JackBoosY requested a review from PhoebeHui September 15, 2021 02:33
@BillyONeal
Copy link
Member

It seems like this port is trying to vendor libsquish when it should depend on the libsquish port instead?

@JackBoosY
Copy link
Contributor Author

@BillyONeal No, oiio just use the internal FindLibsquish.cmake and oiio depends on libsquish.

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Sep 16, 2021
@BillyONeal
Copy link
Member

@BillyONeal No, oiio just use the internal FindLibsquish.cmake and oiio depends on libsquish.

If it wants to make squish.dll (as reported by @jmacey ) that doesn't sound like an internal-only use. (I know this isn't the bug you're necessarily trying to fix in this PR but ideally we would de-vendor that dependency)

@jmacey
Copy link

jmacey commented Sep 16, 2021 via email

@JackBoosY
Copy link
Contributor Author

@BillyONeal No, oiio just use the internal FindLibsquish.cmake and oiio depends on libsquish.

If it wants to make squish.dll (as reported by @jmacey ) that doesn't sound like an internal-only use. (I know this isn't the bug you're necessarily trying to fix in this PR but ideally we would de-vendor that dependency)

No, it won't, I confirmed that.

@BillyONeal
Copy link
Member

@JackBoosY

I see, there was some confusion here. I was under the impression that I said "this vendors libsquish" and you said, effectively, "it vendors a custom version of libsquish that it only uses internally". But what you actually said was "it does depend on the libsquish port" and Bill was blind.

It still seems like the targets exported by OpenImageIoConfig.cmake should be finding libsquish on their own rather than forcing customers to add Libsquish::Libsquish ?

@BillyONeal
Copy link
Member

To clarify for other reviewers, the situation is:

  • The port libsquish does not provide any kind of cmake bindings.
  • The port openimageio installs share/OpenImageIO/FindLibsquish.cmake -- and it provides Libsquish::Libsquish even though it isn't itself an owner of Libsquish.

The usage text ends up being:

The package openimageio:x86-windows provides CMake targets:

find_package(Modules CONFIG REQUIRED)
target_link_libraries(main PRIVATE OpenImageIO::OpenImageIO)

find_package(OpenImageIO CONFIG REQUIRED)
target_link_libraries(main PRIVATE Libsquish::Libsquish OpenImageIO::OpenImageIO OpenImageIO::OpenImageIO_Util)

@BillyONeal
Copy link
Member 

The package openimageio:x86-windows provides CMake targets:

find_package(Modules CONFIG REQUIRED)
target_link_libraries(main PRIVATE OpenImageIO::OpenImageIO)

This does not work:

C:\Users\bion\Desktop>C:\Dev\vcpkg\vcpkg.exe list | findstr openimageio
openimageio:x86-windows                            2.3.7.2#2        A library for reading and writing images, and a ...

C:\Users\bion\Desktop>type CMakeLists.txt
cmake_minimum_required(VERSION 3.21)
project(demo CXX)

add_executable(main main.cpp)

find_package(Modules CONFIG REQUIRED)
target_link_libraries(main PRIVATE OpenImageIO::OpenImageIO)

C:\Users\bion\Desktop>type main.cpp
#include <stdio.h>

int main() {
    puts("Hello world.");
    return 0;
}

C:\Users\bion\Desktop>cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=C:/Dev/vcpkg/scripts/buildsystems/vcpkg.cmake -S . -B out
-- The CXX compiler identification is MSVC 19.29.30133.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx86/x86/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at C:/Dev/vcpkg/scripts/buildsystems/vcpkg.cmake:784 (_find_package):
  Could not find a package configuration file provided by "Modules" with any
  of the following names:

    ModulesConfig.cmake
    modules-config.cmake

  Add the installation prefix of "Modules" to CMAKE_PREFIX_PATH or set
  "Modules_DIR" to a directory containing one of the above files.  If
  "Modules" provides a separate development package or SDK, be sure it has
  been installed.
Call Stack (most recent call first):
  CMakeLists.txt:6 (find_package)


-- Configuring incomplete, errors occurred!
See also "C:/Users/bion/Desktop/out/CMakeFiles/CMakeOutput.log".

C:\Users\bion\Desktop>

@BillyONeal BillyONeal merged commit 574c125 into microsoft:master Sep 17, 2021
@BillyONeal
Copy link
Member

I merged this anyway because I observe that the problems I'm reporting here aren't problems caused by this PR.

@lgritz
Copy link

lgritz commented Sep 17, 2021

@JackBoosY But it's not part of OIIO's public interface. OIIO internals call into libsquish, but downstream apps that use OIIO's APIs don't need to call libsquish APIs. Is that not the meaning of PRIVATE here? If I've misunderstood, then what would be an example of where PRIVATE would be the right designation?

@BillyONeal
Copy link
Member

@lgritz Even if you only use it internally, it needs to be on your customers' link lines. If you make it PUBLIC, cmake will add those bits to customers' link lines transitively.

@lgritz
Copy link

lgritz commented Sep 17, 2021

@JackBoosY I modified OIIO's CI so that the Windows tests for sure use vcpkg's libsquish (it had previously just used the embedded version), and it seems to work. What's different?

@BillyONeal OK, I can accept that I've misunderstood these modifiers (though they sure have worked for me for a long time). But then, can you explain to me what is an example of when PRIVATE would be appropriate? What's it for, if I have to use PUBLIC for anything that needs to be available to downstream clients at link time?

Is this Windows specific? Because on Linux and OSX, when libOpenImageIO.so links to libsquish.so, a downstream program only needs to link against libOpenImageIO.so. I only need to mark as PUBLIC if the downstream app might directly call into libsquish (i.e., if libsquish types or APIs are part of OIIO's public API).

@jmacey
Copy link

jmacey commented Sep 17, 2021 via email

@BillyONeal
Copy link
Member

@BillyONeal OK, I can accept that I've misunderstood these modifiers (though they sure have worked for me for a long time). But then, can you explain to me what is an example of when PRIVATE would be appropriate? What's it for, if I have to use PUBLIC for anything that needs to be available to downstream clients at link time?

I'm not aware of great uses for PRIVATE libraries that work across platforms, but on Windows where each DLL is an island, if you know the result is a DLL, you don't transitively need customers to supply libraries on their link lines. I think there may be similar cases for *nix but I understand that less.

The more common example is to have private/internal headers that customers aren't supposed to have in their environment but that a project uses internally.

@lgritz
Copy link

lgritz commented Sep 18, 2021

On Unix/Linux, with dynamic libraries, if you have libA that depends on libB only for internals (doesn't expose B in A's public API), that dependency to B can be PRIVATE, and libA linking against libB is enough to ensure that when appC links against libA, it will automatically pull in libB. At least for shared libraries. For static, you may need to specify them, but my impression was that CMake handled the decisions about whether to add B explicitly to the linkage for A depending on whether it was static or dynamic.

Is that not possible in Windows?

So the general guidance is that I should pretty much always be using PUBLIC, regardless of whether a dependency is exposed through my APIs, in order to guarantee that the transitive link needs are met?

I probably have a lot of places to fix, then.

@lgritz
Copy link

lgritz commented Sep 18, 2021

Doing some more reading...

Generally, dynamic libraries encode their dependency information (what they were linked against when you linked the library itself). Static libraries do not. So what CMake is supposed to do is understand this, and for PRIVATE library dependencies, pass on the library interface if it's static but not dynamic.

If I understand correctly, marking everything as PUBLIC seems like overkill.

However, I can believe that what happens is that, if static libs are used for libsquish, although cmake will pass along the library dependency in our exported config, a downstream consumer still won't have the FindLibsquish.cmake, and so may not have a good way to find the library when it needs it for linking?

@JackBoosY
Copy link
Contributor Author

@lgritz

F:\vcpkg\packages\openimageio_x86-windows\bin> dumpbin /DEPENDENTS .\OpenImageIO.dll
Microsoft (R) COFF/PE Dumper Version 14.16.27045.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file .\OpenImageIO.dll

File Type: DLL

  Image has the following dependencies:

    OpenImageIO_Util.dll
    squish.dll
    heif.dll
    jpeg62.dll
    zlib1.dll
    libpng16.dll
    tiff.dll
    IlmImf-2_5.dll
    Imath-2_5.dll
    Half-2_5.dll
    Iex-2_5.dll
    boost_thread-vc141-mt-x32-1_75.dll
    KERNEL32.dll
    MSVCP140.dll
    WS2_32.dll
    VCRUNTIME140.dll
    api-ms-win-crt-runtime-l1-1-0.dll
    api-ms-win-crt-heap-l1-1-0.dll
    api-ms-win-crt-math-l1-1-0.dll
    api-ms-win-crt-stdio-l1-1-0.dll
    api-ms-win-crt-locale-l1-1-0.dll
    api-ms-win-crt-convert-l1-1-0.dll
    api-ms-win-crt-environment-l1-1-0.dll
    api-ms-win-crt-time-l1-1-0.dll
    api-ms-win-crt-string-l1-1-0.dll
    api-ms-win-crt-filesystem-l1-1-0.dll

  Summary

       4C000 .data
       8A000 .rdata
       2F000 .reloc
        1000 .rsrc
      4CA000 .text
        1000 _RDATA

@JackBoosY JackBoosY mentioned this pull request Sep 18, 2021
Closed
@lgritz
Copy link

lgritz commented Sep 18, 2021

@JackBoosY so the PUBLIC/PRIVATE is ok the way it is, the problem was just that the debug OIIO build was selecting the release squish build?

That sounds like it's probably a bug in my FindLibsquish.cmake. Let me see if I can produce a fix for that.

@lgritz
Copy link

lgritz commented Sep 18, 2021

You know what? OIIO's CI tests Windows, but only release build. Adding a debug build to see if I can reproduce.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 18, 2021 •
edited
Loading

AFAIU multiple issues are discussed here.

  1. Windows, vcpkg issue: app-local deployment of squish.dll. Independent of exported cmake configuration. But possible related to build types/item 3, [OpenImageIO] [libsquish] Dependent squish library is not copied in debug mode #20233 (comment).
  2. All platforms, OIIO issue: Passing linking information for static linkage. Yes, this means PUBLIC in cmake, but Libs.private in pkg-config. And populating the link interface when consuming OIIO mea
  3. All platforms: OIIO/vcpkg/user integration: OIIO comes with a set of cmake find modules. These modules must be installed with OIIO, in order to populate the target link interface when consuming OIIO. The problem is that these modules are not prepared to deal with vcpkg debug+relase setup, in particular the use of different binary names (d debug suffix). For some dependencies, this is patched in vcpkg's port.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Sep 18, 2021 •
edited
Loading

@lgritz The result of dumpbin proves that if we want to use oiio.lib, you must add squish.lib to the link queue, which means that libsquish should be exported to the INTERFACE_LINK_LIBRARIES of openimageioTargets.cmake, which means that PUBLIC should be used instead of PRIVATE.
Otherwise, we will meet the LNK2019 problem.

@JackBoosY
Copy link
Contributor Author

  1. Windows, vcpkg issue: app-local deployment of squish.dll. Independent of exported cmake configuration. But possible related to build types/item 3, [OpenImageIO] [libsquish] Dependent squish library is not copied in debug mode #20233 (comment).

When users use libsquish through vcpkg, they should use Findlibsquish.cmake installed in share/OpenImageIO, which also solves this issue.

  1. All platforms, OIIO issue: Passing linking information for static linkage. Yes, this means PUBLIC in cmake, but Libs.private in pkg-config. And populating the link interface when consuming OIIO mea

Yes, agreed.

  1. All platforms: OIIO/vcpkg/user integration: OIIO comes with a set of cmake find modules. These modules must be installed with OIIO, in order to populate the target link interface when consuming OIIO. The problem is that these modules are not prepared to deal with vcpkg debug+relase setup, in particular the use of different binary names (d debug suffix). For some dependencies, this is patched in vcpkg's port.

As I said in 1, even if libsquish does not export its config.cmake/targets.cmake, we can use the installed Findlibsquish.cmake to find and use it.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 18, 2021

As I said in 1, even if libsquish does not export its config.cmake/targets.cmake, we can use the installed Findlibsquish.cmake to find and use it.

Basically yes. But:

File list for libsquish, from microsoft.vcpkg.ci:

libsquish:/bin/squish.dll
libsquish:/debug/bin/squishd.dll
libsquish:/debug/lib/squishd.lib
libsquish:/include/squish.h
libsquish:/include/squish_export.h
libsquish:/lib/squish.lib
libsquish:/share/libsquish/copyright
libsquish:/share/libsquish/vcpkg_abi_info.txt

@JackBoosY
Copy link
Contributor Author

@dg0yt Fine, I will make a PR to export the libsquish targets.

@JackBoosY JackBoosY mentioned this pull request Sep 18, 2021
Merged
@dg0yt
Copy link
Contributor

dg0yt commented Sep 21, 2021

So what CMake is supposed to do is understand this, and for PRIVATE library dependencies, pass on the library interface if it's static but not dynamic.

If I understand correctly, marking everything as PUBLIC seems like overkill.

@lgritz After reading another PR, I would like to another insight:
The effect of the PUBLIC and PRIVATE keywords is consistent over various target_... cmake commands.
The purpose is less obvious for target_link_libraries (PRIVATE perhaps only useful for header-only libraries or file-less configuration sets) but it is quite obvious for target_compile_definitions:

if(BUILD_SHARED_LIBS)
    target_compile_options(lcms2 PRIVATE -DCMS_DLL_BUILD)
    target_compile_options(lcms2 PUBLIC -DCMS_DLL)
endif()
target_compile_options(lcms2 PRIVATE -DUNICODE -D_UNICODE)

CMake doesn't require any PUBLIC/PRIVATE/INTERFACE keywords for target_link_libraries, but everything is public then, and you must not mix the forms.

@lgritz
Copy link

lgritz commented Sep 21, 2021

I don't buy the "only header libraries are PRIVATE", that seems to contradict everything I read in Craig Scott's book, "Professional CMake," and many other sources.

From Professional CMake, 9th ed, p. 248,

CMake handles some special cases specific to linking static libraries. If a library A is listed as a PRIVATE dependency for a static library target B, then A will effectively be treated as a PUBLIC dependency as far as linking is concerned (and only for linking). This is because the private A library will still need to be added to the linker command line of anything linking to B in order for symbols from A to be found at link time. If B was a shared library, the private library A that it depends on would not need to be listed on the linker command line. This is all handled transparently by CMake, so the developer typically doesn’t need to concern themselves with the details beyond specifying the PUBLIC, PRIVATE and INTERFACE dependencies with target_link_libraries().

I believe that PRIVATE is the right designator to use here, because the squish library is used internally to OIIO, but is not part of the exported API. If squish is a dynamic library, there is no need to propagate this transitive dependency because it's handled by the linker; if squish is a static library, CMake already is supposed to know that (for this one case) it needs to treat it as public and propagate the dependency. I think we're using PUBLIC/PRIVATE correctly, but something else is going wrong for this one library (circumstantial evidence: if I was really using PRIVATE incorrectly, wouldn't ALL the libraries have the same failures?).

@dg0yt
Copy link
Contributor

dg0yt commented Sep 21, 2021

CMake handles some special cases specific to linking static libraries. If a library A is listed as a PRIVATE dependency for a static library target B, then A will effectively be treated as a PUBLIC dependency as far as linking is concerned (and only for linking).

Okay. I would prefer to see "special cases" in the CMake documentation, not in secondary literature. And CMake documentation just has the following:

Libraries and targets following PRIVATE are linked to, but are not made part of the link interface.

I guess the behaviour comes down to this code:
https://github.com/Kitware/CMake/blob/6261210a671aa20d691b816df6a2237777995b06/Source/cmTargetLinkLibrariesCommand.cxx#L482-L499
which uses $<LINK_ONLY:...> when PRIVATE is used while touching a static library. It seems right ... but cmake documentation also states

Content of ... except when evaluated in a link interface while propagating Transitive Usage Requirements, in which case it is the empty string.

I am confused now 😕

@lgritz
Copy link

lgritz commented Sep 21, 2021

"Linked to, but not part of the link interface" is terse, but has all the information. For shared libraries, A linking B means that an app linking A only needs to name A (which itself retains the reference to B). But for static libraries, this is not the case; an app using A needs to also link B explicitly, and CMake is supposed to track this.

Here's some primary documentation that implies this: https://cmake.org/cmake/help/v3.21/guide/tutorial/Selecting%20Static%20or%20Shared%20Libraries.html#mathfunctions-cmakelists-txt-add-library-static
In this example, SqrtLibrary is a static library with a compiled component, and the MathFunctions library calls target_link_libraries(MathFunctions PRIVATE SqrtLibrary) so it's definitely not the case that PRIVATE is only for header-only libraries. They expect this example to work, presumably.

Also, to add the the confusion, I'm using PRIVATE for all the external libraries, yet there is only this one library that we are having trouble with. To me, that indicates that to really understand what's going on, we need to understand what is different about this library.

I have a few hypotheses, but not having a Windows machine and being a total vcpkg noob, I'm not sure which are plausible, but maybe you can recognize which have merit:

  • Maybe my FindLibsquish.cmake is subtly broken and is causing this difficulty (not hard to believe; we've already established that it's broken as far as finding the right debug version of the library).
  • Maybe all the other libraries pick up dynamic libraries, but the vcpkg recipe for libsquish is only building a static version of the library, and that's causing problems somehow (either by itself, or some flaw in my build system that isn't very robust with static libraries)?
  • Maybe that static libsquish library is not built with POSITION_INDEPENDENT_CODE property enabled and so can't be mixed with the other dynamic libraries?
  • Maybe libsquish is fine, and the problem for downstream is simply that I am not installing FindSquish.cmake and calling find_dependency(libsquish) in my exported cmake config, so the downstream app fails at link time simply because it can't find libsquish, not because there's anything wrong with it or with the exports?

@lgritz
Copy link

lgritz commented Sep 21, 2021

Also, https://cmake.org/cmake/help/v3.21/prop_tgt/LINK_INTERFACE_LIBRARIES.html :

By default linking to a shared library target transitively links to targets with which the library itself was linked. For an executable with exports (see the ENABLE_EXPORTS target property) no default transitive link dependencies are used. This property replaces the default transitive link dependencies with an explicit list. When the target is linked into another target using the target_link_libraries() command, the libraries listed (and recursively their link interface libraries) will be provided to the other target also. If the list is empty then no transitive link dependencies will be incorporated when this target is linked into another target even if the default set is non-empty. This property is initialized by the value of the CMAKE_LINK_INTERFACE_LIBRARIES variable if it is set when a target is created. This property is ignored for STATIC libraries.

Can you tell if the vcpkg libsquish is generating shared, static, or both? I'm still gravitating toward suspecting that the problem here might be that the vcpkg libsquish build itself is maybe making a static library only that is not correctly set up to mix properly with the other dynamic libraries?

@lgritz
Copy link

lgritz commented Sep 21, 2021

To hammer a previous point with a concrete example: Why does PRIVATE link against gif library work, and against libtiff works, but PRIVATE link against squish does not? I think that when we can articulate the answer to that question, we'll know how to solve the problem for squish once and for all.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 23, 2021

@lgritz wrote:

Why does PRIVATE link against gif library work, and against libtiff works, but PRIVATE link against squish does not?

Let's refer to "libsquish", to avoid confusion with the software named "Squish" which has an official Find module in CMake.

Depending on actual values are given is to target_link_libraries, the exported config is different for various libraries. I built openimageio[gif,webp]:x64-linux from vpckg master now (so with vcpkg patches). The INTERFACE_LINK_LIBRARIES for OpenImageIO::OpenImageIO has the following entries:

OpenImageIO::OpenImageIO_Util
$<$<TARGET_EXISTS:Imath::Imath>:Imath::Imath>
$<$<TARGET_EXISTS:Imath::Half>:Imath::Half>
$<$<TARGET_EXISTS:IlmBase::Imath>:IlmBase::Imath>
$<$<TARGET_EXISTS:IlmBase::Half>:IlmBase::Half>
IlmBase::IlmBaseConfig
IlmBase::Half
IlmBase::IexMath
IlmBase::IlmBaseConfig
IlmBase::IlmBaseConfig
IlmBase::IlmBaseConfig
IlmBase::Iex
Threads::Threads
-pthread
$<LINK_ONLY:$<$<TARGET_EXISTS:OpenEXR::OpenEXR>:OpenEXR::OpenEXR>>
$<LINK_ONLY:$<$<TARGET_EXISTS:OpenEXR::IlmImf>:OpenEXR::IlmImf>>
$<LINK_ONLY:$<$<TARGET_EXISTS:IlmBase::IlmThread>:IlmBase::IlmThread>>
$<LINK_ONLY:$<$<TARGET_EXISTS:IlmBase::Iex>:IlmBase::Iex>>
$<LINK_ONLY:OpenEXR::IlmImfConfig>
$<LINK_ONLY:OpenEXR::IlmImfConfig>
$<LINK_ONLY:IlmBase::Iex>
$<LINK_ONLY:IlmBase::Half>
$<LINK_ONLY:IlmBase::Imath>
$<LINK_ONLY:IlmBase::IlmThread>
$<LINK_ONLY:ZLIB::ZLIB>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::Half>
$<LINK_ONLY:IlmBase::IexMath>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::Iex>
$<LINK_ONLY:Threads::Threads>
$<LINK_ONLY:Libsquish::Libsquish>
${_IMPORT_PREFIX}/lib/libgif.a
$<LINK_ONLY:heif>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libjpeg.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libjpeg.a>
$<LINK_ONLY:OpenEXR::IlmImfConfig>
$<LINK_ONLY:OpenEXR::IlmImfConfig>
$<LINK_ONLY:IlmBase::Iex>
$<LINK_ONLY:IlmBase::Half>
$<LINK_ONLY:IlmBase::Imath>
$<LINK_ONLY:IlmBase::IlmThread>
$<LINK_ONLY:ZLIB::ZLIB>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::Half>
$<LINK_ONLY:IlmBase::IexMath>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::IlmBaseConfig>
$<LINK_ONLY:IlmBase::Iex>
$<LINK_ONLY:Threads::Threads>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libjpeg.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libjpeg.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libtiff.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libtiffd.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/liblzma.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/liblzmad.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libjpeg.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libjpeg.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libz.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libz.a>
$<LINK_ONLY:m>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libjpeg.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libjpeg.a>
$<LINK_ONLY:ZLIB::ZLIB>
$<LINK_ONLY:WebP::WebP>
$<LINK_ONLY:WebP::WebPDemux>
$<LINK_ONLY:ZLIB::ZLIB>
$<LINK_ONLY:$<$<TARGET_EXISTS:OpenColorIO::OpenColorIO>:OpenColorIO::OpenColorIO>>
$<LINK_ONLY:$<$<TARGET_EXISTS:OpenColorIO::OpenColorIOHeaders>:OpenColorIO::OpenColorIOHeaders>>
$<LINK_ONLY:$<$<BOOL:>:pugixml::pugixml>>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libbz2.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libbz2d.a>
$<LINK_ONLY:ZLIB::ZLIB>
${_IMPORT_PREFIX}/lib/libboost_filesystem.a
${_IMPORT_PREFIX}/lib/libboost_system.a
${_IMPORT_PREFIX}/lib/libboost_thread.a
$<LINK_ONLY:-lpthread>
${_IMPORT_PREFIX}/lib/libboost_chrono.a
${_IMPORT_PREFIX}/lib/libboost_date_time.a
${_IMPORT_PREFIX}/lib/libboost_atomic.a
$<LINK_ONLY:rt>
$<LINK_ONLY:dl>

So the form varies:

  • libsquish: $<LINK_ONLY:Libsquish::Libsquish>
  • gif: ${_IMPORT_PREFIX}/lib/libgif.a
  • tiff: Given tiff's dependencies, a series of entries: 
    $<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libtiff.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libtiffd.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/liblzma.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/liblzmad.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libjpeg.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libjpeg.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libz.a>
$<$<NOT:$<CONFIG:DEBUG>>:${_IMPORT_PREFIX}/lib/libz.a>
$<$<CONFIG:DEBUG>:${_IMPORT_PREFIX}/debug/lib/libz.a>
$<LINK_ONLY:m>

This illustrates the pros and cons of the variants:

  • libsquish is added as a target. So it must be imported as a target, too. In a config file, this can be done with find_dependency. Given the oiio's own FindLibSquish.cmake module, this implies that the module must be deployed and made available via CMAKE_MODULE_PATH.
  • gif is added as path to the archive file. CMake doesn't have to resolve anything in this case.
    However, it won't link the debug binary in a debug build, which can lead to subtle errors when linking an executable to both variants.
  • tiff is added as path to the archive, with release and debug variants. This is the proper form when using paths.
    But all its transitive usage requirements are listed explicitly, i.e. it repeats details of the build of tiff. This would be independent of these details, and much shorter, with target TIFF::TIFF (plus find_dependency).

@lgritz
Copy link

lgritz commented Sep 23, 2021

Would it be better if I only used targets for dependencies that reliably export config files (and have a corresponding find_dependency in my OpenImageIOConfig.cmake), and for all other cases, use old fashioned ${FOO_LIBRARIES} variable?

@JackBoosY
Copy link
Contributor Author

@lgritz Yes, using module mode to find dependencies can avoid calling find_dependency.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 24, 2021 •
edited
Loading

Yes, using module mode to find dependencies can avoid calling find_dependency.

I agree on the "Yes", but I don't undestand the second part. IMO the modern principles are:

  • Prefer imported targets over ${FOO_LIBRARIES}, because targets carry various properties at once, including transitive usage requirements.
  • Prefer config files over Find modules because config file are exported from the build of the exporting package, exactly based on the configuration, instead of needing to guess ex-post.
  • Prefer find_dependency over find_package when looking for dependencies from config scripts. find_dependency is a useful macro, but it is not made for Find modules.

The downside are that

  • not all packages provide config files
  • the official Find modules provided by CMake only slowly added targets

so you have to be careful about minimum versions to be installed on the user side.

@lgritz Thanks for your persistence on clarifying PRIVATE with target_link_libraries.

@lgritz
Copy link

lgritz commented Sep 24, 2021

I had somewhat recently start switching to targets, even for my Find* modules, mostly because of the compactness (just say the target in target_link_libraries, and it also takes care of the includes for you) and simply because I thought it was "modern" usage. I did not consider any downsides, which now I see. I should probably simply back out of that, and to back to the variables at least for the things where I've written my own Find modules, which tend to be fairly simple (maybe too simple). I never considered the "missing for my exported config" issue, probably because I tend to use dynamic libraries myself, where the transitive linkage takes care of itself.

I appreciate your patience with me on this. I'm trying to get it right, but especially not being a windows user and tending not to use static libraries, I think I was not myself bumping into some of the edge cases that are now more apparent to me.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 24, 2021

Yeah, vcpkg comes with static, and it comes with combined debug+release. It is not really an edge case, but that's why custom find modules often fail. At least, please make use of select_library_configurations where possible. I usually take CMake's FindZLIB.cmake as a guideline.

@lgritz
Copy link

lgritz commented Sep 25, 2021

Hopefully this will make things a little easier for downstream builders: AcademySoftwareFoundation/OpenImageIO#3108

Basically, my new strategy is:
(a) Use upstream config files and their targets exclusively in cases where they are reliable and available across all versions of the upstream dependency that we support.
(b) When config files aren't always available, use Find modules distributed with CMake, and use targets if it provides them and they seem to work in all CMake versions we support (we presume that if distributed with CMake, they probably work well).
(c) When those aren't available, such as for less popular libraries like libsquish, use our own custom Find modules and in that case, exclusively use the cmake variables -- not the targets! -- because downstream libraries won't know about those targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] approved these changes

@PhoebeHui PhoebeHui PhoebeHui approved these changes

Assignees

@PhoebeHui PhoebeHui

Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JackBoosY @jmacey @BillyONeal @lgritz @dg0yt @PhoebeHui