PyO3 dependency as a feature#240
Merged
Merged
Conversation
gbin
approved these changes
Feb 10, 2025
Collaborator
|
Thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Python as an optional dep
PyO3, useful as it is, can take up time during compilation in both the copper repo and for users. It'd be best to define it as a
--featurefor those who want to opt in to python bindings instead of requiring it.In my project, im using cross to handle cross compilation and I needed to do some minor configuration to get it working
Right now, python is defaulted as disabled. We can flip that if we want it on by default.
I've also kept it as disabled on macOS. We could leave it platform-agnostic and just not enable it on CI for macOS. I'd prefer that approach but went with the current state of things.
Looking at the docs https://pyo3.rs/v0.4.1/overview#using-rust-from-python, we need to do some shared lib renaming, and ditto for windows. Apparently that can be handled during python packaging but I don't think it's really worth the effort at the moment.
Python module naming
As I was poking around into the python setup, i noticed this https://github.com/copper-project/copper-rs/blob/master/core/cu29_export/build.rs#L6
f we're alright with python interfaces importing the module as
we'd be able to use python interfaces without violating the
OUT_DIRsandbox https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script. We could also rename the pymodule tolibcu29_types,libcu29,libcu29_readerbut i think that'd just be more confusing.Ideally, we'd be able to just symlink the output or handle renaming in cargo, but that isn't supported rust-lang/cargo#1970
I tested this out locally with
examples/cu_standalone_structlog/readlog.pyand got it to the point where it tried to read a log but I didn't have one so it raised an exception. I figured that's better than hitting a pybind exception.