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

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.

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 October 26, 2020 15:00
wheel
];

propagatedBuildInputs = [
Copy link
Member Author

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

Copy link
Member

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

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

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.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a 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.

Comment on lines +30 to +33
license = with lib.licenses; [
asl20
mit
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

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.

Copy link
Member

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.

@FRidh FRidh requested a review from bbigras October 26, 2020 15:08
- 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

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

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?

Copy link
Member Author

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.

Copy link
Member

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

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.

@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 November 6, 2020 04:05
@FRidh
Copy link
Member Author

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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants