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

draco: Improve installation and packaging support. #775

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

tomfinegan
Copy link
Contributor

  • Fixed omission of draco_version.h when installed in static
    configurations.
  • Remove DracoConfig.cmake: it was broken and we actually use
    draco-config.cmake.
  • Update FindDraco.cmake so it actually has a chance of working
    properly. Fixed usage of find_path() and subsequent related errors.
  • Correct the usage of CMakePackageConfigHelpers in installation target setup
    and the config template.
  • Add a CMake version file.
  • Normalize the Draco variables exposed in CMake.
    • draco_FOUND -> DRACO_FOUND
    • DRACO_INCLUDE_DIRS, draco_INCLUDE_DIRS -> DRACO_INCLUDE_DIR
    • DRACO_LIBRARIES, draco_LIBRARIES -> DRACO_LIBRARY
    • DRACO_LIBRARY_DIRS, draco_LIBRARY_DIRS -> DRACO_LIBRARY_DIR
    • draco_VERSION_STRING -> DRACO_VERSION
  • Use full paths variants of GNUInstallDirs variables to init our
    install paths.

Fixes #764

- Fixed omission of draco_version.h when installed in static
  configurations.
- Remove DracoConfig.cmake: it was broken and we actually use
  draco-config.cmake.
- Update FindDraco.cmake so it actually has a chance of working
  properly. Fixed usage of find_path() and subsequent related errors.
- Correct the usage of CMakePackageConfigHelpers in installation target setup
  and the config template.
- Add a CMake version file.
- Normalize the Draco variables exposed in CMake.
  - draco_FOUND -> DRACO_FOUND
  - DRACO_INCLUDE_DIRS, draco_INCLUDE_DIRS -> DRACO_INCLUDE_DIR
  - DRACO_LIBRARIES, draco_LIBRARIES -> DRACO_LIBRARY
  - DRACO_LIBRARY_DIRS, draco_LIBRARY_DIRS -> DRACO_LIBRARY_DIR
  - draco_VERSION_STRING -> DRACO_VERSION
- Use full paths variants of GNUInstallDirs variables to init our
  install paths.

Fixes #764
Copy link
Collaborator

@FrankGalligan FrankGalligan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@FrankGalligan FrankGalligan self-requested a review December 6, 2021 16:10
Copy link
Collaborator

@FrankGalligan FrankGalligan left a comment

Choose a reason for hiding this comment

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

LGTM.

@JackBoosY
Copy link

JackBoosY commented Dec 24, 2021

I don't agree with this. We should use export to export these targets and use cmake to generate configuration files instead of hard coding them.
The usage issue still exists in Windows:

1> CMake Error at F:/vcpkg/installed/x86-windows/share/draco/draco-config.cmake:11 (message):
1>   File or directory draco referenced by variable DRACO_LIBRARY does not exist
1>   !
1> Call Stack (most recent call first):
1>   F:/vcpkg/installed/x86-windows/share/draco/draco-config.cmake(28): (set_and_check)
1>   F:/vcpkg/scripts/buildsystems/vcpkg.cmake(788): (_find_package)
1>   C:\Users\usr\source\repos\CMakeProject2\CMakeProject2/CMakeLists.txt(11): (find_package)
1> 
1> 
1> -- Configuring incomplete, errors occurred!

draco-config.cmake:

get_filename_component(VCPKG_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_DIR}/../../" ABSOLUTE)
####### Expanded from @PACKAGE_INIT@ by configure_package_config_file() #######
####### Any changes to this file will be overwritten by the next CMake run ####
####### The input file was draco-config.cmake.template                            ########

get_filename_component(PACKAGE_PREFIX_DIR "${CMAKE_CURRENT_LIST_DIR}/../../" ABSOLUTE)

macro(set_and_check _var _file)
  set(${_var} "${_file}")
  if(NOT EXISTS "${_file}")
    message(FATAL_ERROR "File or directory ${_file} referenced by variable ${_var} does not exist !")
  endif()
endmacro()

macro(check_required_components _NAME)
  foreach(comp ${${_NAME}_FIND_COMPONENTS})
    if(NOT ${_NAME}_${comp}_FOUND)
      if(${_NAME}_FIND_REQUIRED_${comp})
        set(${_NAME}_FOUND FALSE)
      endif()
    endif()
  endforeach()
endmacro()

####################################################################################
set_and_check(DRACO_INCLUDE_DIR "${VCPKG_IMPORT_PREFIX}/include")
set_and_check(DRACO_LIB_DIR "${VCPKG_IMPORT_PREFIX}/lib")
set_and_check(DRACO_LIBRARY "draco")

Please note the library name DRACO_LIBRARY should be draco.lib in Windows.

@SpaceIm
Copy link
Contributor

SpaceIm commented Apr 2, 2022

I agree with @JackBoosY

I don't understand these convoluted Find & config files (old one and new one). Why don't you properly export targets with regular CMake helpers and include the generated file in CMake config file like it's supposed to be done?

@tomfinegan
Copy link
Contributor Author

I agree with @JackBoosY

I don't understand these convoluted Find & config files (old one and new one). Why don't you properly export targets with regular CMake helpers and include the generated file in CMake config file like it's supposed to be done?

That's fine, but the Find and config files remain. They are working correctly, and have tests to prove they function correctly. What exactly do you need from an exports file that is not contained in the config or Find scripts?

If you're having trouble figuring out how to use the find/config files: see the test. It's quite simple to use them.

@SpaceIm
Copy link
Contributor

SpaceIm commented Apr 4, 2022

What exactly do you need from an exports file that is not contained in the config or Find scripts?

An imported target (modern CMake), not variables (old CMake).

Moreover current config file makes installed package non-relocatable I think: https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html#generating-a-package-configuration-file

What I want to say: don't try to write yourself non-standard config file based on find_library() etc, use CMake helpers and export target with a namespace, so that consumers can inject draco with:

find_package(draco REQUIRED CONFIG)
target_link_libraries(foo PRIVATE draco::draco)

@tomfinegan
Copy link
Contributor Author

I'll be happy to review a PR that adds the above, but the recommendation at present remains building from source. Short of that the find_package setup is known to work-- if you have a configuration that does not work, please file an issue that includes reproduction steps.

Moreover current config file makes installed package non-relocatable I think:
https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html#generating-a-package-configuration-file

As suggested in the linked document the draco config file is populated via configure_package_config_file(), and should be relocatable. Again, please file an issue if you have a reproducible problem.

Note that what you're asking for largely exists at present using the current config file. As demonstrated in the install test, which uses the config file with find_package():

https://github.com/google/draco/blob/master/src/draco/tools/install_test/CMakeLists.txt

It does use a variable to determine the name of the library to link, but that does not seem that onerous. The rest of the noise in there is required to handle platform specific hassles (e.g. rpath on MacOS).

@tomfinegan
Copy link
Contributor Author

As previously requested: please file a new issue instead of updating this PR. I'm locking the conversation here, but I'm happy to continue discussion in a new issue should you choose to file one.

@google google locked as off-topic and limited conversation to collaborators Apr 4, 2022
@tomfinegan tomfinegan deleted the install_updates branch April 8, 2022 21:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect install paths
5 participants