CMake: install draco.dll in bin folder #838

SpaceIm · 2022-04-08T12:07:31Z

closes #834

this option allows to disable installation of static lib when shared lib is also built for non-msvc compilers

src/draco/tools/install_test/CMakeLists.txt

tomfinegan · 2022-04-08T17:23:20Z

It took me some time to understand the purpose of this install_test, so yes moving dll or injecting its location in PATH is still required while running install_check executable.

I'm not sure why its purpose isn't obvious: we're making sure the installed dev package can function by building something that requires draco.

I believe that all this logic, as well as CMAKE_PREFIX_PATH & CMAKE_INSTALL_RPATH (enabling CMAKE_INSTALL_RPATH_USE_LINK_PATH externally is far better here) should be moved in test.py.

Discussions about this here aren't that productive. If you believe your above statement to be correct, please update your PR and we'll see how the CI run goes.

BUILD_SHARED_LIBS doesn't make sense here in the CMake paradigm. It is supposed to control shared/static of current libraries, not external dependencies.

I have no idea what this statement means. BUILD_SHARED_LIBS is exactly how you control your build configuration per CMake documentation. Please elaborate.

Edit to add:

From BUILD_SHARED_LIBS (currently tagged as latest, but actually v3.23.0):

If present and true, this will cause all libraries to be built shared unless the library was explicitly added as a static library.
This variable is often added to projects as an option() so that each user of a project can decide if they want to build the project using shared or static libraries.

tomfinegan · 2022-04-08T18:18:17Z

Exception: install_check run failed!
exit_code: 3221225781

I believe the above exit code means the DLL not found at runtime.

- CMAKE_PREFIX_PATH, rpath injection etc are now defined externally in test.py - prefer cxx_std_11 compile feature instead of CXX_STANDARD - inject dll location to PATH env var on windows instead of copying the dll

SpaceIm · 2022-04-08T18:59:05Z

BUILD_SHARED_LIBS doesn't make sense here in the CMake paradigm. It is supposed to control shared/static of current libraries, not external dependencies.

I have no idea what this statement means. BUILD_SHARED_LIBS is exactly how you control your build configuration per CMake documentation. Please elaborate.

Edit to add:

From BUILD_SHARED_LIBS (currently tagged as latest, but actually v3.23.0):

If present and true, this will cause all libraries to be built shared unless the library was explicitly added as a static library.
This variable is often added to projects as an option() so that each user of a project can decide if they want to build the project using shared or static libraries.

BUILD_SHARED_LIBS controls the libraries defined in add_library() of current CMakeLists, not external libraries already built and discovered by find_package(). This is why I say it's odd to rely on BUILD_SHARED_LIBS in a CMakeLists where the only target is an executable. A CMakeLists is supposed to be agnostic regarding static/shared status of external dependencies, it's controlled externally (ie command line or Preset) by providing the path of the proper flavor with CMAKE_PREFIX_PATH.

tomfinegan · 2022-04-08T19:22:42Z

BUILD_SHARED_LIBS doesn't make sense here in the CMake paradigm. It is supposed to control shared/static of current libraries, not external dependencies.

I have no idea what this statement means. BUILD_SHARED_LIBS is exactly how you control your build configuration per CMake documentation. Please elaborate.
Edit to add:
From BUILD_SHARED_LIBS (currently tagged as latest, but actually v3.23.0):

If present and true, this will cause all libraries to be built shared unless the library was explicitly added as a static library.
This variable is often added to projects as an option() so that each user of a project can decide if they want to build the project using shared or static libraries.

BUILD_SHARED_LIBS controls the libraries defined in add_library() of current CMakeLists, not external libraries already built and discovered by find_package(). This is why I say it's odd to rely on BUILD_SHARED_LIBS in a CMakeLists where the only target is an executable. A CMakeLists is supposed to be agnostic regarding static/shared status of external dependencies, it's controlled externally (ie command line or Preset) by providing the path of the proper flavor with CMAKE_PREFIX_PATH.

When the install test runs:

test.py builds and installs draco in static and shared configurations
- Each config installs the result to its own custom prefix.
test.py builds and installs the test program in shared and static configurations.
- Each config installs the result to its own custom prefix.
test.py runs install_test in each configuration.

I suppose it's a bit hacky to abuse BUILD_SHARED_LIBS in the context of the install test program CMake build, but in the context of the test and what is being tested it makes perfect sense to me. I'm not sure why you're conflating the usage of BUILD_SHARED_LIBS in the test script with normal CMake usage or practices. Adding another variable is silly when BUILD_SHARED_LIBS can be reused in the test. As you say: it does not influence an executable only CMake build.

SpaceIm · 2022-04-08T19:33:17Z

CMake Error at CMakeLists.txt:20 (find_package):
  Could not find a package configuration file provided by "draco" with any of
  the following names:

    dracoConfig.cmake
    draco-config.cmake

  Add the installation prefix of "draco" to CMAKE_PREFIX_PATH or set
  "draco_DIR" to a directory containing one of the above files.  If "draco"
  provides a separate development package or SDK, be sure it has been
  installed.

draco-config.cmake is not installed in a standard location. See https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure

<prefix>/                                                       (W)
<prefix>/(cmake|CMake)/                                         (W)
<prefix>/<name>*/                                               (W)
<prefix>/<name>*/(cmake|CMake)/                                 (W)
<prefix>/(lib/<arch>|lib*|share)/cmake/<name>*/                 (U)
<prefix>/(lib/<arch>|lib*|share)/<name>*/                       (U)
<prefix>/(lib/<arch>|lib*|share)/<name>*/(cmake|CMake)/         (U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/cmake/<name>*/         (W/U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/<name>*/               (W/U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/<name>*/(cmake|CMake)/ (W/U)

Currently it's <prefix>/share/cmake.

I'll change the install destination (the most common destination is ${CMAKE_INSTALL_LIBDIR}/cmake/draco).

tomfinegan · 2022-04-08T19:37:39Z

draco-config.cmake is not installed in a standard location. See https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure

<prefix>/                                                       (W)
<prefix>/(cmake|CMake)/                                         (W)
<prefix>/<name>*/                                               (W)
<prefix>/<name>*/(cmake|CMake)/                                 (W)
<prefix>/(lib/<arch>|lib*|share)/cmake/<name>*/                 (U)
<prefix>/(lib/<arch>|lib*|share)/<name>*/                       (U)
<prefix>/(lib/<arch>|lib*|share)/<name>*/(cmake|CMake)/         (U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/cmake/<name>*/         (W/U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/<name>*/               (W/U)
<prefix>/<name>*/(lib/<arch>|lib*|share)/<name>*/(cmake|CMake)/ (W/U)

Currently it's <prefix>/shared/cmake.

I'll change the install destination (the most common destination is ${CMAKE_INSTALL_LIBDIR}/cmake/draco).

I'm uncertain how you're reaching that conclusion for $prefix/share/cmake. That's where it was going prior to this PR, at least.

SpaceIm · 2022-04-08T19:39:33Z

I'm uncertain how you're reaching that conclusion for $prefix/share/cmake. That's where it was going prior to this PR, at least.

Yes, but the test was hiding an issue since CMAKE_PREFIX_PATH was set with the exact location of the config file, while common usage is to put the root install dir in CMAKE_PREFIX_PATH (see the recommendation when find_package() fails: Add the installation prefix of "draco" to CMAKE_PREFIX_PATH).

SpaceIm · 2022-04-08T19:46:54Z

I also see that config version file name is wrong, it should be draco-config-version.cmake. See https://cmake.org/cmake/help/latest/command/find_package.html#search-modes

I'll fix this also.

tomfinegan · 2022-04-08T19:55:01Z

I also see that config version file name is wrong, it should be draco-config-version.cmake. See https://cmake.org/cmake/help/latest/command/find_package.html#search-modes

I'll fix this also.

Agreed on both counts-- I'm going to fix those out of band. I don't want to mix obvious bug fixes in with this PR that's adding functionality.

SpaceIm · 2022-04-08T20:11:12Z

RUN command: /Users/runner/work/draco/draco/src/draco/tools/install_test/_test_install_shared/bin/install_check
COMMAND output:
dyld: Library not loaded: @rpath/libdraco.4.dylib
  Referenced from: /Users/runner/work/draco/draco/src/draco/tools/install_test/_test_install_shared/bin/install_check
  Reason: image not found

I guess CMAKE_INSTALL_RPATH_USE_LINK_PATH=ON doesn't work here because config file doesn't provide a proper imported target which can be used in target_link_libraries().
I'll revert this part since it would require too much change (I have a branch for this but it changes CMakeLists so that it builds either static or shared, and you have moved away from this logic since 1.4.0 :s ).

tomfinegan · 2022-04-08T20:22:23Z

RUN command: /Users/runner/work/draco/draco/src/draco/tools/install_test/_test_install_shared/bin/install_check
COMMAND output:
dyld: Library not loaded: @rpath/libdraco.4.dylib
  Referenced from: /Users/runner/work/draco/draco/src/draco/tools/install_test/_test_install_shared/bin/install_check
  Reason: image not found
I guess CMAKE_INSTALL_RPATH_USE_LINK_PATH=ON doesn't work here because config file doesn't provide a proper imported target which can be used in target_link_libraries(). I'll revert this part since it would require too much change (I have a branch for this but it changes CMakeLists so that it builds either static or shared, and you have moved away from this logic since 1.4.0 :s ).

Sounds good. The ability to build shared+static targets simultaneously on macos/linux is not something I want to do drop without a really good reason.

SpaceIm · 2022-04-08T20:27:14Z

A good reason would be to provide a unique draco::draco target in CMake config file, regardless of shared/static flavor. It's not something you can achieve when you build both static & shared (when you build both, folks usually provide 1 target name for static and another for shared, but IMOO it's not a good practice, and here it would be even worse since msvc doesn't build both). But it's out of scope of this PR ;)

tomfinegan · 2022-04-08T20:33:55Z

A good reason would be to provide a unique draco::draco target in CMake config file, regardless of shared/static flavor. It's not something you can achieve when you build both static & shared (when you build both, folks usually provide 1 target name for static and another for shared, but IMOO it's not a good practice, and here it would be even worse since msvc doesn't build both). But it's out of scope of this PR ;)

CMake isn't the only build system draco needs to support, and MSVC is not the only toolchain.

… with CMAKE_INSTALL_RPATH

cmake/draco_install.cmake

cmake/draco_options.cmake

cmake/draco_install.cmake

cmake/draco-config.cmake.template

Fix windows DLL install location and update the install test with the following - Use cxx_std_11 compile feature instead of forcing CXX_STANDARD to C++11. - Move install test prefix handling to test.py (was in the install test CMakeLists.txt). - Use CMAKE_INSTALL_RPATH and add dylib location to PATH instead of copying it only on Windows-- makes test more generic and makes install_test much simpler.

SpaceIm added 2 commits April 8, 2022 14:34

install draco.dll in bin folder

3b06588

add DRACO_INSTALL_STATIC option

88b91d7

this option allows to disable installation of static lib when shared lib is also built for non-msvc compilers

SpaceIm force-pushed the fix/install-destination-dll-draco branch from 58d4a95 to 88b91d7 Compare April 8, 2022 12:35

SpaceIm commented Apr 8, 2022

View reviewed changes

src/draco/tools/install_test/CMakeLists.txt Show resolved Hide resolved

cleanup CMakeLists of install_test

7872b2d

- CMAKE_PREFIX_PATH, rpath injection etc are now defined externally in test.py - prefer cxx_std_11 compile feature instead of CXX_STANDARD - inject dll location to PATH env var on windows instead of copying the dll

SpaceIm added 2 commits April 8, 2022 21:47

fix install destination of CMake config files

18f3c10

fix CMake config version file name

7af6206

tomfinegan mentioned this pull request Apr 8, 2022

Correct the install location for CMake configuration files. #839

Merged

inject path of draco shared lib in executable's rpath of install tree…

3f5a071

… with CMAKE_INSTALL_RPATH

tomfinegan suggested changes Apr 8, 2022

View reviewed changes

SpaceIm added 3 commits April 8, 2022 23:27

Merge branch 'master' into fix/install-destination-dll-draco

85abbb3

cmake formatting

9049a38

revert add DRACO_INSTALL_STATIC option

586fa33

SpaceIm changed the title ~~CMake: install draco.dll in bin folder & allow to not install static when shared lib is built~~ CMake: install draco.dll in bin folder Apr 8, 2022

SpaceIm mentioned this pull request Apr 8, 2022

draco: fix MinGW + modernize conan-io/conan-center-index#10128

Merged

4 tasks

tomfinegan approved these changes Apr 8, 2022

View reviewed changes

tomfinegan merged commit a737103 into google:master Apr 9, 2022

SpaceIm deleted the fix/install-destination-dll-draco branch April 9, 2022 00:26

SpaceIm mentioned this pull request Jul 12, 2022

Bump draco/1.5.3 conan-io/conan-center-index#11647

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: install draco.dll in bin folder #838

CMake: install draco.dll in bin folder #838

SpaceIm commented Apr 8, 2022

tomfinegan commented Apr 8, 2022 •

edited

Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 •

edited

Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 •

edited

Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 •

edited

Loading

SpaceIm commented Apr 8, 2022

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 •

edited

Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 •

edited

Loading

tomfinegan commented Apr 8, 2022

CMake: install draco.dll in bin folder #838

CMake: install draco.dll in bin folder #838

Conversation

SpaceIm commented Apr 8, 2022

tomfinegan commented Apr 8, 2022 • edited Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 • edited Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 • edited Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 • edited Loading

SpaceIm commented Apr 8, 2022

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 • edited Loading

tomfinegan commented Apr 8, 2022

SpaceIm commented Apr 8, 2022 • edited Loading

tomfinegan commented Apr 8, 2022

tomfinegan commented Apr 8, 2022 •

edited

Loading

SpaceIm commented Apr 8, 2022 •

edited

Loading

SpaceIm commented Apr 8, 2022 •

edited

Loading

SpaceIm commented Apr 8, 2022 •

edited

Loading

SpaceIm commented Apr 8, 2022 •

edited

Loading

SpaceIm commented Apr 8, 2022 •

edited

Loading