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

setuptools-rust-hook: Use correct python for cross #262123

Closed
wants to merge 12 commits into from

Conversation

kjeremy
Copy link
Contributor

@kjeremy kjeremy commented Oct 19, 2023

Description of changes

Sets PYO3_PYTHON according to https://pyo3.rs/v0.20.0/building_and_distribution#cross-compiling

Setting PYO3_INCLUDE_DIR doesn't seem to be necessary for fixing #262073 but I set it for good measure.

Fixes #262073

@Artturin
@Cynerd

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

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.

not to fit on cross things but looks logical to me

@Artturin
Copy link
Member

Has to target staging

@kjeremy kjeremy changed the base branch from master to staging October 20, 2023 02:45
@kjeremy
Copy link
Contributor Author

kjeremy commented Oct 20, 2023

Has to target staging

I changed the target to staging.

@kjeremy
Copy link
Contributor Author

kjeremy commented Oct 20, 2023

The ofborg-eval failure doesn't look related to this PR.

@Artturin
Copy link
Member

@ofborg eval

1 similar comment
@kjeremy
Copy link
Contributor Author

kjeremy commented Oct 20, 2023

@ofborg eval

@Artturin
Copy link
Member

Artturin commented Oct 20, 2023

Gotta force-push

Do git commit --amend and git push --force

@ghost
Copy link

ghost commented Oct 23, 2023

I cherry-picked this back to master but it's still trying to run hostPlatform binaries:

declare -x PYO3_CROSS_INCLUDE_DIR="/nix/store/pzlcmpd2fjc401cjsfx3b424mdl5w86b-python3-armv7l-unknown-linux-gnueabihf-3.11.5/include/python3.11"
declare -x PYO3_CROSS_LIB_DIR="/nix/store/pzlcmpd2fjc401cjsfx3b424mdl5w86b-python3-armv7l-unknown-linux-gnueabihf-3.11.5/lib/python3.11"
declare -x PYO3_PYTHON="/nix/store/pzlcmpd2fjc401cjsfx3b424mdl5w86b-python3-armv7l-unknown-linux-gnueabihf-3.11.5/bin/python3.11"

Is there something in staging but not yet in master that makes this PR work?

Also, note that setuptoolsRustBuildHook.__spliced doesn't exist. Something is really borked here.

@Artturin
Copy link
Member

Also, note that setuptoolsRustBuildHook.__spliced doesn't exist. Something is really borked here.

It's this #228139
The hooks aren't spliced withing the sets callPackage but they will be spliced outside of it

pkgsCross.aarch64-multiplatform.__splicedPackages.python3Packages.setuptoolsRustBuildHook.__spliced is spliced

nix-repl> pkgsCross.aarch64-multiplatform.__splicedPackages.python3.pkgs.setuptoolsRustBuildHook.__spliced
error: attribute '__spliced' missing

is the fault of #211340

@Artturin
Copy link
Member

Artturin commented Oct 23, 2023

declare -x PYO3_PYTHON="/nix/store/pzlcmpd2fjc401cjsfx3b424mdl5w86b-python3-armv7l-unknown-linux-gnueabihf-3.11.5/bin/python3.11" should be the build python I think

nix-repl> p = lib.elemAt pkgsCross.aarch64-multiplatform.__splicedPackages.python3.pkgs.cryptography.nativeBuildInputs 10
nix-repl> p.stdenv.hostPlatform.system
"aarch64-linux"

@kjeremy do you have binfmt enabled?

python.pythonForBuild.pkgs.setuptoolsRustBuildHook would make hostPlatform correct, but then the vars aren't for the target

declare -x PYO3_CROSS_INCLUDE_DIR="/nix/store/ffll6glz3gwx342z0ch8wx30p5cnqz1z-python3-3.11.5/include/python3.11"
declare -x PYO3_CROSS_LIB_DIR="/nix/store/ffll6glz3gwx342z0ch8wx30p5cnqz1z-python3-3.11.5/lib/python3.11"
declare -x PYO3_PYTHON="/nix/store/ffll6glz3gwx342z0ch8wx30p5cnqz1z-python3-3.11.5/bin/python3.11"

@Artturin
Copy link
Member

Artturin commented Oct 23, 2023

Could work around it by doing pyBin = "python${python.pythonVersion}";
but then
#error "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."
for some reason, even though

declare -x PYO3_CROSS_INCLUDE_DIR="/nix/store/h4ja6mc0f8r6qnyil5vi0mjwcv4jb8y2-python3-armv7l-unknown-linux-gnueabihf-3.11.5/include/python3.11"
declare -x PYO3_CROSS_LIB_DIR="/nix/store/h4ja6mc0f8r6qnyil5vi0mjwcv4jb8y2-python3-armv7l-unknown-linux-gnueabihf-3.11.5/lib/python3.11"
declare -x PYO3_PYTHON="python3.11"

The hook for the wrong host is dragging in the python to nativeBuildInputs via propagation but that shouldn't be the issue.

Currently building on staging with no binfmt to see if there's something missing on master

@ghost
Copy link

ghost commented Oct 23, 2023

Could work around it by doing pyBin = "python${python.pythonVersion}";

Eeeew.

but then
#error "LONG_BIT definition appears wrong for platform (bad gcc/glibc config?)."
for some reason, even though

Can we just move all the python hooks out of hooks and into pythonPackages so they get spliced? Otherwise we're going to keep having problems like this :(

Also I think the with statement at the top of hooks/default.nix is a huge footgun.

@kjeremy
Copy link
Contributor Author

kjeremy commented Oct 23, 2023

declare -x PYO3_PYTHON="/nix/store/pzlcmpd2fjc401cjsfx3b424mdl5w86b-python3-armv7l-unknown-linux-gnueabihf-3.11.5/bin/python3.11" should be the build python I think

nix-repl> p = lib.elemAt pkgsCross.aarch64-multiplatform.__splicedPackages.python3.pkgs.cryptography.nativeBuildInputs 10
nix-repl> p.stdenv.hostPlatform.system
"aarch64-linux"

@kjeremy do you have binfmt enabled?

python.pythonForBuild.pkgs.setuptoolsRustBuildHook would make hostPlatform correct, but then the vars aren't for the target

declare -x PYO3_CROSS_INCLUDE_DIR="/nix/store/ffll6glz3gwx342z0ch8wx30p5cnqz1z-python3-3.11.5/include/python3.11"
declare -x PYO3_CROSS_LIB_DIR="/nix/store/ffll6glz3gwx342z0ch8wx30p5cnqz1z-python3-3.11.5/lib/python3.11"
declare -x PYO3_PYTHON="/nix/store/ffll6glz3gwx342z0ch8wx30p5cnqz1z-python3-3.11.5/bin/python3.11"

@Artturin
Yes I have binfmt enabled.

@kjeremy
Copy link
Contributor Author

kjeremy commented Oct 25, 2023

Not sure what the right fix is here.

@ghost
Copy link

ghost commented Oct 25, 2023

Not sure what the right fix is here.

We probably need to deal with #263019 first.

Or at least implement some kind of warning if unspliced dependencies get used. Otherwise it's extremely difficult to understand what's actually going on. When unspliced dependencies get into the mix, mkDerivation will just randomly use packages from the wrong platform (e.g. you might put something in nativeBuildInputs, but mkDerivation will use the version of that dependency for the hostPlatform). Until we fix this, sorting out what's really going on is like stumbling around in the dark.

@ghost
Copy link

ghost commented Oct 25, 2023

Rebase on

and then run

nix-instantiate . -A pkgsCross.armv7l-hf-multiplatform.python3Packages.cryptography

you will get

trace: warning: derivation ncurses:
                     unspliced build-host dependency ncurses
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

trace: warning: derivation python3:
                     unspliced build-host dependency python3
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

trace: warning: derivation !!no pname!!:
                     unspliced build-host dependency cargo-vendor-normalise
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

trace: warning: derivation !!no pname!!:
                     unspliced build-host dependency armv7l-unknown-linux-gnueabihf-cargo-1.72.1
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

trace: warning: derivation python3:
                     unspliced build-host dependency python3
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

trace: warning: derivation cryptography:
                     unspliced build-host dependency setuptools-rust-setup-hook
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

trace: warning: derivation python3:
                     unspliced build-host dependency python3
                       build=x86_64-unknown-linux-gnu
                       target=armv7l-unknown-linux-gnueabihf

So, those are the bugs we need to fix.

Adam Joseph and others added 11 commits November 17, 2023 03:01
In order to avoid setting off the unspliced-dependency check, we
need to make sure that the targetPackages.stdenv.cc.bintools used by
gcc gets spliced.  In order for it to be spliced, it needs a
top-level entry in all-packages.nix.  Therefore, this commit adds one.

This also causes us to stop abusing targetPackages to access the
linker, which is a good thing.
This commit adds a `lib.warn` check to stdenv.mkDerivation, causing
it to emit a warning if a dependency which ought to be spliced in
fact is not.
Co-authored-by: Artturi <Artturin@artturin.com>
@SuperSandro2000
Copy link
Member

@kjeremy can you do the fix noticed in the comment before this one?

@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 18, 2023

@kjeremy can you do the fix noticed in the comment before this one?

Do you mean rebase on #263082? If so then yes I can do that. Note that my solution to this isn't correct because I had binfmt enabled.

@kjeremy kjeremy requested a review from a user December 18, 2023 20:25
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Dec 18, 2023
@kjeremy
Copy link
Contributor Author

kjeremy commented Dec 18, 2023

@SuperSandro2000 I've rebased and did a force push

!(list-of-shame ? ${approximate-pname})
)

# Hi there! If you're reading this, it's probably
Copy link
Member

Choose a reason for hiding this comment

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

This would be way easier to read and a lot less lines if we'd wrote one setence per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would be but that's part of #263082. I feel like if that got merged it would force us to address the cross-compilation issues.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@kjeremy
Copy link
Contributor Author

kjeremy commented Jul 18, 2024

cryptography seems to build for me now

@kjeremy kjeremy closed this Jul 18, 2024
@kjeremy kjeremy deleted the fix-pyo3-python branch July 18, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 6.topic: python 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: python-cryptography fails to cross compile for armv7l
5 participants