-
Notifications
You must be signed in to change notification settings - Fork 109
Add installation of data-files build artifacts into Python package #574
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
Changes from 1 commit
a12e0c6
13807a7
afc6653
991689e
d0add2d
b7b0c7a
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,13 @@ | ||
| [package] | ||
| name = "data-files" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
| build = "build.rs" | ||
|
|
||
| [dependencies] | ||
| pyo3 = "0.27" | ||
|
|
||
| [lib] | ||
| name = "_lib" # private module to be nested into Python package | ||
| crate-type = ["cdylib"] | ||
| path = "rust/lib.rs" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| include build.rs | ||
| include Cargo.toml | ||
| include Cargo.lock | ||
| graft rust | ||
| graft tests | ||
|
|
||
| # Data files generated by Rust build scripts generally do not need to be listed in the manifest | ||
| # bceause they are not generated as part of the `sdist`; the Rust extension is also not bulit in an | ||
| # sdist, so neither are the generated files. You should ensure that all source files needed to | ||
| # generate the build files _are_ included in the manifest, however. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| use std::fs; | ||
| use std::io::{self, Write}; | ||
| use std::path::Path; | ||
|
|
||
| fn main() -> io::Result<()> { | ||
| println!("cargo::rerun-if-changed=build.rs"); | ||
| let out_dir_raw = std::env::var("OUT_DIR").unwrap(); | ||
| let out_dir = Path::new(&out_dir_raw); | ||
| fs::File::create(out_dir.join("my_file.txt"))?.write_all(b"Generated by a build script.\n")?; | ||
|
|
||
| let sub_dir = out_dir.join("dir"); | ||
| fs::create_dir_all(&sub_dir)?; | ||
| fs::File::create(sub_dir.join("a.txt"))?.write_all(b"This is file A.\n")?; | ||
| fs::File::create(sub_dir.join("b.txt"))?.write_all(b"This is file B.\n")?; | ||
| Ok(()) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| from pathlib import Path | ||
|
|
||
| import nox | ||
|
|
||
| SETUPTOOLS_RUST = Path(__file__).parents[2] | ||
|
|
||
|
|
||
| @nox.session() | ||
| def test(session: nox.Session): | ||
| session.install(str(SETUPTOOLS_RUST), "setuptools", "pytest") | ||
| session.install("--no-build-isolation", ".") | ||
| session.run("pytest", "tests", *session.posargs) | ||
|
|
||
|
|
||
| @nox.session() | ||
| def test_inplace(session: nox.Session): | ||
| session.install(str(SETUPTOOLS_RUST), "setuptools", "pytest") | ||
| session.install("--no-build-isolation", "--editable", ".") | ||
| try: | ||
| session.run("pytest", "tests", *session.posargs) | ||
| finally: | ||
| # Clear out any data files that _did_ exist | ||
| session.run( | ||
| "python", | ||
| "-c", | ||
| """ | ||
| import os | ||
| import shutil | ||
| from pathlib import Path | ||
| import data_files | ||
|
|
||
| try: | ||
| os.remove(Path(data_files.__file__).parent / "my_file.txt") | ||
| except FileNotFoundError: | ||
| pass | ||
| shutil.rmtree(Path(data_files.__file__).parent / "_data", ignore_errors=True)""", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| [build-system] | ||
| requires = ["setuptools", "setuptools-rust"] | ||
| build-backend = "setuptools.build_meta" | ||
|
|
||
| [project] | ||
| name = "data-files" | ||
| version = "1.0" | ||
|
|
||
| [tool.setuptools.packages] | ||
| # Pure Python packages/modules | ||
| find = { where = ["python"] } | ||
|
|
||
| [[tool.setuptools-rust.ext-modules]] | ||
| target = "data_files._lib" | ||
| [tool.setuptools-rust.ext-modules.data-files] | ||
| # Keys correspond to files/directories in the Rust extension's build directory `OUT_DIR`. | ||
| # Values are Python packages that the corresponding file or directory should be placed inside. | ||
| "my_file.txt" = "data_files" | ||
| "dir" = "data_files._data" | ||
|
Member
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. I wonder if the value should be the final filename inside the directory? e.g.
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. I agree it's a bit weird as-is. I'd prototyped it this way because "find the installation directory of this Python package" is a built-in function to the
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. I've left this as-is in the latest commit for now but I'm not married to it. With Just for comparison purposes too:
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| __all__ = ["library_ok", "data_files_content"] | ||
|
|
||
| from pathlib import Path | ||
| from ._lib import library_ok | ||
|
|
||
|
|
||
| def data_files_content() -> dict[Path, str]: | ||
| us = Path(__file__).parent | ||
| paths = [us / "my_file.txt"] | ||
| paths.extend((us / "_data" / "dir").glob("*.txt")) | ||
| return {path.relative_to(us): path.read_text() for path in paths} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| #[pyo3::pymodule] | ||
| mod _lib { | ||
| use pyo3::prelude::*; | ||
|
|
||
| #[pyfunction] | ||
| fn library_ok() -> bool { | ||
| true | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| from pathlib import Path | ||
| import data_files | ||
|
|
||
|
|
||
| def test_rust_extension(): | ||
| assert data_files.library_ok() | ||
|
|
||
|
|
||
| def test_data_file_content(): | ||
| expected = { | ||
| Path("my_file.txt"): "Generated by a build script.\n", | ||
| Path("_data") / "dir" / "a.txt": "This is file A.\n", | ||
| Path("_data") / "dir" / "b.txt": "This is file B.\n", | ||
| } | ||
| assert data_files.data_files_content() == expected |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,14 @@ | ||
| from .build import build_rust | ||
| from .clean import clean_rust | ||
| from .extension import Binding, RustBin, RustExtension, Strip | ||
| from .extension import Binding, RustBin, RustExtension, Strip, UniversalDataSource | ||
| from .version import version as __version__ # noqa: F401 | ||
|
|
||
| __all__ = ("Binding", "RustBin", "RustExtension", "Strip", "build_rust", "clean_rust") | ||
| __all__ = ( | ||
| "Binding", | ||
| "RustBin", | ||
| "RustExtension", | ||
| "Strip", | ||
| "UniversalDataSource", | ||
| "build_rust", | ||
| "clean_rust", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| from setuptools import Distribution | ||
| from setuptools.command.build_ext import build_ext as CommandBuildExt | ||
| from setuptools.command.build_ext import get_abi3_suffix | ||
| from setuptools.command.build_py import build_py as setuptools_build_py | ||
| from setuptools.command.install_scripts import install_scripts as CommandInstallScripts | ||
|
|
||
| from ._utils import check_subprocess_output, format_called_process_error, Env | ||
|
|
@@ -124,10 +125,12 @@ def run_for_extension(self, ext: RustExtension) -> None: | |
| assert self.plat_name is not None | ||
| if self.target is _Platform.CARGO_DEFAULT: | ||
| self.target = _override_cargo_default_target(self.plat_name, ext.env) | ||
| dylib_paths = self.build_extension(ext) | ||
| self.install_extension(ext, dylib_paths) | ||
| dylib_paths, artifact_dirs = self.build_extension(ext) | ||
| self.install_extension(ext, dylib_paths, artifact_dirs) | ||
|
|
||
| def build_extension(self, ext: RustExtension) -> List["_BuiltModule"]: | ||
| def build_extension( | ||
| self, ext: RustExtension | ||
| ) -> Tuple[List["_BuiltModule"], List[Path]]: | ||
| env = _prepare_build_environment(ext.env, ext) | ||
|
|
||
| if not os.path.exists(ext.path): | ||
|
|
@@ -204,10 +207,14 @@ def build_extension(self, ext: RustExtension) -> List["_BuiltModule"]: | |
| else: | ||
| targets = [self.target] | ||
|
|
||
| cargo_messages = "" | ||
| cargo_messages: Dict[str, List[str]] = {} | ||
| for target in targets: | ||
| target_command = command.copy() | ||
| if target is not None: | ||
| if target is None: | ||
| # Normalize the entries in `cargo_messages` to always be in terms of the | ||
| # actual target triple. | ||
| target = get_rust_host(ext.env) | ||
| else: | ||
| target_command += ["--target", target] | ||
| if rustc_args: | ||
| target_command += ["--"] | ||
|
|
@@ -221,12 +228,12 @@ def build_extension(self, ext: RustExtension) -> List["_BuiltModule"]: | |
| # If quiet, capture all output and only show it in the exception | ||
| # If not quiet, forward all cargo output to stderr | ||
| stderr = subprocess.PIPE if quiet else None | ||
| cargo_messages += check_subprocess_output( | ||
| cargo_messages[target] = check_subprocess_output( | ||
| target_command, | ||
| env=env, | ||
| stderr=stderr, | ||
| text=True, | ||
| ) | ||
| ).splitlines() | ||
| except subprocess.CalledProcessError as e: | ||
| # Don't include stdout in the formatted error as it is a huge dump | ||
| # of cargo json lines which aren't helpful for the end user. | ||
|
|
@@ -246,7 +253,7 @@ def build_extension(self, ext: RustExtension) -> List["_BuiltModule"]: | |
| if ext._uses_exec_binding(): | ||
| # Find artifact from cargo messages | ||
| artifacts = _find_cargo_artifacts( | ||
| cargo_messages.splitlines(), | ||
| [line for messages in cargo_messages.values() for line in messages], | ||
| package_id=package_id, | ||
| kinds={"bin"}, | ||
| ) | ||
|
|
@@ -276,7 +283,7 @@ def build_extension(self, ext: RustExtension) -> List["_BuiltModule"]: | |
| else: | ||
| # Find artifact from cargo messages | ||
| artifacts = _find_cargo_artifacts( | ||
| cargo_messages.splitlines(), | ||
| [line for messages in cargo_messages.values() for line in messages], | ||
| package_id=package_id, | ||
| kinds={"cdylib", "dylib"}, | ||
| ) | ||
|
|
@@ -300,10 +307,33 @@ def build_extension(self, ext: RustExtension) -> List["_BuiltModule"]: | |
|
|
||
| # guaranteed to be just one element after checks above | ||
| dylib_paths.append(_BuiltModule(ext.name, artifact_path)) | ||
| return dylib_paths | ||
|
|
||
| if not ext.data_files: | ||
| return dylib_paths, [] | ||
|
|
||
| if self.target is _Platform.UNIVERSAL2: | ||
| search_targets = ext.universal2_data_files_from.targets() | ||
| else: | ||
| search_targets = list(cargo_messages) | ||
| out_dirs = [ | ||
| out_dir | ||
| for target in search_targets | ||
| if (out_dir := _find_cargo_out_dir(cargo_messages[target], package_id)) | ||
| is not None | ||
| ] | ||
| if not out_dirs: | ||
| raise ExecError( | ||
| f"extension {ext.name} requests data files, but no corresponding" | ||
| " build-script out directories could be found" | ||
| ) | ||
|
|
||
| return dylib_paths, out_dirs | ||
|
|
||
| def install_extension( | ||
| self, ext: RustExtension, dylib_paths: List["_BuiltModule"] | ||
| self, | ||
| ext: RustExtension, | ||
| dylib_paths: List["_BuiltModule"], | ||
| build_artifact_dirs: List[Path], | ||
| ) -> None: | ||
| debug_build = self._is_debug_build(ext) | ||
|
|
||
|
|
@@ -400,6 +430,39 @@ def install_extension( | |
| mode |= (mode & 0o444) >> 2 # copy R bits to X | ||
| os.chmod(ext_path, mode) | ||
|
|
||
| if not ext.data_files: | ||
| return | ||
|
|
||
| # We'll delegate the finding of the package directories to Setuptools, so we | ||
| # can be sure we're handling editable installs and other complex situations | ||
| # correctly. | ||
| build_py = cast(setuptools_build_py, self.get_finalized_command("build_py")) | ||
|
|
||
| def get_package_dir(package: str) -> Path: | ||
| if self.inplace: | ||
| # If `inplace`, we have to ask `build_py` (like `build_ext` would). | ||
| return Path(build_py.get_package_dir(package)) | ||
| # ... If not, `build_ext` knows where to put the package. | ||
| return Path(build_ext.build_lib) / Path(*package.split(".")) | ||
|
|
||
| for source, package in ext.data_files.items(): | ||
| dest = get_package_dir(package) | ||
| dest.mkdir(mode=0o755, parents=True, exist_ok=True) | ||
| for artifact_dir in build_artifact_dirs: | ||
| source_full = artifact_dir / source | ||
| dest_full = dest / source_full.name | ||
| if source_full.is_file(): | ||
| logger.info( | ||
| "Copying data file from %s to %s", source_full, dest_full | ||
| ) | ||
| shutil.copy2(source_full, dest_full) | ||
| elif source_full.is_dir(): | ||
| logger.info( | ||
| "Copying data directory from %s to %s", source_full, dest_full | ||
| ) | ||
| shutil.copytree(source_full, dest_full, dirs_exist_ok=True) | ||
| # This tacitly makes "no match" a silent non-error. | ||
|
Member
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. I would probably prefer no match to be a build failure? IMO this implies the configuration is wrong. What use case is there where silently missing files is desirable?
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. I can't remember my reasoning, but honestly it might just have been that I wrote the loop like this, then realised the implication and put it in a comment rather than actually following the thought all the way through to see what should happen. It's even easier to error on missing when there's only one artifact directory, so I did that in b7b0c7a. |
||
|
|
||
| def get_dylib_ext_path(self, ext: RustExtension, target_fname: str) -> str: | ||
| assert self.plat_name is not None | ||
| build_ext = cast(CommandBuildExt, self.get_finalized_command("build_ext")) | ||
|
|
@@ -835,6 +898,20 @@ def _find_cargo_artifacts( | |
| return artifacts | ||
|
|
||
|
|
||
| def _find_cargo_out_dir(cargo_messages: List[str], package_id: str) -> Optional[Path]: | ||
| # Chances are that the line we're looking for will be the third laste line in the | ||
| # messages. The last is the completion report, the penultimate is generally the | ||
|
jakelishman marked this conversation as resolved.
Outdated
|
||
| # build of the final artifact. | ||
| for messsage in reversed(cargo_messages): | ||
| if "build-script-executed" not in messsage or package_id not in messsage: | ||
| continue | ||
| parsed = json.loads(messsage) | ||
| if parsed.get("package_id") == package_id: | ||
| out_dir = parsed.get("out_dir") | ||
| return None if out_dir is None else Path(out_dir) | ||
| return None | ||
|
|
||
|
|
||
| def _replace_cross_target_dir(path: str, ext: RustExtension, *, quiet: bool) -> str: | ||
| """Replaces target director from `cross` docker build with the correct | ||
| local path. | ||
|
|
||
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.
Possible sticking point here -
ext-modulesis a list, butext-modules.data-filesis a table? How does this interact if there are multipleext-modules?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.
I'm relatively sure that the
data-filestable attaches to the most recent entry in theext-moduleslist - I think this is just the single-item case of it.I actually wanted to write this example as
but using an inline table with linebreaks inside it only arrived in TOML 1.1 late last year, and the Python
tomllibdoesn't handle it yet.Uh oh!
There was an error while loading. Please reload this page.
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.
Sorry, to finish the thought - so the options you can write in TOML 1.0 today are
or
and these parse to the same thing:
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.
The two ways of writing the multi-table I gave above both work fine in Python and produce the expected list output. It's (at the end of the day) just regular TOML syntax, though admittedly one that I wasn't 100% confident on off the top of my head.