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

WIP: python3.pkgs.maturin: PEP 517 build backend #101771

Closed
wants to merge 3 commits into from
Closed

Conversation

@FRidh
Copy link
Member

@FRidh FRidh commented Oct 26, 2020

Motivation for this change

#83614

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
FRidh added 3 commits Oct 26, 2020
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.
@FRidh FRidh requested a review from jonringer as a code owner Oct 26, 2020
wheel
];

propagatedBuildInputs = [
Copy link
Member Author

@FRidh FRidh Oct 26, 2020

probably better not to propagate and let packages add their own versions of rustc and cargo to the derivation

Copy link
Contributor

@martinetd martinetd Oct 26, 2020

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
Copy link
Member Author

@FRidh FRidh Oct 26, 2020

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 = {
Copy link
Member Author

@FRidh FRidh Oct 26, 2020

This would not be necessary if we would not propagate rustc/cargo.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

And two final new lines missing.

license = with lib.licenses; [
asl20
mit
];
Copy link
Member

@SuperSandro2000 SuperSandro2000 Oct 26, 2020

Suggested change
license = with lib.licenses; [
asl20
mit
];
license = with lib.licenses; [ asl20 mit ];

Copy link
Member Author

@FRidh FRidh Oct 26, 2020

I don't think that is an improvement. Separate lines for list items helps with merge conflicts. Granted, they're unlikely here.

Copy link
Member

@SuperSandro2000 SuperSandro2000 Oct 26, 2020

I don't think those licenses change anytime soon. Most meta blocks do this in one line.

@FRidh FRidh requested a review from bbigras Oct 26, 2020
- 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(
Copy link
Member Author

@FRidh FRidh Oct 26, 2020

this is now done with 0003 and packages = ...

mit
];
homepage = "https://github.com/ijl/orjson";
broken = true; # Requires rust nightly
Copy link
Contributor

@martinetd martinetd Oct 26, 2020

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?

Copy link
Member Author

@FRidh FRidh Oct 26, 2020

If cargo needs to fetch dependencies then indeed it won't work. We basically need parts of buildRustPackage then.

Copy link
Contributor

@martinetd martinetd Oct 26, 2020

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 = [
Copy link
Contributor

@martinetd martinetd Oct 26, 2020

I also think it'd be clearer if user packages bring their own rust programs explicitly but not sure what's the norm there.

@FRidh FRidh changed the title python3.pkgs.maturin: PEP 517 build backend WIP: python3.pkgs.maturin: PEP 517 build backend Nov 2, 2020
@ryantm ryantm marked this pull request as draft Nov 6, 2020
@FRidh
Copy link
Member Author

@FRidh FRidh commented Mar 6, 2021

maturin support has been added to Nixpkgs

@FRidh FRidh closed this Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants