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

Stub file not getting included when using --manifest flag or building wheels #1517

Closed
bkolligs opened this issue Mar 5, 2023 Discussed in #1516 · 11 comments
Closed

Stub file not getting included when using --manifest flag or building wheels #1517

bkolligs opened this issue Mar 5, 2023 Discussed in #1516 · 11 comments

Comments

@bkolligs
Copy link

bkolligs commented Mar 5, 2023

Discussed in #1516

Originally posted by bkolligs March 4, 2023
Note that I originally opened this in discussion, but seems more like a bug than just a normal question.

I am trying to deploy a wheel of a Python package, and the stub file I have in the root of my package is not getting copied to the produced wheel when I use the --manifest flag with maturin develop. Here's an example:
I have the following directory structure for the package maturin_manifest:
image
With the following pyproject.toml:

[build-system]
requires = ["maturin>=0.14,<0.15"]
build-backend = "maturin"

[project]
name = "maturin-manifest"
requires-python = ">=3.7"
classifiers = [
    "Programming Language :: Rust",
    "Programming Language :: Python :: Implementation :: CPython",
    "Programming Language :: Python :: Implementation :: PyPy",
]


[tool.maturin]
features = ["pyo3/extension-module"]
manifest-path = "rust/Cargo.toml"

When I perform the command maturin develop -m rust/Cargo.toml the stub file is not detected:

🔗 Found pyo3 bindings
🐍 Found CPython 3.9 at ~/venvs/rust39/bin/python
📡 Using build options features from pyproject.toml
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
📦 Built wheel for CPython 3.9 to /tmp/.tmpTQaQI7/maturin_manifest-0.1.0-cp39-cp39-linux_x86_64.whl
🛠 Installed maturin-manifest-0.1.0

But when I perform maturin develop inside the rust directory it does work.
Additionally, when I try and simulate building this package from another, say as a dependency in setup.cfg with pip install ., the same thing occurs, and the stub file does not exist in the library directory.

$ ls venvs/rust39/lib/python3.9/site-packages/maturin_manifest
__init__.py
maturin_manifest.cpython-39-x86_64-linux-gnu.so
__pycache__
@messense messense added the bug Something isn't working label Mar 5, 2023
@bkolligs
Copy link
Author

bkolligs commented Mar 5, 2023

I think the issue is related to this line:

rust_module: project_root.to_path_buf(),

Unless I’m terribly mistaken when the rust_module defaults to project_root (which I assume is the place you call maturin from), it doesn’t realize the stub file could be inside the directory containing Cargo.toml

@bkolligs
Copy link
Author

bkolligs commented Mar 5, 2023

Ah yeah, we default to looking for pyproject.toml to determine the project_root:

let project_root = if pyproject_file.is_file() {
pyproject_file.parent().unwrap_or(manifest_dir)
} else {
manifest_dir
};

@bkolligs
Copy link
Author

bkolligs commented Mar 5, 2023

Which is then used here:

module = PathBuf::from(module_name);
writer.add_directory(&module)?;
let type_stub = project_layout
.rust_module
.join(format!("{module_name}.pyi"));
if type_stub.exists() {
eprintln!("📖 Found type stub file at {module_name}.pyi");
writer.add_file(&module.join("__init__.pyi"), type_stub)?;
writer.add_bytes(&module.join("py.typed"), b"")?;
}
};

@bkolligs
Copy link
Author

bkolligs commented Mar 5, 2023

So ideally we would pass information about the active manifest dir in the ProjectLayout structure that we could use to check if the stub file is in there. Do you have any thoughts about that @messense ? We could call this rust_dir so it would be the analog to python_dir for the rust module

pub python_dir: PathBuf,

this seems like a minor change since ProjectLayout is not re-exported here

maturin/src/lib.rs

Lines 56 to 61 in 6a695e2

