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

Add note on install_manifest.txt to usage.rst #996

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ihnorton
Copy link

@ihnorton ihnorton commented Jun 8, 2023

Add a note on how skbuild uses install_manifest, and warn that EPs can be problematic (no install_manifest in the expected directory -> no module in the wheel). #997

@henryiii
Copy link
Contributor

henryiii commented Jun 9, 2023

What does the install_manifest look like in superbuilds? Is it in subdirectories?

@ihnorton
Copy link
Author

ihnorton commented Jun 9, 2023

Is it in subdirectories?

Yes (related: https://github.com/orgs/scikit-build/discussions/997)

docs/usage.rst Outdated Show resolved Hide resolved
Compiled targets and other install files are identified by parsing the CMake-generated
``install_manifest.txt`` file from :func:`skbuild.constants.CMAKE_BUILD_DIR()`. Note:
if scikit-build is driving nested CMake projects (externalproject / "superbuild"), the
manifest file may not be present in the ``CMAKE_BUILD_DIR``.
Copy link
Contributor

@jcfr jcfr Jul 16, 2023

Choose a reason for hiding this comment

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

The way to currently deal with this is to explicitly include the cmake_install.cmake by adding code like the following in the outer project:

#-----------------------------------------------------------------------------
# Install inner project
message(STATUS "Adding install rules for inner project")
install(CODE "
include(\"${CMAKE_BINARY_DIR}/inner-build/cmake_install.cmake\")
")

Notes:

  • Make sure to tweak ${CMAKE_BINARY_DIR}/inner-build based on your project.
  • It may be possible to generalize this to include cmake_install.cmake from other projects also built using ExternalProject ...

ihnorton and others added 2 commits July 16, 2023 17:08
Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #996 (ce3e3d8) into main (bd8802f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #996   +/-   ##
=======================================
  Coverage   86.26%   86.26%           
=======================================
  Files          33       33           
  Lines        1587     1587           
  Branches      350      350           
=======================================
  Hits         1369     1369           
  Misses        155      155           
  Partials       63       63           

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.

None yet

3 participants