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
WIP: python3.pkgs.maturin: PEP 517 build backend #101771
Conversation
The Python package python3.pkgs.maturin needs to know the paths to cargo and rustc.
With this module it is possible to use matura as a Python PEP 517 build backend.
wheel | ||
]; | ||
|
||
propagatedBuildInputs = [ |
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.
probably better not to propagate and let packages add their own versions of rustc and cargo to the derivation
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 also think it'd be clearer if user packages bring their own rust programs explicitly but not sure what's the norm there.
patches = [ | ||
./0001-Don-t-build-maturin-executable.patch | ||
./0002-declare-maturin-executable.patch | ||
./0003-install-python-module.patch |
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.
0003 along with a small change in 0001 could be upstreamed
@@ -25,6 +25,10 @@ in rustPlatform.buildRustPackage rec { | |||
# Requires network access, fails in sandbox. | |||
doCheck = false; | |||
|
|||
passthru = { |
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.
This would not be necessary if we would not propagate rustc/cargo.
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.
And two final new lines missing.
license = with lib.licenses; [ | ||
asl20 | ||
mit | ||
]; |
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.
license = with lib.licenses; [ | |
asl20 | |
mit | |
]; | |
license = with lib.licenses; [ asl20 mit ]; |
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 don't think that is an improvement. Separate lines for list items helps with merge conflicts. Granted, they're unlikely here.
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 don't think those licenses change anytime soon. Most meta blocks do this in one line.
- target = os.path.join(self.install_scripts, executable_name) | ||
- os.makedirs(self.install_scripts, exist_ok=True) | ||
- self.copy_file(source, target) | ||
- self.copy_tree( |
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.
this is now done with 0003 and packages = ...
mit | ||
]; | ||
homepage = "https://github.com/ijl/orjson"; | ||
broken = true; # Requires rust nightly |
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 don't see how that can work even if that hadn't required rust nightly -- have you tried on omething that builds with maturein and a common rust?
I mean it sounds like it'd require internet or vendoring cargo deps like we do in other rust packages; otherwise maturin would just let cargo try to pull its deps as it feels like?
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.
If cargo needs to fetch dependencies then indeed it won't work. We basically need parts of buildRustPackage
then.
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.
orjson's Cargo.toml:
[dependencies]
associative-cache = { version = "1" }
bytecount = { path = "./bytecount", default_features = false, features = ["generic-simd", "runtime-dispatch-simd"] }
encoding_rs = { path = "./encoding_rs", default_features = false, features = ["simd-accel"] }
inlinable_string = { version = "0.1" }
itoa = { version = "0.4", default_features = false }
once_cell = { version = "1", default_features = false }
pyo3 = { version = "0.12", default_features = false, features = ["extension-module"]}
ryu = { version = "1" }
serde = { version = "1", default_features = false }
serde_json = { path = "./json", default_features = false, features = ["std"] }
smallvec = { version = "1", default_features = false, features = ["const_generics", "union", "specialization", "write"] }
wyhash = { version = "0.4" }
I think it would, yes :)
Honestly they'd all reqiure at least pyo3 so it's unrealistic without; that's the reason I ended up using buildRustPackage instead of the python variant for ankirspy. I haven't looked at how to mix & match both, kind of lost motivation now I've seen this requires (and will keep requiring for the foreseeable future) nightly :/
wheel | ||
]; | ||
|
||
propagatedBuildInputs = [ |
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 also think it'd be clearer if user packages bring their own rust programs explicitly but not sure what's the norm there.
maturin support has been added to Nixpkgs |
Motivation for this change
#83614
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)