Skip to content

python-cryptography: fix cross#226120

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

python-cryptography: fix cross#226120
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@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
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Apr 14, 2023
@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 14, 2023

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

@ofborg ofborg bot requested a review from SuperSandro2000 April 14, 2023 07:09
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Apr 14, 2023
Copy link
Copy Markdown
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
Copy Markdown
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.

, py
, pytz
, hypothesis
, buildPackages
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants