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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions pkgs/development/python-modules/cryptography/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

, rust
, cargo
, rustc
, python3
}:

let
Expand All @@ -46,15 +51,26 @@ buildPythonPackage rec {

cargoRoot = "src/rust";

env = {
PYO3_CROSS_LIB_DIR = "${python3}/lib/";
CARGO_BUILD_TARGET = "${rust.toRustTargetSpec stdenv.hostPlatform}";
};

nativeBuildInputs = lib.optionals (!isPyPy) [
cffi
] ++ [
rustPlatform.cargoSetupHook
setuptools-rust
] ++ (with rustPlatform; [ rust.cargo rust.rustc ]);
cargo
rustc
Comment on lines +62 to +63
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 •
edited by ghost
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited by ghost
Loading

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 •
edited
Loading

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.

];

buildInputs = [ openssl ]
++ lib.optionals stdenv.isDarwin [ Security libiconv ];
buildInputs = [
openssl
setuptools-rust
rustPlatform.cargoSetupHook
Comment on lines +68 to +69
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.

] ++ lib.optionals stdenv.isDarwin [
Security
libiconv
];

propagatedBuildInputs = lib.optionals (!isPyPy) [
cffi
Expand Down