#[cfg(feature = "scaffolding")]
mod new_project;
mod project_layout;
pub mod pyproject_toml;
mod python_interpreter;
mod source_distribution;
as externally public?

This would effectively infer the rust-source directory as the manifest directory, as an analog to the pyproject.toml argument tool.maturin.python-source.

@messense
Copy link
Member

messense commented Mar 6, 2023

You could add cargo_toml_path instead of rust-source since Cargo only needs to work with Cargo.toml not its directory.

Pull requests are welcome!

@bkolligs
Copy link
Author

bkolligs commented Mar 6, 2023

You could add cargo_toml_path instead of rust-source since Cargo only needs to work with Cargo.toml not its directory.

Pull requests are welcome!

That’s a good idea! I’ll send you a PR over the next couple of days.

@messense
Copy link
Member

messense commented Mar 6, 2023

it doesn’t realize the stub file could be inside the directory containing Cargo.toml

On second thought, you should move stub file next to pyproject.toml instead because that's the Python project root, supporting multiple places for stub file in pure Rust project layout adds complexity and There should be one-- and preferably only one --obvious way to do it..

But when I perform maturin develop inside the rust directory it does work.

And this shouldn't work.

@bkolligs
Copy link
Author

bkolligs commented Mar 6, 2023

On second thought, you should move stub file next to pyproject.toml instead because that's the Python project root,

Let me try this, I was originally having trouble getting that to work as well

@bkolligs
Copy link
Author

bkolligs commented Mar 6, 2023

Here are some experiments that I have conducted. Same project directory as above.

  1. maturin develop inside rust will install the wheel to the current virtual environment with the stub file correctly. I would expect this since we don't need to have the pyproject.toml in the same directory, which is desired in my case.
  2. maturin develop in the project root will install the wheel but without the stub file, as we've been discussing above.
  3. Moving the stub file to be alongside the pyproject.toml file and then calling maturin develop works to place the stub file in the site-packages directory.
  4. After moving the stub file to the root, installing the rust bindings pointed to by the pyproject.toml with pip install . in the root includes the stub file as well.
  5. Building wheel with stub file in the root includes the stub file in the wheel.

Seems like this works as intended.

My overarching goal with this Issue was to write a "rust first" crate with optional Python bindings. What I mean by that is that the root crate is meant to be used by arbitrary Rust crates, and then the bindings can be installed optionally when one wants to use the crate from Python. The convenience that this enables is installing either a wheel or Rust crate from the github link as follows:

Rust dependency

In another crate's Cargo.toml

[dependencies]
maturin_manifest = { git = ssh://[email protected]/user/maturin_manifest.git }

Python dependency

In a Python package that wants to use this repository and build the bindings using maturin

[options.extras_require]
all =
    maturin_manifest@git+ssh://[email protected]/user/maturin_manifest.git

With just the pyproject.toml at the root, this use case is quite smooth. Putting the stub file in the root directory seems like a fine workaround, although it would be nice to separate the rust from the python a bit more so that the intent of "rust first with optional bindings" comes across clearly.

@messense
Copy link
Member

messense commented Mar 6, 2023

My overarching goal with this Issue was to write a "rust first" crate with optional Python bindings. What I mean by that is that the root crate is meant to be used by arbitrary Rust crates, and then the bindings can be installed optionally when one wants to use the crate from Python.

Well, I mean you can move pyproject.toml next to your Python Rust binding's Cargo.toml (and remove manifest-path from pyproject.toml) right?

[options.extras_require]
all =
    maturin_manifest@git+ssh://[email protected]/user/maturin_manifest.git#subdirectory=rust

@bkolligs
Copy link
Author

bkolligs commented Mar 6, 2023

Oh nice, I didn't actually know that subdirectory option existed. I think that's exactly what I needed! We can close this issue.

@bkolligs bkolligs closed this as completed Mar 6, 2023
@messense messense removed the bug Something isn't working label Mar 6, 2023
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

No branches or pull requests

2 participants