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

Incorrect install paths #764

Closed
jansol opened this issue Nov 10, 2021 · 3 comments · Fixed by #775
Closed

Incorrect install paths #764

jansol opened this issue Nov 10, 2021 · 3 comments · Fixed by #775
Labels
cmake Bugs and feature requests related to Draco's CMake build

Comments

@jansol
Copy link

jansol commented Nov 10, 2021

When given an absolute path for e.g. CMAKE_INSTALL_LIBDIR, cmake does NOT prepend CMAKE_INSTALL_PREFIX, that is only done for relative paths (see the CMake manual)

However path wrangling in draco_install.cmake always unconditionally prepends the prefix, leading to broken installs e.g. on NixOS. Nix always passes absolute paths to cmake for the specific install directories, as they may have different prefixes in order to split large packages into smaller ones (such as having a separate package for documentation or data files).

@tomfinegan
Copy link
Contributor

tomfinegan commented Nov 10, 2021

When given an absolute path for e.g. CMAKE_INSTALL_LIBDIR, cmake does NOT prepend CMAKE_INSTALL_PREFIX, that > is only done for relative paths (see the CMake manual)

Can you cite the section of the manual that you're referring to? I took a quick look over the script, and just reread the CMAKE_INSTALL_PREFIX documentation. There is no mention of behavioral changes relating to absolute paths. The GNUInstallDirs documentation is pretty clear that CMAKE_INSTALL_LIBDIR is not absolute and is built using CMAKE_INSTALL_PREFIX. CMAKE_INSTALL_FULL_LIBDIR is absolute, but the CMAKE_INSTALL_LIBDIR is not. Specifying an absolute path as the value of that variable appears incorrect by my reading.

The CMake install() code in Draco is ancient, and I see from looking at the documentation that things have changed, but the problem you're having downstream is clearly related to the behavior of Nix, which is not typical:

Nix always passes absolute paths to cmake for the specific install directories.

CMAKE_INSTALL_LIBDIR, according to CMake docs, is a relative path (GNUInstallDirs doc):

CMAKE_INSTALL_<dir>

Destination for files of a given type. This value may be passed to the DESTINATION options of install() commands for the
corresponding file type. It should typically be a path relative to the installation prefix so that it can be converted to an
absolute path in a relocatable way (see CMAKE_INSTALL_FULL_<dir>). However, an absolute path is also allowed.

Edit to add:

Looking at this a bit more it does seem that the install for Draco could use some updates-- I would be happy to look at a PR that makes this work a little better for your use case. The Draco install rules were not written with a fully custom install path structure in mind.

@jansol
Copy link
Author

jansol commented Nov 10, 2021

My bad, the part I was thinking of is about the destination parameter to install() and it is described right in the intro section of the page I linked:

If a relative path is given it is interpreted relative to the value of the CMAKE_INSTALL_PREFIX variable. The prefix can be relocated at install time using the DESTDIR mechanism explained in the CMAKE_INSTALL_PREFIX variable documentation.

If an absolute path (with a leading slash or drive letter) is given it is used verbatim.

Draco is explicitly prepending the prefix to all of the paths, i.e.:

install(TARGETS draco_decoder DESTINATION "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}")

Whereas the same result would be obtained with simply:

install(TARGETS draco_decoder DESTINATION "${CMAKE_INSTALL_BINDIR}")

...except in the case where CMAKE_INSTALL_BINDIR is an absolute path, like with Nix, where the latter would also work the way it is intended. Not sure what the best way to generate the paths for the pkg-config and cmake config files is, though.

@tomfinegan tomfinegan added cmake Bugs and feature requests related to Draco's CMake build and removed Waiting Response labels Nov 24, 2021
@tomfinegan
Copy link
Contributor

@jansol Thank you for the PR. I haven't had a chance to look it over yet, but we'll try to get this pulled in for the next release.

tomfinegan added a commit that referenced this issue Dec 4, 2021
- 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
tomfinegan added a commit that referenced this issue Dec 4, 2021
- 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
tomfinegan added a commit that referenced this issue Dec 6, 2021
- 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 path variants of GNUInstallDirs variables to init our
  install paths.

Fixes #764

* Add install() for draco-version.cmake.
danielgronlund pushed a commit to danielgronlund/draco that referenced this issue Aug 22, 2024
- 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 path variants of GNUInstallDirs variables to init our
  install paths.

Fixes google#764

* Add install() for draco-version.cmake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Bugs and feature requests related to Draco's CMake build
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants