-
Notifications
You must be signed in to change notification settings - Fork 79
Test cases for different use cases of generated files #1193
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
base: main
Are you sure you want to change the base?
Changes from all commits
0426e9a
7bdec1e
1ce8a55
d280d76
979a38f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) | ||
| # 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}) | ||
| 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}" | ||
| ''' |
| 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? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| """ | ||
|
|
||
| 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() | ||
| ) | ||
LecrisUT marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For
inplacemode, it is run withcmake -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 supportinginplacesince 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.There was a problem hiding this comment.
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
inplaceandxfailit with an explanation (thatinplacemeans the build tree must look like the package layout)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build tree from
inplacewould 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My inclination is
It sounds like
inplaceis in flux and step 2. could be beyond the scope of this PR.There was a problem hiding this comment.
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.