Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions tests/packages/cmake_generated/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
cmake_minimum_required(VERSION 3.15)

project(
${SKBUILD_PROJECT_NAME}
LANGUAGES CXX
VERSION 1.2.3)

# Generate files at config time (configure_file) and at build time
# (add_custom_command) Note that bundling a generated file with sdist is out of
# scope for now. Note: cmake_generated/nested1/generated.py should try to open
# both generated and static files.
configure_file(src/cmake_generated/nested1/generated.py.in generated.py)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For inplace mode, it is run with cmake -S /path/to/src -B /path/to/src. So for it to "work" you would have to configure it in the appropriate src-like paths that would then be installed. Alternatively, and my preference, forget about supporting inplace since even getting that one to work by the user is a struggle. In that mode we don't do anything special either, we copy everything as if it were a pure python project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right... I don't remember from when we talked whether the long-term plan was to rely on running out of the build tree or only support an installed path. It makes sense to clarify one way or the other. It makes sense to me to discourage inplace and xfail it with an explanation (that inplace means the build tree must look like the package layout)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Build tree from inplace would be different from build tree with redirect (what #1170 would become). The latter would make sense to focus support more on, but the former is really just an ugly workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My inclination is

  1. Assume CMakeLists.txt authors are not concerning themselves with the layout of the build tree, make the default test scenario assume incompatible layouts, and be clear what the expected behavior is.
  2. If there is an expectation of using the build tree in some situations and there are caveats on the build tree location or layout, be clear that it is a special situation, and test and document it.

It sounds like inplace is in flux and step 2. could be beyond the scope of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, sounds good. We indeed need to document the different editable modes better, but that's for a separate PR.

# We always expect the install phase to run, so the build tree layout can be
# different than the package layout.
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/generated.py
DESTINATION ${SKBUILD_PROJECT_NAME}/nested1)

file(
GENERATE
OUTPUT configured_file
CONTENT "value written by cmake file generation")
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/configured_file
DESTINATION ${SKBUILD_PROJECT_NAME})

set(OUTPUT_FILE "${CMAKE_CURRENT_BINARY_DIR}/generated_data")
set(FILE_CONTENT "value written by cmake custom_command")
set(GENERATE_SCRIPT "file(WRITE \"${OUTPUT_FILE}\" \"${FILE_CONTENT}\")")
add_custom_command(
OUTPUT "${OUTPUT_FILE}"
COMMAND "${CMAKE_COMMAND}" -P
"${CMAKE_CURRENT_BINARY_DIR}/generate_file.cmake"
DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/generate_file.cmake"
COMMENT "Generating ${OUTPUT_FILE} using CMake scripting at build time"
VERBATIM)
file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/generate_file.cmake"
"${GENERATE_SCRIPT}")
add_custom_target(generate_file ALL DEPENDS "${OUTPUT_FILE}")
install(FILES "${OUTPUT_FILE}" DESTINATION ${SKBUILD_PROJECT_NAME}/namespace1)

add_library(pkg MODULE src/cmake_generated/pkg.cpp)

if(NOT WIN32)
# Explicitly set the bundle extension to .so
set_target_properties(pkg PROPERTIES SUFFIX ".so")
endif()

# Set the library name to "pkg", regardless of the OS convention.
set_target_properties(pkg PROPERTIES PREFIX "")
install(TARGETS pkg DESTINATION ${SKBUILD_PROJECT_NAME})
24 changes: 24 additions & 0 deletions tests/packages/cmake_generated/pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
[build-system]
requires = ["scikit-build-core"]
build-backend = "scikit_build_core.build"

[project]
name = "cmake_generated"
dynamic = ["version"]

[tool.scikit-build]
# Bundling a generated file in the sdist is not supported at this time.
# sdist.cmake = false
wheel.license-files = []
wheel.exclude = ["**.cpp", "**.in"]

[tool.scikit-build.metadata.version]
provider = "scikit_build_core.metadata.regex"
input = "CMakeLists.txt"
regex = 'project\([^)]+ VERSION (?P<value>[0-9.]+)'

