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

python-cryptography: fix cross #226120

Closed
wants to merge 1 commit into from
Closed

python-cryptography: fix cross #226120

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 14, 2023

This PR was produced by banging my head against the wall. Repeatedly.

Description of changes

Make pkgsCross.*.python-cryptography build.

  • setuptools-rust and rustPlatform.cargoSetupHook has been moved from nativeBuildInputs to buildInputs.

  • rustPlatform seems to interfere with splicing, so rustc and cargo are now top-level parameters in order to facilitate that.

  • Two environment variables need to be passed explicitly:

    • CARGO_BUILD_TARGET because otherwise the final link step attempts to use the hostPlatform linker

    • PYO3_CROSS_LIB_DIR because without it pyo3 throws a fit.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux (cross from x86_64-linux)
  • Fits CONTRIBUTING.md.

- setuptools-rust and rustPlatform.cargoSetupHook has been moved
  from nativeBuildInputs to buildInputs.

- rustPlatform seems to interfere with splicing, so rustc
  and cargo are now top-level parameters in order to facilitate that.

- Two environment variables need to be passed explicitly:

  - CARGO_BUILD_TARGET because otherwise the final link step
    attempts to use the hostPlatform linker

  - PYO3_CROSS_LIB_DIR because without it pyo3 throws a fit.
@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Apr 14, 2023
@ghost
Copy link
Author

ghost commented Apr 14, 2023

@ofborg build pkgsCross.aarch64-multiplatform.python3Packages.cryptography

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.

The prefix for python packages is python310Packages.cryptography or python310.pkgs.cryptography.

This definitely must go to staging.

Comment on lines +68 to +69
setuptools-rust
rustPlatform.cargoSetupHook
Copy link
Member

Choose a reason for hiding this comment

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

setuptools is consumed by the python that is building as far as I know. setup hooks also usually go to nativeBuildInputs. I assume the hooks themselves require some fixes to fix this properly.

@@ -21,6 +21,11 @@
, py
, pytz
, hypothesis
, buildPackages
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
, buildPackages

Comment on lines +62 to +63
cargo
rustc
Copy link
Member

Choose a reason for hiding this comment

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

I think we used rustPlaform to make sure all the rust things fit together. Can you describe the problem why you have split this?

Copy link
Author

@ghost ghost Apr 14, 2023

Choose a reason for hiding this comment

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

rustPlatform breaks splicing, which is why #212795 needs that CARGO_BUILD_TARGET hack. actually #212795 does the same thing.

We should stop using rustPlatform.cargo and rustPlatform.rustc. The splicing magic only seems to work for arguments that are passed directly rather than by reference through an attrset.

@Artturin can probably explain this better, he actually understands it while I just pretend to.

Copy link
Member

@Artturin Artturin Apr 14, 2023

Choose a reason for hiding this comment

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

It would be similar to #211340 but it's not the issue here because those attrs are spliced correctly pkgsCross.aarch64-multiplatform.__splicedPackages.rustPlatform.rust.rustc.__spliced (because they're not inside a derivation, read through the issue.)

nix-repl> pkgsCross.aarch64-multiplatform.python3Packages.cryptography.nativeBuildInputs
[ ... «derivation /nix/store/7zsx9k68akrnhbznhr2q63fcxv853ksv-cargo-setup-hook.sh.drv» «derivation /nix/store/31rhagc1nqf5d0wdjwvc0hbg3dg71xcl-python3.10-setuptools-rust-1.5.2.drv» «derivation /nix/store/1977acij72524vy0c8ij19ng7nzrf6yv-cargo-1.68.2.drv» «derivation /nix/store/b0lpygng67j4iww09wr9c45gl66vs84d-rustc-1.68.2.drv» ]

no aarch64-unknown-linux-gnu suffix so they should be for build

Copy link
Member

@Artturin Artturin Apr 14, 2023

Choose a reason for hiding this comment

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

diff --git a/pkgs/development/python-modules/cryptography/default.nix b/pkgs/development/python-modules/cryptography/default.nix
index ba2407923f3..105a5dd4dfc 100644
--- a/pkgs/development/python-modules/cryptography/default.nix
+++ b/pkgs/development/python-modules/cryptography/default.nix
@@ -77,6 +77,8 @@ buildPythonPackage rec {
     "--disable-pytest-warnings"
   ];
 
+  passthru = { inherit rustPlatform; };
+
   disabledTestPaths = lib.optionals (stdenv.isDarwin && stdenv.isAarch64) [
     # aarch64-darwin forbids W+X memory, but this tests depends on it:
     # * https://cffi.readthedocs.io/en/latest/using.html#callbacks
nix-repl> pkgsCross.aarch64-multiplatform.__splicedPackages.python3Packages.cryptography.rustPlatform.rust.rustc.__spliced
{ buildBuild = «derivation /nix/store/b0lpygng67j4iww09wr9c45gl66vs84d-rustc-1.68.2.drv»; buildHost = «derivation /nix/store/b0lpygng67j4iww09wr9c45gl66vs84d-rustc-1.68.2.drv»; buildTarget = «derivation /nix/store/b0lpygng67j4iww09wr9c45gl66vs84d-rustc-1.68.2.drv»; hostHost = «derivation /nix/store/g01vq22qlqgpcihqjvg2g79byg04gplv-rustc-1.68.2.drv»; hostTarget = «derivation /nix/store/g01vq22qlqgpcihqjvg2g79byg04gplv-rustc-1.68.2.drv»; }

Copy link
Author

@ghost ghost Apr 20, 2023

Choose a reason for hiding this comment

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

It would be similar to #211340 but it's not the issue here because those attrs are spliced correctly ... (because they're not inside a derivation, read through the issue.)

In other words, it is exactly the issue here, but "spliced correctly" has been defined in a way that is totally contrary to peoples' expectations about how this should work, just so this can be declared NOTABUG.

I think it is time to admit that the splicing magic was not a good idea. It misleads people into thinking they know what's going on when they don't.

Your passthru hack is the same kind of thing: sure, it works, but for reasons that 99% of the people reading the code will not understand. It is a footgun. Edit: sorry, I misinterpreted that diff as being a proposed solution rather than an investigatory tool.

Copy link
Member

@Artturin Artturin Apr 20, 2023

Choose a reason for hiding this comment

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

Your passthru hack is the same kind of thing: sure, it works, but for reasons that 99% of the people reading the code will not understand. It is a footgun.

It's not a hack, it's just for inspecting the attrset and showing that the package attrs have __spliced.

@SuperSandro2000
Copy link
Member

I think this will be fixed on a higher level and for all packages in #212795 and also for cryptography. I think the linked PR is the more correct solution for your problem, hence I am going to close this one. We would appreciate it, if you could test #212795 and give us feedback, if it fixes your problem.

@ghost ghost deleted the pr/fixcross/python-cryptography branch January 23, 2024 06:47
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.

2 participants