[[tool.scikit-build.generate]]
path = "cmake_generated/_version.py"
template = '''
__version__ = "${version}"
'''
67 changes: 67 additions & 0 deletions tests/packages/cmake_generated/src/cmake_generated/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
"""Package that includes several non-Python non-module files.

Support some test cases aimed at testing our ability to find generated files
and static package data files in editable installations.

We are exercising the importlib machinery to find files that
are generated in different phases of the build and in different parts of
the package layout to check that the redirection works correctly in an
editable installation.

The test package includes raw data files and shared object libraries that
are accessed via `ctypes`.

We test files (generated and static)

* at the top level of the package,
* in subpackages, and
* in a namespace package.

We test access

* from modules at the same level as the files,
* one level above and below, and
* from parallel subpackages.

Question: Do we want to test both relative and absolute imports or just one or the other?
Copy link
Collaborator

Choose a reason for hiding this comment

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

We did have issues with absolute and relative imports behaving differently. We should probably try to support it, but where do you have in mind to check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be relevant to interdependent submodules, if parts of the package were in the source tree and parts were in the build tree, so I would say between nested1.generated and nested2

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the only difference I can think of in there is breaking up the circular dependency and importing only the module and not any from ... import. I don't think we actually have any control on that thing though and would be independent of the mechanism (as long as it can resolve with or without navigating the potential circular dependencies)

"""

import ctypes
import sys
from importlib.resources import as_file, files, read_text

try:
from ._version import __version__
except ImportError:
__version__ = None


def get_static_data():
return read_text("cmake_generated", "static_data").rstrip()


def get_configured_data():
return files().joinpath("configured_file").read_text().rstrip()


def get_namespace_static_data():
# read_text is able to handle a namespace subpackage directly, though `files()` is not.
return read_text("cmake_generated.namespace1", "static_data").rstrip()


def get_namespace_generated_data():
# Note that `files("cmake_generated.namespace1")` doesn't work.
# Ref https://github.com/python/importlib_resources/issues/262
return (
files().joinpath("namespace1").joinpath("generated_data").read_text().rstrip()
)


def ctypes_function():
if sys.platform == "win32":
lib_suffix = "dll"
else:
lib_suffix = "so"
with as_file(files().joinpath(f"pkg.{lib_suffix}")) as lib_path:
lib = ctypes.cdll.LoadLibrary(str(lib_path))
return lib.func
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
static value in namespace package
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from importlib.resources import read_text


def get_static_data():
return read_text("cmake_generated.nested1", "static_data").rstrip()
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""Try to open both generated and static files from various parts of the package."""
from importlib.resources import files, read_text

from .. import __version__

try:
from .. import nested2
except ImportError:
nested2 = None

def cmake_generated_static_data():
return read_text("cmake_generated", "static_data").rstrip()

def cmake_generated_nested_static_data():
return files("cmake_generated.nested1").joinpath("static_data").read_text().rstrip()

def get_configured_data():
return files("cmake_generated").joinpath("configured_file").read_text().rstrip()

def cmake_generated_namespace_generated_data():
return read_text("cmake_generated.namespace1","generated_data").rstrip()

nested_data = "success"

def nested2_check():
if nested2 is not None:
return "success"
Comment on lines +25 to +27
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not try ... import directly in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'm not sure whether there is reason to consider how the importer machinery might behave differently during import as during execution, so I wanted at least one case that really exercised the redirections of modules that depended on each other at import time. If you are confident that this shouldn't be an issue, we probably don't need nested2 at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure whether there is reason to consider how the importer machinery might behave differently during import as during execution

Not that I know of, other than resolving the circular dependency. But if you want for that to do the testing, could you also print the exception when it failed?

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
static value in subpackage 1
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
def nested1_generated_check():
# noinspection PyUnresolvedReferences
from ..nested1.generated import nested_data

return nested_data
1 change: 1 addition & 0 deletions tests/packages/cmake_generated/src/cmake_generated/pkg.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
extern "C" int func() {return 42;}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
static value in top-level package
Loading
Loading